fix: otel prometheus duplicated attributes #62
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #62 +/- ##
==========================================
- Coverage 78.01% 78.00% -0.02%
==========================================
Files 56 56
Lines 3539 3555 +16
==========================================
+ Hits 2761 2773 +12
- Misses 643 646 +3
- Partials 135 136 +1
☔ View full report in Codecov by Sentry. |
|
I just realized the implementation makes the metric one take precedence. Will fix and increase test coverage. |
|
Fixed, please have a look. |
| // | ||
| // 5. added logic to remove and detect duplicated attributes (instead of concatenating them to stay compatible | ||
| // with gRPC) |
There was a problem hiding this comment.
I would refrain from performing any further customisations to the original exporter. I would prefer extending our existing label deduplication logic in stats package instead, which now detects duplicate labels that the user provides, and include an extra validation for labels managed by our own library.
There was a problem hiding this comment.
Not a bad idea, although I don't think we'll go back to having the original exporter. It would require too many changes to our dashboards, plus the exporter is fairly small/simple, it shouldn't be much trouble to maintain it.
Anyhow, I moved the logic into stats now, the only concern is the duplication of sanitization logic. Have a look let me know what you think 👍
| if _, ok := s.config.excludedTags[k]; ok { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Do we need this?
| if _, ok := s.config.excludedTags[k]; ok { | |
| continue | |
| } |
There was a problem hiding this comment.
I thought I'd leave it for backwards compatibility in case somewhere there is an exclusion rule that would work only before the tag is sanitized. I thought it wouldn't hurt having for now.
| if _, ok := s.config.excludedTags[k]; ok { | ||
| delete(tags, k) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Same as above, better safe than sorry since we might not be the only ones using this lib and I'm not planning to check everywhere if removing it might break an exclusion 🤷♂️
Description
This is to address an error that happens when the same label is set both at the resource level and at the metric label.
The underlying Prometheus client is the one raising the error here.
The proposal is to remove the duplicate at the metric level (so the resource will take precedence) and to log it in order to take action if needed/wanted.
In the gRPC version the resource attributes take precedence and the metric is not lost.
Notion Ticket
< Notion Link >
Security