include a flag to disable metric prefixes

[dprotaso]

the default continues to include the prefix

Part of: https://github.com/knative/pkg/issues/2174

[knative-prow-robot]

[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[bot]]

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%)	:arrow_down:
metrics/config_observability.go	92.59% <100.00%> (+0.59%)	:arrow_up:
metrics/opencensus_exporter.go	65.90% <100.00%> (ø)
metrics/prometheus_exporter.go	90.32% <100.00%> (+1.03%)	:arrow_up:

---

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]

/assign @skonto @evankanderson @upodroid

[dprotaso]

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

[dprotaso]

/hold

[dprotaso]

/hold cancel ok done: https://github.com/knative/serving/pull/11711

[dprotaso]

bump @skonto @evankanderson

[evankanderson]

/assign @MontyCarter

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

[skonto]

@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]

/hold

[dprotaso]

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]

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]

@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]

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

[github-actions[bot]]

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.