search

open-telemetry / opentelemetry-java-instrumentation

OpenTelemetry auto-instrumentation and instrumentation libraries for Java
https://opentelemetry.io
Apache License 2.0
1.81k stars 795 forks source link

Support Service Peer Name Mappings with Lettuce #6813

Closed floric closed 11 months ago

floric commented 1 year ago

Is your feature request related to a problem? Please describe. Using otel.instrumentation.common.peer-service-mapping it is possible to get more developer friendly descriptions of contacted systems via HTTP. So for example Grafana with Tempo instead of only "HTTP GET" the name of the mapped system will be shown. Unfortunately this doesn't work for Redis with Lettuce. It might be caused by the different attribute named net.sock.peer.name instead of net.peer.name for REST.

EDIT: in the Semantic Conventions for Database Client Calls they also specifiy net.peer.name as the correct field. One recent MR for 1.17.0 changed this attribute (https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/6268). I'm not sure which attribute is the correct one.

Describe the solution you'd like Would it be possible to support these mappings for Lettuce (any maybe all clients providing net.sock.peer.name )? I'm not sure if this mapping happens in the client implementation or on a higher level after the attributes have been generated.

Describe alternatives you've considered Currently, it's okay to have only the domain names. But this feature would be nice.

Additional context If you can show me the place in code where the mapping happens and aggree with this request, I could create a PR and do the work. :)

mateuszrzeszutek commented 1 year ago

Hey @floric , What lettuce version are you using?

floric commented 1 year ago

Oh, sorry, I missed this fact: io.opentelemetry.lettuce-5.1 based on the Java Agent in version 1.18.0. The service itself uses Lettuce 6.1.9.

mateuszrzeszutek commented 1 year ago

Okay, I see. I think there are 2 solutions that we could implement here: 1) Make PeerServiceAttributesExtractor use net.sock.peer.name if net.peer.name is not available. @trask do you remember why we're only using net.peer.name to look up the peer service? 2) Pass the RedisURI to the Tracing implementation - this is not supported by the Lettuce API out of the box, we would probably have to instrument the RedisClient constructor and somehow pass the URI to ClientResources#tracing()

floric commented 1 year ago

As far as I understand solution 1 would be more generic and suitable for other spans with the same attributes. Right? Also the change (addition) appears to be much simpler than solution 2.

trask commented 1 year ago
  1. Make PeerServiceAttributesExtractor use net.sock.peer.name if net.peer.name is not available. @trask do you remember why we're only using net.peer.name to look up the peer service?

We may have just missed this when introducing net.sock.peer.name. It makes sense to me to add.

trask commented 11 months ago

revisiting this, I don't think net.sock.peer.name is supposed to be set if net.peer.name is not available. my understanding is that in this case the value should be set into net.sock.peer.name and net.sock.peer.name would be left blank

we've done a lot of work in this area around network conventions recently, @floric can you see if this issue still exists in the latest Java agent release?

floric commented 11 months ago

revisiting this, I don't think net.sock.peer.name is supposed to be set if net.peer.name is not available. my understanding is that in this case the value should be set into net.sock.peer.name and net.sock.peer.name would be left blank

we've done a lot of work in this area around network conventions recently, @floric can you see if this issue still exists in the latest Java agent release?

Hey :) thanks for the feedback. I'll fetch the latest version and see if it is solved and then come back.

floric commented 11 months ago

@trask the properties are still identical with version 1.29.0. But the issue itself is not that important. We can live with the existing property names. If you agree, we can close this issue.

trask commented 11 months ago

sounds good, the net peer semantic conventions are completely changing in the upcoming Java agent 2.0 to match the upcoming related stable semantic conventions, so maybe give this another try in 2.0 and send us an issue if you think there's something that can be improved