-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adjust the way {pipeline,task}run
metrics are surfaced.
#4204
Conversation
The following is the coverage report on the affected files.
|
It's been bugging me that `NewController` methods for the `TaskRun` and `PipelineRun` controllers launch a go routine to harvest metrics, and it occurred to me that there might be a better way. Borrowing from the CloudEvent client's use of hand-crafted injection logic: https://github.com/tektoncd/pipeline/blob/7297c48d26da98552be4ee3c50d94a130bd8e79d/pkg/reconciler/events/cloudevent/cloudeventclient.go#L29 This change takes a similar approach, creating `pkg/{task,pipeline}runmetrics` packages, which surface their `*Recorder` via `.Get(ctx)` methods. Rather than the controller spinning off go routine, this piggybacks on informer injection to "Start()" that process after the dependent informers have been started.
8c5c126
to
b774a42
Compare
The following is the coverage report on the affected files.
|
/retest the alpha tests are very flaky 😞 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
Hmm, the race detector blew up, gonna take a quick look at where |
I don't see the usual stack trace in the blended |
Nothing at 10, going for 100 while I walk the dog :) |
Well all I got was:
Let's see if prow can repro it. /retest |
I guess not. |
It's been bugging me that
NewController
methods for theTaskRun
andPipelineRun
controllers launch a go routine to harvest metrics, and it occurred to me that there might be a better way.Borrowing from the CloudEvent client's use of hand-crafted injection logic:
pipeline/pkg/reconciler/events/cloudevent/cloudeventclient.go
Line 29 in 7297c48
This change takes a similar approach, creating
pkg/{task,pipeline}runmetrics
packages, which surface their*Recorder
via.Get(ctx)
methods. Rather than the controller spinning off go routine, this piggybacks on informer injection to "Start()" that process after the dependent informers have been started./kind cleanup
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
Release Notes