Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[prometheusremotewriteexporter] Support sending trace id and span id for exemplars #8380

Merged
merged 2 commits into from
Mar 15, 2022

Conversation

dashpole
Copy link
Contributor

Description:
Fixes #8351. This is aligned with the specification for OTLP -> prometheus.

The specification does not say how to handle collisions between attributes named trace_id and span_id and the trace id or span id of the exemplar itself. This PR has attributes take precedence over trace id and span id from the exemplar itself.

Testing:

Unit test checks that trace id and span id are added as labels on prometheus exemplars.

@dashpole dashpole requested review from a team and mx-psi March 10, 2022 21:04
@dashpole dashpole added the comp:prometheus Prometheus related issues label Mar 10, 2022
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one nit to make this more consistent with the rest of the codebase

pkg/translator/prometheusremotewrite/testutils_test.go Outdated Show resolved Hide resolved
pkg/translator/prometheusremotewrite/testutils_test.go Outdated Show resolved Hide resolved
@dashpole
Copy link
Contributor Author

cc @anneelisabethlelievre, who initially implemented exemplars for PRW
cc @Aneurysm9

@mx-psi mx-psi added the ready to merge Code review completed; ready to merge by maintainers label Mar 15, 2022
@jpkrohling
Copy link
Member

jpkrohling commented Mar 15, 2022

@dashpole, could you please address the conflict? Given the approvals, I'll merge this PR afterward.

@dashpole
Copy link
Contributor Author

It is non-trivial to rebase (since we now need to handle max exemplar size including these new values), so i'd like another review from someone before we merge.

@dashpole
Copy link
Contributor Author

The additional change is that we have to keep track of the number of runes for the new trace id and span id to make sure the total doesn't exceed limits. If it does exceed the limit, we need to keep trace and span id, and ignore filtered attributes. The relevant line from the spec is: filtered_attributes MUST be added as labels on the OpenMetrics exemplar unless they would exceed the OpenMetrics limit on characters. I've added a unit test which tests that case

@dashpole
Copy link
Contributor Author

Good to go now!

@jpkrohling jpkrohling merged commit fa86bb0 into open-telemetry:main Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:prometheus Prometheus related issues ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[prometheusremotewriteexporter] TraceID and SpanID must be added as exemplar attributes.
6 participants