prometheus: Add support for setting Translation Strategy config option

[ywwg]

As per the specification: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk_exporters/prometheus.md#configuration

This is part of a broader effort to unify the behavior of all the touch points between open telemetry metrics and prometheus: prometheus/prometheus#16542

Fixes #6668

[codecov]

Codecov Report

❌ Patch coverage is 79.80769% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.0%. Comparing base (3342341) to head (943b82a).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
exporters/prometheus/exporter.go	72.0%	15 Missing and 6 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #7111     +/-   ##
=======================================
- Coverage   83.0%   83.0%   -0.1%     
=======================================
  Files        265     265             
  Lines      24788   24850     +62     
=======================================
+ Hits       20583   20628     +45     
- Misses      3830    3842     +12     
- Partials     375     380      +5     

Files with missing lines	Coverage Δ
exporters/prometheus/config.go	100.0% <100.0%> (ø)
exporters/prometheus/exporter.go	84.1% <72.0%> (-2.1%)	⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ArthurSens]

First round of review, couldn't find the time to properly test things myself 😬

[ArthurSens]

Hmmm, something here isn't working with the WithNamespace config option. I tried running the go-contrib's prometheus example, but I've added a replace directive to the go.mod with your branch.

On line 34, I've changed the following:

exporter, err := prometheus.New(prometheus.WithNamespace("my-strange-namespace"), prometheus.WithTranslationStrategy(otlptranslator.UnderscoreEscapingWithSuffixes))

I've run the example and curl'ed the metrics endpoint with the following command:

curl -H 'Accept: application/openmetrics-text;version=1.0.0;escaping=allow-utf-8;q=0.5,application/openmetrics-text;version=0.0.1;q=0.4,text/plain;version=1.0.0;escaping=allow-utf8;q=0.3,text/plain;version=0.0.4;q=0.2,/;q=0.1' localhost:2223/metrics

And I've got UTF-8 characters (see dashes in the namespace):

# HELP "my-strange-namespace_foo_total" a simple counter
# TYPE "my-strange-namespace_foo_total" counter
{"my-strange-namespace_foo_total",A="B",C="D",otel_scope_my_custom_attribute="test",otel_scope_name="go.opentelemetry.io/contrib/examples/prometheus",otel_scope_schema_url="",otel_scope_version=""} 5

[ywwg]

And I've got UTF-8 characters (see dashes in the namespace):

fixed and added test

[ywwg]

I do not understand the codespell failure

[ywwg]

oh I figured it out, the error was in a previous CI step.

[ArthurSens]

Alright, tested a few different combinations of translation strategies, content type headers, and whether type/unit suffixes were deduplicated from names or not, and everything seems to work correctly :)

The only thing I'm concerned about now is how this affects the Collector configuration surface. I honestly have no idea how this works 😅

@dmathieu, @jade-guiton-dd, is it safe to deprecate the WithoutUnits option? Will this affect the collector config file format?

[ywwg]

I did note that the prometheus otel endpoint also has a separate flag for counter suffixes, so maybe we do need that one.

[ywwg]

I'm going to proceed assuming we want to keep the WithCounterSuffixes option as non-deprecated, but deprecate WithUnits

[ArthurSens]

I did note that the prometheus otel endpoint also has a separate flag for counter suffixes, so maybe we do need that one.

Could you share where have you seen this? I don't see it in our configuration page 🤔

[ywwg]

nevermind, I was mistaken -- it's only in the otel-go sdk

[jade-guiton-dd]

is it safe to deprecate the WithoutUnits option? Will this affect the collector config file format?

The Collector uses go-contrib/otelconf to implement the config schema defined in opentelemetry-configuration.

As long as otelconf keeps its stability promises (by choosing to ignore the deprecation, or by implementing without_units using translation_strategy under the hood), I don't think the Collector's config should be directly affected.

[ywwg]

The Collector uses go-contrib/otelconf to implement the config schema defined in opentelemetry-configuration.

Ah, I haven't seen these before. Is that a user-facing set of structs? Since we are moving toward translation strategies across the board I'd prefer these objects to be in sync with the spec and SDK as well (by adding TranslationStrategy and deprecating WithoutTypeSuffix/WithoutUnits)

[ArthurSens]

Ah, I haven't seen these before. Is that a user-facing set of structs? Since we are moving toward translation strategies across the board I'd prefer these objects to be in sync with the spec and SDK as well (by adding TranslationStrategy and deprecating WithoutTypeSuffix/WithoutUnits)

It is, this struct turns into the collector config file and will coordinate how the collector internal metrics are exposed

[ArthurSens]

@pellared, do you happen to have some time to review this PR while David is on PTO? We depend on getting this in to upgrade otel-go and prometheus dependencies in the collector 😅

[pellared]

@ArthurSens, made a quick review (I am low on time at the moment).

[ywwg]

The problem I'm running in to now is that otel-go, uniquely, has two separate settings for suffixes, type and unit separately, whereas the other projects only have one. This would require a large set of translation strategies (8) to cover all of the permutations implied.

Although in the meetings we have said that the new option (WithTranslationStrategy) should override the old one, I think that's not actually the best way to do it. I think it is ok for a user to say:

"I want underscore escaping with suffixes, disable unit suffixes, disable counter suffixes." This is a bit like "I'd like a hamburger, hold the bun," but it's logically consistent. Although this request is the same as "I want underscore escaping without suffixes," if someone wants to write it this way, they can.

By allowing this, we can allow for "I want underscore escaping with suffixes, disable unit suffixes ONLY," something that the tests check for.

Since there is no way to generate an option that adds suffixes, we don't need to worry about inverse cases ("I want no suffixes, but add suffixes").

I have updated the code accordingly. But note @ArthurSens that this contradicts what we talked about in the spec meeting so we will have to be careful about how we document this.

I am still deprecating the "withoutXSuffixes" options -- people should not use them!

[ArthurSens]

I have updated the code accordingly. But note @ArthurSens that this contradicts what we talked about in the spec meeting so we will have to be careful about how we document this.

In the spec meeting, we were talking about configuration files. This is a different case.

If something has its Prometheus configuration handled by YAML files, we have no idea what comes first and what comes second when we parse this file. So we can't make assumptions and just say that translation_strategy overrides the others.

We're building a functional builder pattern on this PR that you're working on, and the situation is the complete opposite. Ordering is easy to notice, and making overrides is the hard thing to implement :)

[ywwg]

We're building a functional builder pattern on this PR that you're working on, and the situation is the complete opposite. Ordering is easy to notice, and making overrides is the hard thing to implement :)

Yes, but after working with the code, the situation now is that even in the functional builder, order does not matter. Let me know if the new behavior is still unclear -- it's not quite a matter of one option "overriding" the other.

[ywwg]

status: Waiting for @pellared to come back from PTO for second approval

[ywwg]

squashed down

[ywwg]

merge? all comments resolved, last fixes applied

[ywwg]

thank you all for your reviews!

[pellared]

thank you all for your reviews!

Thank you for your contribution