webhook: register metrics only with default stats reporter #3005
Conversation
This is a followup from [knative#2931][1]. RegisterMetrics may register unwanted metrics if the default stats reporter is not used. [1]: knative#2931
knative-prow
bot
added
the
kind/cleanup
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zhouhaibing089 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow
bot
added
needs-ok-to-test
|
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 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. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3005 +/- ##
==========================================
+ Coverage 78.75% 78.77% +0.01%
==========================================
Files 188 188
Lines 11107 8870 -2237
==========================================
- Hits 8747 6987 -1760
+ Misses 2094 1617 -477
Partials 266 266 ☔ View full report in Codecov by Sentry. |
|
/ok-to-test |
knative-prow
bot
added
ok-to-test
| @@ -147,6 +152,8 @@ func New( | |||
| logger := logging.FromContext(ctx) | |||
|
|
|||
| if opts.StatsReporter == nil { | |||
| // Register webhook metrics | |||
| metricsOnce.Do(func() { RegisterMetrics(opts.StatsReporterOptions...) }) | |||
There was a problem hiding this comment.
Before RegisterMetrics would always be called. Now it's only being invokved when the opts.StatsReporter isnt' set.
This seems like a breaking change
There was a problem hiding this comment.
I agree with your assessment. It might break users who specify StatsReporter yet still rely on RegisterMetrics to register views properly.
However I consider this as an unpleasant situation when users want to use a different StatsReporter but register the same view with slightly different tags.
To be fair, this is not something that I need anymore since the default StatsReporter can be customized already.
As it is today, webhook.NewStatsReporter() and webhook.RegisterMetrics() seems to be independent interfaces while they actually share the same data structure (tracks the same metrics tags, and relies on the same opts.StatsReporterOptions to initialize).
There was a problem hiding this comment.
@skonto do you have any opinions?
There was a problem hiding this comment.
I think a migration path would be to have a flag disableDefaultMetrics with default being false.
|
This Pull Request is stale because it has been open for 90 days with |
github-actions
bot
added
lifecycle/stale
This is a followup from knative/pkg#2931. RegisterMetrics may register unwanted metrics if the default stats reporter is not used.
/kind cleanup