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

Expose DialOptions used to create the connection to support instrumentation/tracing features #642

Closed
rarguelloF opened this issue Mar 7, 2023 · 10 comments

Comments

@rarguelloF
Copy link

Hi 👋

I was wondering if it would be possible to expose the dialOptions that were used during any of the Dial* functions to support instrumentation/tracing libraries like OpenTelemetry.

As an example, they state Redis instrumentations should at least provide the db.redis.database_index field (ref), which is currently inaccessible since this struct and fields are unexported.

If this sounds good to you, I'm happy to open a PR implementing this change.

Thanks!

@stevenh
Copy link
Collaborator

stevenh commented Mar 8, 2023

I don't think exposing dialOptions will solve the problem as a SELECT can be run at any time which could make the information stored in dialOptions.db incorrect to use for tracing.

@rarguelloF
Copy link
Author

rarguelloF commented Mar 8, 2023

@stevenh thanks for your response 😄
yeah you are right, if somehow the currently used db (not just the one used when opening the connection) would be accessible in the Conn, that would be perfect.

Actually, the Redis select command doc says:

Since the currently selected database is a property of the connection, clients should track the currently selected database and re-select it on reconnection.

Do you know if this library supports that? If it does, then it should be trivial to expose the selected db.

@stevenh
Copy link
Collaborator

stevenh commented Mar 8, 2023

Nope this doesn't currently track the connection DB. I'd even go so far as to say calling SELECT manually will result in unpredictable results due to the pooled nature of the connections, so while it's not prevented doing so would be dangerous.

@rarguelloF
Copy link
Author

@stevenh in that case what would be the best approach to support this kind of instrumentation in this library?

@stevenh
Copy link
Collaborator

stevenh commented Aug 20, 2023

I would write a wrapper to achieve that, I wouldn't add it directly as many people don't need to want it as it would be performance hit, all be it small.

@andotorg
Copy link

What every time support opentelemetry?

@stevenh
Copy link
Collaborator

stevenh commented Oct 28, 2023

What every time support opentelemetry?

Not sure I understand the question, could you clarify?

@stevenh
Copy link
Collaborator

stevenh commented Jan 28, 2024

This is related to a similar conversation about adding slog support, which has me thinking about a v2 to support easier extension which should consider this use case as well.

Would like to peoples thoughts on that as an approach?

@stevenh
Copy link
Collaborator

stevenh commented Jan 28, 2024

Thinking more about this, if we could enable connection wrapping using dial options that would allow users to enable tracing and logging with zero overhead for others, something to consider.

@stevenh
Copy link
Collaborator

stevenh commented Jan 28, 2024

Going to close this as a duplicate of #568 we can continue this conversation there.

@stevenh stevenh closed this as completed Jan 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants