add namespace label/tag to non-deprecated throttle metrics #7879

gabemontero · 2024-04-12T12:06:44Z

Changes

Back when implementing #6744 for #6631 we failed to realize that with k8s quota policies being namespace scoped, knowing which namespace the throttled items were in could have some diagnostic value.

Now that we have been using the metric added for a bit, this realization is now very apparent.

This changes introduces the namespace tag. Also, since last touching this space, the original metric was deprecated and a new one with a shorter name was added. This change only updates the non-deprecated metric with the new label.

Lastly, the default behavior is preserved, and use of the new label only occurs when explicitly enabled in observability config map.

Fixes #7878

/kind feature

Submitter Checklist

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

[/ ] Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
[/ ] Has Tests included if any functionality added or changed
[ /] pre-commit Passed
[ /] Follows the commit message standard
[ /] Meets the Tekton contributor standards (including functionality, content, code)
[ /] Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
[ /] Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
[ /] Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

Add 'namespace' label/tag to the 'tekton_pipelines_controller_running_taskruns_throttled_by_quota' and 'tekton_pipelines_controller_running_taskruns_throttled_by_node' metrics, as kubernetes quota definitions are namespace scoped, hence certain namespaces may be more susceptible to quota throttling than others, and in a multi-node environment, not all namespaces are necessarily on the same node.

To enable this new label/tag, set 'metrics.taskrun.throttle.enable-namespace' to 'true' in the 'config-observability' ConfigMap

tekton-robot · 2024-04-12T12:14:33Z

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

File	Old Coverage	New Coverage	Delta
pkg/taskrunmetrics/metrics.go	84.8%	86.3%	1.5

tekton-robot · 2024-04-12T12:18:02Z

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

File	Old Coverage	New Coverage	Delta
pkg/taskrunmetrics/metrics.go	84.8%	86.3%	1.5

tekton-robot · 2024-04-12T13:23:28Z

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

File	Old Coverage	New Coverage	Delta
pkg/taskrunmetrics/metrics.go	84.8%	85.5%	0.7

tekton-robot · 2024-04-12T13:25:56Z

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

File	Old Coverage	New Coverage	Delta
pkg/taskrunmetrics/metrics.go	84.8%	85.5%	0.7

tekton-robot · 2024-04-12T13:59:09Z

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

File	Old Coverage	New Coverage	Delta
pkg/taskrunmetrics/metrics.go	84.8%	85.5%	0.7

tekton-robot · 2024-04-12T14:02:38Z

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

File	Old Coverage	New Coverage	Delta
pkg/taskrunmetrics/metrics.go	84.8%	85.5%	0.7

gabemontero · 2024-04-16T13:52:01Z

ping @khrm - can you take a look when you get the chance?

also, see my question in the description wrt if adding labels to existing experimental metrics is "OK"

khrm

We can add this behind a flag also. Like we do for count.reason

gabemontero · 2024-04-16T20:46:38Z

We can add this behind a flag also. Like we do for count.reason

Ah you mean https://github.com/tektoncd/pipeline/blob/main/docs/metrics.md#configuring-metrics-using-config-observability-configmap

Before I make the change, how about an early review on the name .... say

metrics.taskrun.throttle.enable-namespace

for the new flag's name @khrm ?

Thanks for the pointer.

tekton-robot · 2024-04-23T15:26:35Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/metrics.go	73.7%	76.2%	2.5
pkg/taskrunmetrics/metrics.go	84.8%	85.7%	1.0

tekton-robot · 2024-04-23T15:28:22Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/metrics.go	73.7%	76.2%	2.5
pkg/taskrunmetrics/metrics.go	84.8%	85.7%	1.0

gabemontero · 2024-04-23T16:09:57Z

ok @khrm new flag to enable the namespace label on the throttle metric is added ... PTAL

tekton-robot · 2024-04-24T19:12:19Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/metrics.go	73.7%	76.2%	2.5
pkg/taskrunmetrics/metrics.go	84.8%	85.7%	1.0

tekton-robot · 2024-04-24T19:22:32Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/metrics.go	73.7%	76.2%	2.5
pkg/taskrunmetrics/metrics.go	84.8%	85.7%	1.0

afrittoli

Thanks @gabemontero
/lgtm

gabemontero · 2024-04-29T11:24:54Z

Thanks @gabemontero /lgtm

likewise thanks @afrittoli

question to you and @khrm - any suggestion on who I should reach out to in order to get the approve label (minimally assuming @khrm you are fine with my updates from your comments) ?

khrm · 2024-04-29T16:39:36Z

pkg/taskrunmetrics/metrics.go


+	for ns, cnt := range trsThrottledByQuota {
+		ctx, err = tag.New(ctx, []tag.Mutator{tag.Insert(namespaceTag, ns)}...)


This doesn't behave properly for metrics.taskrun.throttle.enable-namespace : false.

good catch ... I've updated the code path and unit test to cover both flavors of the new config option - thanks

khrm · 2024-04-29T16:41:00Z

Yes, this is fine.

tekton-robot · 2024-04-30T18:14:06Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/metrics.go	73.7%	76.2%	2.5
pkg/taskrunmetrics/metrics.go	84.8%	86.1%	1.3

tekton-robot · 2024-04-30T18:17:05Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/metrics.go	73.7%	76.2%	2.5
pkg/taskrunmetrics/metrics.go	84.8%	86.1%	1.3

Back when implementing tektoncd#6744 for tektoncd#6631 we failed to realize that with k8s quota policies being namespace scoped, knowing which namespace the throttled items were in could have some diagnostic value. Now that we have been using the metric added for a bit, this realization is now very apparent. This changes introduces the namespace tag. Also, since last touching this space, the original metric was deprecated and a new one with a shorter name was added. This change only updates the non-deprecated metric with the new label. Lastly, the default behavior is preserved, and use of the new label only occurs when explicitly enabled in observability config map.

tekton-robot · 2024-05-01T01:15:49Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/metrics.go	73.7%	76.2%	2.5
pkg/taskrunmetrics/metrics.go	84.8%	86.1%	1.3

tekton-robot · 2024-05-01T01:19:44Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/metrics.go	73.7%	76.2%	2.5
pkg/taskrunmetrics/metrics.go	84.8%	86.1%	1.3

gabemontero · 2024-05-01T12:30:25Z

unrelated flake on alpha integration

/test pull-tekton-pipeline-alpha-integration-tests

gabemontero · 2024-05-01T14:26:55Z

@afrittoli @khrm PTAL at the updates from @khrm 's last review - thanks

khrm

/approve

We can optimize this later on.

@afrittoli @chitrangpatel

afrittoli

/approve

tekton-robot · 2024-05-08T13:08:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, khrm

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:

~~OWNERS~~ [afrittoli]

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

chitrangpatel · 2024-05-10T16:59:42Z

/lgtm

tekton-robot added kind/feature Categorizes issue or PR as related to a new feature. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. labels Apr 12, 2024

tekton-robot requested review from afrittoli and vdemeester April 12, 2024 12:06

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 12, 2024

gabemontero force-pushed the add-ns-quota-metrics branch from 1f01335 to c980ba4 Compare April 12, 2024 13:14

gabemontero force-pushed the add-ns-quota-metrics branch from c980ba4 to 35c90d3 Compare April 12, 2024 13:51

khrm reviewed Apr 16, 2024

View reviewed changes

gabemontero force-pushed the add-ns-quota-metrics branch from 35c90d3 to ab4ea8f Compare April 23, 2024 15:18

gabemontero force-pushed the add-ns-quota-metrics branch from ab4ea8f to 785e820 Compare April 24, 2024 19:02

afrittoli reviewed Apr 26, 2024

View reviewed changes

tekton-robot assigned afrittoli Apr 26, 2024

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 26, 2024

khrm reviewed Apr 29, 2024

View reviewed changes

gabemontero force-pushed the add-ns-quota-metrics branch from 785e820 to db76fe0 Compare April 30, 2024 18:06

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2024

gabemontero force-pushed the add-ns-quota-metrics branch from db76fe0 to 7bccfad Compare May 1, 2024 01:08

khrm reviewed May 6, 2024

View reviewed changes

afrittoli reviewed May 8, 2024

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 8, 2024

tekton-robot assigned chitrangpatel May 10, 2024

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 10, 2024

tekton-robot merged commit 5c40712 into tektoncd:main May 10, 2024
13 checks passed

gabemontero deleted the add-ns-quota-metrics branch May 10, 2024 18:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add namespace label/tag to non-deprecated throttle metrics #7879

add namespace label/tag to non-deprecated throttle metrics #7879

gabemontero commented Apr 12, 2024 •

edited

tekton-robot commented Apr 12, 2024

tekton-robot commented Apr 12, 2024

tekton-robot commented Apr 12, 2024

tekton-robot commented Apr 12, 2024

tekton-robot commented Apr 12, 2024

tekton-robot commented Apr 12, 2024

gabemontero commented Apr 16, 2024

khrm left a comment

gabemontero commented Apr 16, 2024

tekton-robot commented Apr 23, 2024

tekton-robot commented Apr 23, 2024

gabemontero commented Apr 23, 2024

tekton-robot commented Apr 24, 2024

tekton-robot commented Apr 24, 2024

afrittoli left a comment

gabemontero commented Apr 29, 2024

khrm Apr 29, 2024

gabemontero Apr 30, 2024

khrm commented Apr 29, 2024

tekton-robot commented Apr 30, 2024

tekton-robot commented Apr 30, 2024

tekton-robot commented May 1, 2024

tekton-robot commented May 1, 2024

gabemontero commented May 1, 2024

gabemontero commented May 1, 2024

khrm left a comment

afrittoli left a comment

tekton-robot commented May 8, 2024

chitrangpatel commented May 10, 2024


		for ns, cnt := range trsThrottledByQuota {
		ctx, err = tag.New(ctx, []tag.Mutator{tag.Insert(namespaceTag, ns)}...)

add namespace label/tag to non-deprecated throttle metrics #7879

add namespace label/tag to non-deprecated throttle metrics #7879

Conversation

gabemontero commented Apr 12, 2024 • edited

Changes

Submitter Checklist

Release Notes

tekton-robot commented Apr 12, 2024

tekton-robot commented Apr 12, 2024

tekton-robot commented Apr 12, 2024

tekton-robot commented Apr 12, 2024

tekton-robot commented Apr 12, 2024

tekton-robot commented Apr 12, 2024

gabemontero commented Apr 16, 2024

khrm left a comment

Choose a reason for hiding this comment

gabemontero commented Apr 16, 2024

tekton-robot commented Apr 23, 2024

tekton-robot commented Apr 23, 2024

gabemontero commented Apr 23, 2024

tekton-robot commented Apr 24, 2024

tekton-robot commented Apr 24, 2024

afrittoli left a comment

Choose a reason for hiding this comment

gabemontero commented Apr 29, 2024

khrm Apr 29, 2024

Choose a reason for hiding this comment

gabemontero Apr 30, 2024

Choose a reason for hiding this comment

khrm commented Apr 29, 2024

tekton-robot commented Apr 30, 2024

tekton-robot commented Apr 30, 2024

tekton-robot commented May 1, 2024

tekton-robot commented May 1, 2024

gabemontero commented May 1, 2024

gabemontero commented May 1, 2024

khrm left a comment

Choose a reason for hiding this comment

afrittoli left a comment

Choose a reason for hiding this comment

tekton-robot commented May 8, 2024

chitrangpatel commented May 10, 2024

gabemontero commented Apr 12, 2024 •

edited