gomodule / redigo

Go client for Redis
Apache License 2.0
9.76k stars 1.25k forks source link

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

Closed rarguelloF closed 10 months ago

rarguelloF commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

stevenh commented 1 year ago

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 commented 1 year ago

What every time support opentelemetry?

stevenh commented 1 year ago

What every time support opentelemetry?

Not sure I understand the question, could you clarify?

stevenh commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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