webhook: add options to disable resource_namespace tag in metrics

[zhouhaibing089]

To add some context, historically, resource_name was removed from this tag list due to its high potential of causing high metrics cardinality. See knative/pkg#1464 for more information.

While that's great, but it might not be sufficient for large scale use cases where namespaces can be super dynamic (with generateName, too) or grows fase enough. There is an issue report from tektoncd/pipeline#3171 which talks about this.

This proposal makes it possible to disable resource_namespace tag via an option function. The default behavior is not changed, so no user impact if any of existing users rely on this tag. There is no API contract change either due to the beauty of variadic functions.

Now downstream projects can consume this by override StatsReporter in webhook context options with their own preference. As a caveat here, if downstream project does choose to override StatsReporter, the default ReportMetrics function shouldn't be called by default as they may now have a different set of tag keys to report. As such, this function is only called if the default StatsReporter is used.

webhook: add options to disable resource_namespace tag in metrics

[knative-prow[bot]]

Welcome @zhouhaibing089! It looks like this is your first PR to knative/pkg 🎉

[knative-prow[bot]]

Hi @zhouhaibing089. Thanks for your PR.

I'm waiting for a knative 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.

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.

[dprotaso]

/hold we have a release in the next week

Will revisit this after

[dprotaso]

/ok-to-test

[zhouhaibing089]

/retest

(The failed tests reported seem to be fine locally)

[dprotaso]

(The failed tests reported seem to be fine locally)

They're consistent in CI - try using a docker container with ubuntu?

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 95.74468% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 78.75%. Comparing base (6b13f01) to head (46a2e06). Report is 20 commits behind head on main.

Files	Patch %	Lines
webhook/stats_reporter.go	95.69%	2 Missing and 2 partials :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2931      +/-   ##
==========================================
+ Coverage   78.68%   78.75%   +0.07%     
==========================================
  Files         188      188              
  Lines       11051    11107      +56     
==========================================
+ Hits         8695     8747      +52     
- Misses       2092     2094       +2     
- Partials      264      266       +2     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[zhouhaibing089]

Kindly ping, may I get some attention here?

[skonto]

Hi @zhouhaibing089 sorry for the delay, just to clarify here we have two changes: a) avoid resourceNamespaceKey if someone sets a custom reporter using WithoutResourceNamespace() (in Webhook options) b) register metrics only when the default reporter is used.

I think it would be more flexible if we can exclude an arbitrary tag key instead of having if statements like:

if r.opts.resourceNamespace {
	mutators = append(mutators, tag.Insert(resourceNamespaceKey, req.Namespace))
}

We could use:

type options struct {
	tagsToExclude    []tag.Key
}

wdyth?

[zhouhaibing089]

I think it would be more flexible if we can exclude an arbitrary tag key..

Thanks. I completely agree (PR updated with your suggestion).

[zhouhaibing089]

/retest

[dprotaso]

Overall looks great thanks for the changes - some minor stuff

[dprotaso]

/hold cancel

[dprotaso]

/lgtm /approve

thanks @zhouhaibing089 🎉

[knative-prow[bot]]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, zhouhaibing089

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~~ [dprotaso]

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