feat(opentelemetry-exporter-prometheus): add translation strategy support#6653
feat(opentelemetry-exporter-prometheus): add translation strategy support#6653cjihrig wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6653 +/- ##
==========================================
- Coverage 95.54% 95.50% -0.04%
==========================================
Files 372 373 +1
Lines 12339 12377 +38
Branches 2829 2850 +21
==========================================
+ Hits 11789 11821 +32
- Misses 550 556 +6
🚀 New features to boost your workflow:
|
966df4c to
f8d727a
Compare
ArthurSens
left a comment
There was a problem hiding this comment.
Hello, hello, thanks for starting the effort!
I don't know JS at all, so I can't do a very complete review.
Looking at your tests, and comparing them with the ones we did for go (https://github.com/prometheus/otlptranslator/blob/main/metric_namer_test.go), it seems like a lot of edge cases aren't covered here. Is it worth covering them?
| // These should be sanitized, even if escaping is disabled. | ||
| const ATTR_OTEL_SCOPE_NAME_LABEL = | ||
| sanitizePrometheusMetricName(ATTR_OTEL_SCOPE_NAME); | ||
| const ATTR_OTEL_SCOPE_VERSION_LABEL = sanitizePrometheusMetricName( | ||
| ATTR_OTEL_SCOPE_VERSION | ||
| ); | ||
| const ATTR_OTEL_SCOPE_SCHEMA_URL_LABEL = sanitizePrometheusMetricName( | ||
| ATTR_OTEL_SCOPE_SCHEMA_URL | ||
| ); |
There was a problem hiding this comment.
Could you clarify why this needs to be sanitized even if the translation strategy says no escaping?
There was a problem hiding this comment.
Sure. These constants (ATTR_OTEL_SCOPE_NAME, etc.) have dots in them. According to this part of the spec:
Prometheus exporters MUST by default add the scope name as the otel_scope_name label, the scope version as the otel_scope_version label, the scope schema URL as the otel_scope_schema_url label, the scope attributes as labels with otel_scope_ prefix and following the rules described in the Metric Attributes section below, on all metric points, based on the scope the original data point was nested in.
To me, that is a bit ambiguous, but seems like the names should have underscores. It also seems that Golang unconditionally sanitizes these. See this snapshot for example, where dots are preserved everywhere but the scope attributes.
I can add more tests. Are there any in particular that you think are worth porting (since that file is 1200+ lines and this PR doesn't implement all of that functionality)? It's also worth noting that I have a separate branch to add unit suffix support which would bring additional tests. |
|
Yep, 1200 LOC is a lot 😅. Let me try to parse that into human-readable text: Underscore Scaping:
No underscore scaping:
|
| if (hasAttribute) { | ||
| if (hasNonLegacyCharacters(metricName)) { | ||
| if (hasAttribute) { | ||
| metricName = `{${quoteName(metricName)} ${attributesStr}}`; |
There was a problem hiding this comment.
I think the separator inside the braces might be off here. When the metric name is quoted (UTF-8 form), Prometheus expects a comma between the name and the rest of the labels, not a space:
{"test.dotted_total",otel_scope_name="test"} 1
The current output (and what the new tests assert) is:
{"test.dotted_total" otel_scope_name="test"} 1
which I don't think Prometheus/PromQL parsers will accept. Worth double checking against the exposition format spec and updating both stringify() and the new test fixtures if it's indeed wrong.
There was a problem hiding this comment.
Good catch. I'll fix this.
|
@ArthurSens thank you for the feedback. I've added those tests, but I did it in a separate PR (#6727) because even without the translation strategy, I believe there may be a few edge case bugs in the existing implementation. |
Which problem is this PR solving?
The Prometheus exporter does not currently support Translation Strategy.
Related to #6605
Short description of the changes
This PR adds support for Translation Strategy in a way that can also work with Content Negotiation in the future if necessary.
I am opening as a draft because I need to add more tests for the non-default behavior. There is also the issue of adding unit suffixes. I have another branch that implements unit suffixes, but turning them on by default (which is the spec behavior) would be a breaking change, so that will need to be figured out too.
Type of change
How Has This Been Tested?
Checklist: