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
Emit attributes slices as their json representation #5159
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5159 +/- ##
=======================================
- Coverage 84.5% 84.4% -0.1%
=======================================
Files 265 265
Lines 17505 17514 +9
=======================================
+ Hits 14792 14797 +5
- Misses 2404 2405 +1
- Partials 309 312 +3
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we consider this breaking? An alternative would be to have a new EmitJSON (or other) function.
This may be considered a breaking change, but I don't know if a new It seems to me the breaking change here is in the format attributes are exporter in. I'm not sure we have a trivial solution though. |
It looks like it is only used by zipkin and prometheus. Zipkin wouldn't be affected by the change regardless, since it already uses json for complex types: opentelemetry-go/exporters/zipkin/model.go Line 172 in 82b49b4
Prometheus isn't stable, so it can make the change. |
338edfc
to
f5da204
Compare
I'm still concerned that this will be breaking for users. How do people feel about adding add a new EmitJSON function with this behavior, and leaving the existing Emit function as-is? |
How does the exporter decide which method it should call? |
We should recommend that exporters call EmitJSON. We could even consider deprecating (but not removing) Emit(). |
c045206
to
d3a3166
Compare
While I understand the breaking change concern, I don't think Except if we emit a string as json, well get |
Thanks. I do think this change is worth making, especially since it brings us into compliance with a part of a stable specification. |
28b5404
to
565c87f
Compare
Related:
We currently display slices as their Go representation, which is not reliable cross languages, and doesn't match the specification (which asks for JSON).
Benchmark