search

open-telemetry / opentelemetry-java

OpenTelemetry Java SDK
https://opentelemetry.io
Apache License 2.0
2.01k stars 831 forks source link

Feature Request: Better support for injecting into immutable carriers #4208

Open youngm opened 2 years ago

youngm commented 2 years ago

Is your feature request related to a problem? Please describe. TextMapSetter API doesn't work well with immutable Carriers. For example, SQS SendMessageRequest in the AWS Java SDK v2.

Describe the solution you'd like Perhaps the inject() API and TextMapSetter could support returning the newly injected carrier?

Describe alternatives you've considered Currently, my TextMapSetter for Immutable carriers put the new carrier in some kind of variable that I then aquire and actually send. This is sub-optimal because it makes my TextMapSetter, not thread-safe requiring a new instance for each injection. This isn't too bad but seems to go against the idea of the injection api.

jkwatson commented 2 years ago

I think this is a good idea..now we just have to figure out how to make it work while maintaining binary backward compatibility.

anuraaga commented 2 years ago

@youngm SendMessageRequest has a Builder class can you use it to inject?

Returning the type probably would be good but indeed will be somewhat tricky to update instrumentation for. In practice I've seen most immutable objects have a builder type so wondering if it's something we can live with still.

youngm commented 2 years ago

@anuraaga SendMessageRequest does have a builder but it builds a new immutable instance, it doesn't modify the original instance. Creating the new Carrier isn't the issue here. I think the main issue here is the API to get that new carrier to the code sending the carrier.

trask commented 2 years ago

just as a reference, here are two places in the java-instrumentation repo where we hack around immutable carriers:

https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/akka/akka-http-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/akkahttp/client/HttpHeaderSetter.java

https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/java-http-client/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/httpclient/HttpHeadersSetter.java

jkwatson commented 2 years ago

@anuraaga SendMessageRequest does have a builder but it builds a new immutable instance, it doesn't modify the original instance. Creating the new Carrier isn't the issue here. I think the main issue here is the API to get that new carrier to the code sending the carrier.

I think @anuraaga 's idea was to use the builder as the carrier, rather than the request itself.

youngm commented 2 years ago

@anuraaga yeah, that's a good idea and better than the workaround I've been using in many cases. Thanks.