feat: add error metrics for PipelineRun and TaskRun signing failures by mathur07 · Pull Request #1325 · tektoncd/chains

mathur07 · 2025-03-17T06:21:05Z

This commit adds new error metrics to track signing failures for both PipelineRun and TaskRun artifacts.

Error recording in the signing logic with error_type tags.
Updated metrics and tests in both pipelinerunmetrics and taskrunmetrics.

Part-resolution: Jira Issue & #984

Exposing error metrics allows us to utilize other metrics along with this to view the error rate in Grafana Dashboard.

Changes

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Has Docs included if any changes are user facing
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

Metrics added for PipelineRun and TaskRun signing failures (`watcher_pipelinerun_signing_failures_total` and `watcher_taskrun_signing_failures_total`)

linux-foundation-easycla · 2025-03-17T06:21:11Z

The committers listed above are authorized under a signed CLA.

✅ login: aThorp96 / name: Andrew Thorp (2919ef1)
✅ login: mathur07 / name: Shubham Mathur (3f3bcd8)

tekton-robot · 2025-03-17T06:21:14Z

Hi @mathur07. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

aThorp96

LGTM! One optional suggestion. Approval pending go fmt

jkhelil · 2025-03-21T14:24:52Z

@mathur07 can you go thought easycla process, ?
#1325 (comment)

Would you also Remove the WIP on your PR description

aThorp96 · 2025-05-02T19:27:25Z

+					if (signableType.Type() == "TaskRunArtifact" || signableType.Type() == "PipelineRunArtifact") && o.Recorder != nil {
+						o.Recorder.RecordErrorMetric(ctx, "storage")
+					}


The signableType.Type() == "..." logic appears a lot. Could this be abstracted? E.g.:

if err := b.StorePayload(ctx, tektonObj, rawPayload, string(signature), storageOpts); err != nil { logger.Error(err) o.recordErrorMetrics(ctx, signableType.Type(), "storage") } // ... func (o *ObjectSigner) recordErrorMetric(ctx context.Context, signableType string, errType string) { recordableTypes := []string{"TaskRunArtifact", "PipelineRunArtifact"} if o.Recorder != nil && slices.Contains(recordableTypes, signableType) { o.Recorder.RecordErrorMetric(ctx, errType) } }

i have abstracted it:
func (o *ObjectSigner) shouldRecordError(kind string) bool { return kind == "TaskRunArtifact" || kind == "PipelineRunArtifact" }

Let me know if it looks good. TY!

Thanks @mathur07! It's a bit better, however there is still a fair bit of duplication. Any objection to consolidating the shouldRecordErrorMetric and o.Recorder != nil into the same function like in my example?

+1
Use constant for TaskRunArtifact and PipelineRunArtifact kinds

mathur07 · 2025-05-05T20:56:00Z

@khrm can you PTAL too. TY!

aThorp96

/lgtm

tekton-robot · 2025-05-06T19:32:19Z

@aThorp96: changing LGTM is restricted to collaborators

Details

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mathur07 · 2025-05-08T15:02:33Z

@anithapriyanatarajan can you PTAL TY!

aThorp96 · 2025-05-17T09:20:05Z

+	// New error metric for PipelineRun signing failures, defined as a Float64 measure.
+	pipelineRunErrorCount = stats.Float64(
+		chains.PipelineRunErrorCountName,
+		chains.PipelineRunErrorCountDesc,
+		stats.UnitDimensionless,
+	)
+
+	// Define a view for the PipelineRun error metric with an error_type tag.
+	pipelineErrorView *view.View
+
+	// Tag key for error type.
+	errorTypeKey, _ = tag.NewKey("error_type")


I meant to include these comments in my previous request to remove superfluous comments. Do you mind removing these comments?

