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

contrib/go-redis/redis.v8: support wrapping a go-redis v8 client #808

Merged
merged 3 commits into from
Jan 13, 2021

Conversation

seancaffery
Copy link
Contributor

Fixes #803

Introduce a new method, WrapClient, in the go-redis.v8 integration. This is similar to the older go-redis integration in allowing a user to supply an already configured Redis client and have the tracing added to that rather than having the library build and return a client.

redis.NewUniversalClient can return a Client, FailoverClient, or ClusterClient instance. The important difference for this integration is the way that Redis addresses are available from a build client. A Client instance has a Addr field set to what's passed in to the constructor (e.g. 127.0.0.1:6379), FailoverClient sets Addr to FailoverClient and ClusterCleint doesn't make this available at all. This is important because Addr is currently what's being used to set host and port tags on traces.

  • What should we do for host and port tags for the client types that don't have a useful value?
  • ClusterClient has a list of addresses available. Should we / can we add all of them as tags?
  • Is there any other tag differences that we want to expose between the clients?

I've left the host and port tags as they were until we decide what the approach for each should be. These are also currently excluded from the test.

@knusbaum
Copy link
Contributor

knusbaum commented Jan 7, 2021

What should we do for host and port tags for the client types that don't have a useful value?

In general, we can only add the info we can get from the client. If host/port aren't available (or don't have a useful value) then we should skip adding them.

ClusterClient has a list of addresses available. Should we / can we add all of them as tags?

Yes, Let's add the list of them as a tag, unless you can think of a more useful way to tag them.

Is there any other tag differences that we want to expose between the clients?

I am ok exposing any differences that are present. We don't need to (or even want to) make it appear that all clients are the same. We should expose as much useful information as we can get from any of the clients. We can even expose the type of client as a tag, if you think that will be useful.

@seancaffery
Copy link
Contributor Author

I've made the tags specific to each client. I ended up sticking with what was there originally - host, port, db, and addrs for the ClusterClient. I had a look through the exposed options and didn't see anything else that I'd find particularly useful to have on traces. Happy to add more tags if others would like something in there.

Copy link
Contributor

@knusbaum knusbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good. I have some style nits and one request about testing.

contrib/go-redis/redis.v8/redis_test.go Outdated Show resolved Hide resolved
contrib/go-redis/redis.v8/redis_test.go Outdated Show resolved Hide resolved
contrib/go-redis/redis.v8/redis.go Outdated Show resolved Hide resolved
contrib/go-redis/redis.v8/redis.go Outdated Show resolved Hide resolved
contrib/go-redis/redis.v8/redis_test.go Outdated Show resolved Hide resolved
…lients

Adds a WrapClient method which takes an already consructed
redis.UniversalClient and adds a tracing hook
Consolidate the addition of a hook to a Redis client via WrapClient
Copy link
Contributor

@knusbaum knusbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@knusbaum knusbaum merged commit 9892197 into DataDog:v1 Jan 13, 2021
@seancaffery seancaffery deleted the sean/go-redis-wrap-client branch January 13, 2021 23:16
dianashevchenko pushed a commit that referenced this pull request Feb 8, 2021
Introduce a new function, WrapClient, in the go-redis.v8 integration. This
is similar to the older go-redis integration in allowing a user to supply
an already configured Redis client and have the tracing added to that
rather than having the library build and return a client.

Fixes #803
This was referenced Mar 11, 2021
This was referenced Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

contrib/go-redis/redis.v8: support wrapping a go-redis v8 client
2 participants