Fix pipelinerun_taskrun_duration_seconds metric #2849
Conversation
This CL changes reporting taskrun duration as part of pipelinerun metric to use knative metrics Record API to match with the rest of the existing metrics. We have been using opensensus stats Record API for reporting pipelinerun_taskrun_duration_seconds metric as oppose to other metrics that we used knative metrics API to report them. At this point only the metrics that are reported with knative metrics are being correctly plumbed to metrics backend. So as a result pipelinerun_taskrun_duration_seconds was missing on the metrics backend.
tekton-robot
added
size/XS
|
Hi @NavidZ. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
2 similar comments
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
|
@hrishin do you happen to remember why we were using stats.Record in this instance? I can see the unittests are passing either way but I wasn't able to see the metric on StackDriver in the current state and using knative metrics.Record fixed it. |
|
/kind bug |
|
/ok-to-test |
tekton-robot
added
ok-to-test
|
/retest |
Wow, thats good catch @NavidZ, thank you! |
|
/retest |
|
[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 |
tekton-robot
added
the
approved
|
@vdemeester the test failures seem weird. I wasn't able to reproduce them locally. I'm still looking into it.
@hrishin Alrighty. Good to know that it is safe to do have it used metrics.Record. |
|
/retest |
|
Just to link the issues. This change is related to #2814 |
Changes
This CL changes reporting taskrun duration as
part of pipelinerun metric to use knative
metrics Record API to match with the rest of the
existing metrics.
We have been using opensensus stats Record API for
reporting pipelinerun_taskrun_duration_seconds metric
as oppose to other metrics that we used knative metrics
API to report them. At this point only the metrics
that are reported with knative metrics are being
correctly plumbed to metrics backend. So as a result
pipelinerun_taskrun_duration_seconds was missing on
the metrics backend.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmddir, please updatethe release Task to build and release this image.
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.