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

Provide Tracing extension point to handle RedisURI #1312

Closed
anuraaga opened this issue Jun 17, 2020 · 3 comments
Closed

Provide Tracing extension point to handle RedisURI #1312

anuraaga opened this issue Jun 17, 2020 · 3 comments
Labels
status: waiting-for-feedback We need additional information before we can continue

Comments

@anuraaga
Copy link
Contributor

Feature Request

Currently, SocketAddress is provided to Tracing to allow a span to have endpoint information. There is some information that's only available in the RedisURI though, the database name and user when enabling ACLs. It would be nice to provide RedisURI to the extension point.

Is your feature request related to a problem? Please describe

I am implementing an OpenTelemetry implementation of tracing, and there are some attributes we would like to add to match semantic conventions of a database but without the RedisURI, we can't. They aren't high priority attributes but it'd be nice.

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/database.md

Describe the solution you'd like

Passing both SocketAddress and RedisURI to createEndpoint.

Describe alternatives you've considered

Pass both Endpoint and RedisURI to remoteEndpoint. Both seem reasonable.

Teachability, Documentation, Adoption, Migration Strategy

Users implementing Tracing will be able to find the extension point.

@mp911de
Copy link
Collaborator

mp911de commented Jun 17, 2020

In CommandHandler, we do not have access to RedisURI. RedisURI is the intent to establish a connection. The database name and other properties (such as the actual hostname) may change during connect/while working with a connection.

In Redis Cluster/Sentinel arrangements, we only have a single RediURI while the cluster might consist of multiple nodes.

I'd suggest to subclass InetSocketAddress for your setup and configure a custom SocketAddressResolver. Your customized InetSocketAddress would carry the details you require for tracing. You can apply environmental detail through the Tracing SPI (Span.remoteEndpoint).

Let me know if you require any further details.

@mp911de mp911de added the status: waiting-for-feedback We need additional information before we can continue label Jun 17, 2020
@mp911de
Copy link
Collaborator

mp911de commented Jul 1, 2020

Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open the issue.

@mp911de mp911de closed this as completed Jul 1, 2020
@mp911de mp911de removed the type: feature A new feature label Jul 1, 2020
@anuraaga
Copy link
Contributor Author

anuraaga commented Jul 1, 2020

Apologies for the lack of feedback - the approach you suggested looks like it'll work though so will try it out, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-feedback We need additional information before we can continue
Projects
None yet
Development

No branches or pull requests

2 participants