include a flag to disable metric prefixes #2198

dprotaso · 2021-07-22T18:26:19Z

the default continues to include the prefix

Part of: #2174

the default continues to include the prefix

knative-prow-robot · 2021-07-22T18:26:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso

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:

~~metrics/OWNERS~~ [dprotaso]

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

codecov · 2021-07-22T18:32:45Z

Codecov Report

Merging #2198 (ad7ef27) into main (d9b7180) will increase coverage by 0.01%.
The diff coverage is 88.23%.

@@            Coverage Diff             @@
##             main    #2198      +/-   ##
==========================================
+ Coverage   65.98%   65.99%   +0.01%     
==========================================
  Files         220      220              
  Lines        9204     9213       +9     
==========================================
+ Hits         6073     6080       +7     
- Misses       2859     2860       +1     
- Partials      272      273       +1

Impacted Files	Coverage Δ
metrics/config.go	`76.69% <50.00%> (-1.08%)`	⬇️
metrics/config_observability.go	`92.59% <100.00%> (+0.59%)`	⬆️
metrics/opencensus_exporter.go	`65.90% <100.00%> (ø)`
metrics/prometheus_exporter.go	`90.32% <100.00%> (+1.03%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9b7180...ad7ef27. Read the comment docs.

dprotaso · 2021-07-22T18:34:33Z

/assign @skonto @evankanderson @upodroid

dprotaso · 2021-07-22T19:13:35Z

I'm going to get rid of that downstream serving test that tests logic that exists in knative.dev/pkg

dprotaso · 2021-07-22T19:13:42Z

/hold

dprotaso · 2021-07-22T19:27:44Z

/hold cancel
ok done: knative/serving#11711

skonto · 2021-07-26T11:56:06Z

metrics/config_observability.go

+	// When enabled:
+	// - OpenCensus exporter prefixes the domain and component
+	// - Prometheus exporter prefixes the component name (as a prom namespace)
+	EnableDeprecatedMetricPrefix bool


Since this is a breaking change and I see there is a plan here for mirgation, next question would be if there is a plan for removing this flag or users should just keep this flag on.

Probably worth surfacing this topic on the thread.

skonto · 2021-07-26T12:01:32Z

metrics/prometheus_exporter.go

-	e, err := prom.NewExporter(prom.Options{Namespace: config.component})
+	opts := prom.Options{}
+	if config.enableDeprecatedMetricPrefix {
+		opts.Namespace = config.component


Why we set the Namespace to be equal to a component name here (just curious)?

this is the legacy behaviour - which I want to preserve with this flag

dprotaso · 2021-07-28T19:37:37Z

bump @skonto @evankanderson

evankanderson · 2021-09-01T19:34:34Z

/assign @MontyCarter

I think Monty already said that this wouldn't mess up Google, so I'm all in favor.

skonto · 2021-09-02T07:28:44Z

@dprotaso I am fine with this as long as there is an option to define the component name etc.
Right now for metrics like source_name_event_count ambiguity/conflict will be created when all metrics will be gathered at the Prometheus side if we completely remove the prefix. So the flag/prefix combination is useful otherwise we will have to find some other way to distinguish metrics eg. add a label or something.

/lgtm

hold for @MontyCarter

skonto · 2021-09-02T07:33:08Z

/hold

dprotaso · 2021-09-02T13:49:03Z

Yeah the component name would have to move to a label. I'm just introducing this flag so folks can set it explicitly to true prior to us changing the default in the future

MontyCarter · 2021-09-07T19:39:40Z

Thanks for confirming with us.

Sorry for the delay here. I've switched teams; you should probably update your contact on metrics to @ZhiminXiang.

With the ability to keep the legacy behavior in this PR:
/lgtm

Looking forward, Google has already switched to a collector based setup; metric name/label rewrites are no problem.

With regards to the component name, as long as this is available as a label, we can rewrite things in the collector config.

With regards to domain portion of MetricPrefix, our domain value changes with component. It really would be nice to retain the ability to read domain into the config and pipe that through to a label if we get rid of support for MetricPrefix.

Acceptable paths forward for us would include:

Keeping MetricsPrefix behavior as it is in this PR.
Retaining MetricsPrefix legacy behavior, but make the enableDeprecatedMetricPrefix default false (we must opt in).
Continuing to read domain and component into config, but passing these as labels instead.

knative-prow-robot · 2021-11-23T14:15:48Z

@dprotaso: PR needs rebase.

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.

skonto · 2022-01-28T11:19:48Z

@dprotaso gentle ping, been a while, planning to continue this?

github-actions · 2022-07-18T01:35:14Z

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Reopen with /reopen. Mark as fresh by adding the
comment /remove-lifecycle stale.

include a flag to disable metric prefixes

ad7ef27

the default continues to include the prefix

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 22, 2021

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 22, 2021

dprotaso changed the title ~~include a flag to disable metric prefixes the default continues to include the prefix~~ include a flag to disable metric prefixes Jul 22, 2021

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 22, 2021

knative-prow-robot assigned evankanderson, skonto and upodroid Jul 22, 2021

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 22, 2021

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 22, 2021

skonto reviewed Jul 26, 2021

View reviewed changes

skonto mentioned this pull request Jul 30, 2021

[Experimental] Knative Eventing Metrics (v0.24) - does not contain the documented metrics... and also missing count. knative/eventing#5617

Closed

knative-prow-robot assigned MontyCarter Sep 1, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2021

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 2, 2021

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 23, 2021

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 18, 2022

github-actions bot closed this Aug 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

include a flag to disable metric prefixes #2198

include a flag to disable metric prefixes #2198

dprotaso commented Jul 22, 2021 •

edited

Loading

knative-prow-robot commented Jul 22, 2021

codecov bot commented Jul 22, 2021 •

edited

Loading

dprotaso commented Jul 22, 2021

dprotaso commented Jul 22, 2021

dprotaso commented Jul 22, 2021

dprotaso commented Jul 22, 2021

skonto Jul 26, 2021

dprotaso Jul 26, 2021

skonto Jul 26, 2021 •

edited

Loading

dprotaso Jul 26, 2021

dprotaso commented Jul 28, 2021

evankanderson commented Sep 1, 2021

skonto commented Sep 2, 2021 •

edited

Loading

skonto commented Sep 2, 2021

dprotaso commented Sep 2, 2021

MontyCarter commented Sep 7, 2021 •

edited

Loading

knative-prow-robot commented Nov 23, 2021

skonto commented Jan 28, 2022

github-actions bot commented Jul 18, 2022

include a flag to disable metric prefixes #2198

include a flag to disable metric prefixes #2198

Conversation

dprotaso commented Jul 22, 2021 • edited Loading

knative-prow-robot commented Jul 22, 2021

codecov bot commented Jul 22, 2021 • edited Loading

Codecov Report

dprotaso commented Jul 22, 2021

dprotaso commented Jul 22, 2021

dprotaso commented Jul 22, 2021

dprotaso commented Jul 22, 2021

skonto Jul 26, 2021

Choose a reason for hiding this comment

dprotaso Jul 26, 2021

Choose a reason for hiding this comment

skonto Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

dprotaso Jul 26, 2021

Choose a reason for hiding this comment

dprotaso commented Jul 28, 2021

evankanderson commented Sep 1, 2021

skonto commented Sep 2, 2021 • edited Loading

skonto commented Sep 2, 2021

dprotaso commented Sep 2, 2021

MontyCarter commented Sep 7, 2021 • edited Loading

knative-prow-robot commented Nov 23, 2021

skonto commented Jan 28, 2022

github-actions bot commented Jul 18, 2022

dprotaso commented Jul 22, 2021 •

edited

Loading

codecov bot commented Jul 22, 2021 •

edited

Loading

skonto Jul 26, 2021 •

edited

Loading

skonto commented Sep 2, 2021 •

edited

Loading

MontyCarter commented Sep 7, 2021 •

edited

Loading