jkhelil · 2025-05-19T08:33:05Z

 			if err != nil {
 				logger.Error(err)
+				if o.shouldRecordError(signableType.Type()) && o.Recorder != nil {
+					o.Recorder.RecordErrorMetric(ctx, "payload_creation")


use constant for error types

jkhelil · 2025-05-19T08:33:27Z

-				logger.Warnf("Unable to marshal payload: %v", signerType, obj)
+				logger.Warnf("Unable to marshal payload for %s: %v", signerType, err)
+				if o.shouldRecordError(signableType.Type()) && o.Recorder != nil {
+					o.Recorder.RecordErrorMetric(ctx, "marshal_payload")


same here error types better use constant

E.g.:

type TaskRunErrorMetric string const ( TaskRunMarshalPayloadError TaskRunErrorMetric = "marshal_payload" TaskRunPayloadCreationError = "payload_creation" ... ) func RecordErrorMetric(ctx context.Context, errType TaskRunErrorMetric) { ... }

ack.
made requested changes PTAL

This commit adds new error metrics to track signing failures for both PipelineRun and TaskRun artifacts. - Error recording in the signing logic with error_type tags. - Updated metrics and tests in both pipelinerunmetrics and taskrunmetrics.

aThorp96

Some context around the changes included in my refactor

aThorp96 · 2025-05-27T19:26:07Z

This file seemed to define constants not for chains, but for metrics. Since there are metrics-specific modules, it didn't make sense to me to define them here. It also is an antipattern in my experience to define modules and files to group things by type (like constants) rather than domain (like metrics). The contents of this file have been split between three files based on domain: pkg/pipelinerunmetrics/metrics.go, pkg/taskrunmetrics/metrics.go, and a new file pkg/metrics/common.go.

aThorp96 · 2025-05-27T19:28:00Z

-type MetricsRecorder interface {
-	RecordCountMetrics(ctx context.Context, MetricType string)
-}


This interface should is implemented by pipelinerunmetrics.Recorder and taskrunmetricsRecorder. So it's been moved to pkg/metrics/common.go, and the implementations can now enforce at the type level that they do implement the interface without a circular import...

aThorp96 · 2025-05-27T19:29:25Z

+
+// Recorder is an interface for any object which may record metrics
+type Recorder interface {
+	RecordCountMetrics(ctx context.Context, MetricType Metric)


Note that RecordCountMetrics changed its MetricType parameter from string to Metric

aThorp96 · 2025-05-27T19:30:15Z

+	pipelineRunSignedName     common.Metric = "pipelinerun_sign_created_total"
+	pipelineRunSignedDesc     string        = "Total number of signed messages for pipelineruns"
+	pipelineRunUploadedName   common.Metric = "pipelinerun_payload_uploaded_total"
+	pipelineRunUploadedDesc   string        = "Total number of uploaded payloads for pipelineruns"
+	pipelineRunStoredName     common.Metric = "pipelinerun_payload_stored_total"
+	pipelineRunStoredDesc     string        = "Total number of stored payloads for pipelineruns"
+	pipelineRunMarkedName     common.Metric = "pipelinerun_marked_signed_total"
+	pipelineRunMarkedDesc     string        = "Total number of objects marked as signed for pipelineruns"
+	pipelineRunErrorCountName common.Metric = "pipelinerun_signing_failures_total"
+	pipelineRunErrorCountDesc string        = "Total number of PipelineRun signing failures"


Since these are no longer in the chains package (they were only referenced in the pipelinerunmetrics package), they no longer need to be public, so the API is cleaned up a bit

aThorp96 · 2025-05-27T19:30:37Z

 )

-// Recorder holds keys for Tekton metrics
+var _ common.Recorder = &Recorder{}


See my above comment regarding the interface implementation enforcement

mathur07 · 2025-06-02T13:21:28Z

@jkhelil can you please take a look at the new changes? TY!

PuneetPunamiya · 2025-06-05T06:10:08Z

+)
+
+// shouldRecordError returns true if the artifact type is TaskRunArtifact or PipelineRunArtifact.
+func (o *ObjectSigner) shouldRecordError(kind string) bool {


I'm a bit confused with the name of function name and the logic inside the function

PuneetPunamiya · 2025-06-05T06:13:34Z

/approve

aThorp96 · 2025-06-05T14:10:13Z

/lgtm

tekton-robot · 2025-06-05T14:10:16Z

@aThorp96: changing LGTM is restricted to collaborators

Details

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

aThorp96 · 2025-06-05T14:11:24Z

/cc @lcarva do you mind reviewing this PR, if you have a chance?

tekton-robot · 2025-06-16T20:30:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aThorp96, lcarva, PuneetPunamiya

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [PuneetPunamiya,lcarva]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

lcarva · 2025-06-16T20:31:40Z

/lgtm

tekton-robot · 2025-06-16T20:39:04Z

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/chains/signing.go	73.3%	70.4%	-2.9
pkg/pipelinerunmetrics/metrics.go	84.6%	86.2%	1.6
pkg/taskrunmetrics/metrics.go	84.0%	85.7%	1.7

lcarva · 2025-06-16T20:46:21Z

/ok-to-test

tekton-robot · 2025-06-16T20:54:06Z

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/chains/signing.go	73.3%	70.4%	-2.9
pkg/pipelinerunmetrics/metrics.go	84.6%	86.2%	1.6
pkg/taskrunmetrics/metrics.go	84.0%	85.7%	1.7

tekton-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 17, 2025

tekton-robot requested review from PuneetPunamiya and lcarva March 17, 2025 06:21

aThorp96 reviewed Mar 21, 2025

View reviewed changes

Comment thread pkg/chains/constants.go Outdated

Comment thread pkg/pipelinerunmetrics/metrics.go Outdated

mathur07 force-pushed the KONFLUX-6804 branch from 80d0dc9 to 09077d9 Compare April 30, 2025 15:52

mathur07 requested a review from aThorp96 April 30, 2025 15:56

aThorp96 reviewed May 2, 2025

View reviewed changes

mathur07 force-pushed the KONFLUX-6804 branch 2 times, most recently from 5b6f340 to 475aef3 Compare May 5, 2025 20:52

mathur07 requested a review from aThorp96 May 5, 2025 20:54

aThorp96 approved these changes May 6, 2025

View reviewed changes

Comment thread pkg/taskrunmetrics/metrics.go Outdated

mathur07 force-pushed the KONFLUX-6804 branch from 475aef3 to 8000286 Compare May 7, 2025 18:03

mathur07 force-pushed the KONFLUX-6804 branch from 8000286 to 084d707 Compare May 15, 2025 19:17

aThorp96 reviewed May 17, 2025

View reviewed changes

jkhelil reviewed May 19, 2025

View reviewed changes

mathur07 force-pushed the KONFLUX-6804 branch from 084d707 to 93fbcf0 Compare May 27, 2025 16:30

mathur07 requested a review from jkhelil May 27, 2025 16:31

mathur07 force-pushed the KONFLUX-6804 branch from 93fbcf0 to 29e54dc Compare May 27, 2025 16:33

mathur07 force-pushed the KONFLUX-6804 branch from 29e54dc to 3f3bcd8 Compare May 27, 2025 17:17

aThorp96 reviewed May 27, 2025

View reviewed changes

chore: refactor metrics definitions and logic

2919ef1

mathur07 force-pushed the KONFLUX-6804 branch from 6c2f83d to 2919ef1 Compare June 3, 2025 14:10

PuneetPunamiya reviewed Jun 5, 2025

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2025

lcarva approved these changes Jun 16, 2025

View reviewed changes

tekton-robot assigned lcarva Jun 16, 2025

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 16, 2025

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 16, 2025

tekton-robot merged commit 6339bb1 into tektoncd:main Jun 16, 2025
16 checks passed

Uh oh!

Conversation

mathur07 commented Mar 17, 2025 • edited by lcarva Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Submitter Checklist

Release Notes

Uh oh!

linux-foundation-easycla Bot commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tekton-robot commented Mar 17, 2025

Uh oh!

aThorp96 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jkhelil commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mathur07 commented May 5, 2025

Uh oh!

aThorp96 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tekton-robot commented May 6, 2025

Uh oh!

mathur07 commented May 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mathur07 May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aThorp96 left a comment

Choose a reason for hiding this comment

Uh oh!

aThorp96 May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mathur07 commented Jun 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PuneetPunamiya commented Jun 5, 2025

Uh oh!

aThorp96 commented Jun 5, 2025

Uh oh!

tekton-robot commented Jun 5, 2025

Uh oh!

aThorp96 commented Jun 5, 2025

mathur07 commented Mar 17, 2025 •

edited by lcarva

Loading

linux-foundation-easycla Bot commented Mar 17, 2025 •

edited

Loading

jkhelil commented Mar 21, 2025 •

edited

Loading

mathur07 May 27, 2025 •

edited

Loading

aThorp96 May 27, 2025 •

edited

Loading