-
Notifications
You must be signed in to change notification settings - Fork 783
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
Exporting tags consistently #3281
Exporting tags consistently #3281
Conversation
+1 like the idea! |
Codecov Report
@@ Coverage Diff @@
## main #3281 +/- ##
=======================================
Coverage 85.38% 85.39%
=======================================
Files 265 269 +4
Lines 9541 9552 +11
=======================================
+ Hits 8147 8157 +10
- Misses 1394 1395 +1
|
src/OpenTelemetry.Exporter.Jaeger/Implementation/JaegerActivityExtensions.cs
Outdated
Show resolved
Hide resolved
@@ -19,6 +19,7 @@ | |||
<ItemGroup> | |||
<PackageReference Include="System.Collections.Immutable" Version="$(SystemCollectionsImmutablePkgVer)" /> | |||
<PackageReference Include="System.Reflection.Emit.Lightweight" Version="$(SystemReflectionEmitLightweightPkgVer)" /> | |||
<PackageReference Include="System.Text.Json" Version="$(SystemTextJsonPkgVer)" /> |
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.
TODO: Gotta figure out a better place to put the shared TagTransformer.cs
file.
I think this causes an unnecessary new dependency on the SDK project. Technically this is only required for the shared TagTransformer
class which is only used by exporters.
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.
Could have a dummy project just to house shared stuff like https://github.com/open-telemetry/opentelemetry-dotnet-contrib/tree/main/src/OpenTelemetry.Contrib.Shared 🤷
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.
I ended up just excluding TagTransformer.cs
from compilation. See: 673ade0
I tried the separate project idea and ended up needing to pull in the OpenTelemetrySdkEventSource
for logging. Also, a new project may have resulted in a gratuitous nuget package. Both of these concerns could be overcome, so if folks think a separate project is better, I'm happy to go that route.
Nevermind, that caused other problems. I solved this more simply by just moving the code that required System.Text.Json to the projects that actually needed it. 74df0df
src/OpenTelemetry.Exporter.Jaeger/Implementation/JaegerTagTransformer.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.Jaeger/Implementation/JaegerTagTransformer.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/LogRecordExtensions.cs
Outdated
Show resolved
Hide resolved
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.
Some nits and suggestions but LGTM.
#3262 standardized how we handle tags in the OTLP exporter for traces, metrics, and logs.
I have generalized that work in a way to get code reuse across for the Zipkin and Jaeger exporters. I suspect this could be used for Prometheus too, but haven't touched Prometheus in this PR.
This PR will also address a number of bugs:
process.command_args
serializes asSystem.String[]
#3229