contrib/go-redis/redis: use UniversalClient about dd-trace-go HOT 27 CLOSED

Any progress?

from dd-trace-go.

Most of the issues above have been addressed, we should be able to address this soon!

from dd-trace-go.

@pierrre I've started looking into this and as far as I've seen, all we need to do is:

func WrapRing(r *redis.Ring, opts ...ClientOption) {
	r.ForEachShard(func(c *redis.Client) error {
		WrapClient(c, opts...)
		return nil
	})
}

func WrapClusterClient(cc *redis.ClusterClient, opts ...ClientOption) {
	cc.ForEachNode(func(c *redis.Client) error {
		WrapClient(c, opts...)
		return nil
	})
}

Wouldn't this solve the problem?

from dd-trace-go.

Gabriel, how would WrapClient help here? ForEachNode doesn't allow to override client instance of the node.

@niamster Perhaps I'm misunderstanding something, but why would it not work? You don't have to override the client instance. WrapClient will just use WrapProcess under the hood (see here). No need to use the returned client. However we'd still likely need to add our own wrapper for using the WithContext set of functions if that's needed.

does your solution work in concurrency?

@k1ng440 I'm not sure I understand the question. If you could please be more explicit.

from dd-trace-go.

I think this sounds like a reasonable suggestion.

• Is this something that you require or would be using yourself or is it more along the lines of "nice to have"?
• Is this something you'd like us to add into dd-trace-go or is this work that you would like to commit to yourself?

from dd-trace-go.

Is this something that you require or would be using yourself or is it more along the lines of "nice to have"?

Yes it is something that I require, because in my code I use Ring and ClusterClient.

Is this something you'd like us to add into dd-trace-go or is this work that you would like to commit to yourself?

I looked at the code and it seems that it requires internal changes in github.com/go-redis/redis.
So I don't plan to do it myself, because I don't know very well how this lib works.

from dd-trace-go.

PS: for now, I will "wrap" the call to Redis, and create the span from my application code.

from dd-trace-go.

I looked at the code and it seems that it requires internal changes in github.com/go-redis/redis.
So I don't plan to do it myself, because I don't know very well how this lib works.

Sure, that make sense. The code is undergoing a lot of changes now anyway, so it might be best that we do it. I don't have a lot of experience with Redis but I do have a rough idea of how this could be achieved. I could invite you to review, if you would like that. In the meanwhile, please hang tight as we'll have to sort this in our list of priorities.

from dd-trace-go.

The following issues need to be addressed before this work will be possible:

• ClusterClient does not have WrapProcess method (redis/go-redis#477)
  This will not allow us to track commands sent to the cluster. (added in v6.8.3)

• ClusterClient and Ring do not support Context (redis/go-redis#737)
  As Context is not currently available, we can not create anything other than root spans on each command. (merged March 8, 2018)

• (ClusterClient).ForEachNode does not cover all nodes (redis/go-redis#701, redis/go-redis#703)
  We are unable to correctly track which node executes a command in the cluster.

It is at the discretion of the project maintainer whether these changes will be added into the library. We could potentially live without being able to track which node (host/port) in the cluster executes each command, but allowing context to exist in a trace is critical.

from dd-trace-go.

I don't know exactly how to implement it.

I'd like to enable tracing on all kinds of clients:

• Client
• Ring
• ClusterClient
• UniversalClient

from dd-trace-go.

The suggestion above should cover all cases. Both Ring and ClusterClient have ForEach* methods which you can use to wrap each client. The current integration already has the WrapClient function.

Wouldn't that solve your problem? If yes, I think we can close this. I am also happy to add the helpers above to the integration for ease of use.

from dd-trace-go.

Just to clarify: you'd have to update the go-redis library to have those ForEach* methods available and working as expected (the PRs linked above that I've submitted upstream were merged). If it's still unclear to you, I can provide the code snippet (it's basically the one above in my previous message).

It could also be that maybe I didn't completely understand what you're after.

from dd-trace-go.

@gbbr Gabriel, how would WrapClient help here? ForEachNode doesn't allow to override client instance of the node.

Initial request from @pierrre is still relevant.

from dd-trace-go.

@gbbr does your solution work in concurrency?

from dd-trace-go.

Ah, I didn't see that WrapClient wraps command processing behind the scene. Makes sense, thanks @gbbr

from dd-trace-go.

@pierrre's question is absolutely still relevant. It's just that we haven't heard back from him to work together on this further :)

from dd-trace-go.

Hello!
Sorry for the long silence.

The truth is that I've not yet used the tracing for Redis, and it's not a priority for me (my company) anymore. (I don't have any issue with Redis right now)

I still don't understand what I'm supposed to do with all the ForEach* methods and the WrapClient function.
Can you give me an example ?

from dd-trace-go.

@pierrre it's fine. There is no pressure. This isn't something that we need per se and if it's not prioritary for you either than we should suspend it for now.. I've already given an example here.

from dd-trace-go.

Does it mean that after calling the code from #160 (comment) the *redis.Ring or *redis.ClusterClient wil be "ready" for tracing ?
I'm not sure to understand, because WrapClient is returning a new client.
Does it update the internal behavior of the wrapped client, even if we don't use the returned client ?

from dd-trace-go.

Does it update the internal behavior of the wrapped client, even if we don't use the returned client ?

Correct.

The only downsides to not having the returned client are:

• You won't be able to return a pipeline from it (not even sure if that's possible or needed).
• You won't be able to make calls using a context.

from dd-trace-go.

You won't be able to return a pipeline from it

In several application, I'm using TxPipeline().
Is there the same limitation ?

You won't be able to make calls using a context

Then, how can I transmit my "parent spans" ?
How will appear my Redis spans ? Without parent ?

from dd-trace-go.

You're right on all points. We'll have to add that logic from my example into the library in that case, covering the methods to use context and pipeline, just like we do now for regular clients. Thanks for explaining your usecase.

from dd-trace-go.

I'd like to dig this up again. I'm currently experiencing an issue similar to above where I don't seem to be able to create a dd-trace-go wrapped client when using either UniversalClient or ClusterClient. Is this just a feature that is not widely used? I'm still kind of new to go, but it seems like there should be some way to use clients of these other types. Anything to keep in mind if I create a pull request? I'm hesitant since I'm still new to go, but I do really need this functionality.

from dd-trace-go.

I don't seem to be able to create a dd-trace-go wrapped client when using either UniversalClient or ClusterClient. Is this just a feature that is not widely used?

It may be, @pierrre seems to have needed it. We are also using this integration in production, but not those specific structures. I tried implementing it once but got into some complications and gave up. However, that was a long time ago and things might've changed with the library since then.

I'm still kind of new to go, but it seems like there should be some way to use clients of these other types. Anything to keep in mind if I create a pull request?

I think there should be a way. If you want to make sure you're going down the right path, I'd recommend talking to us here about how you plan to implement it and we can discuss together to figure out what the best approach is, before writing any actual code. That way we can make sure we're exploring the right path.

I'm hesitant since I'm still new to go, but I do really need this functionality.

Need not worry about that 🙂If you are patient, we can go through a couple of review rounds once (and if) you make a PR and make sure it reaches a state that matches the repository and Go idioms. I'm happy to help with that.

from dd-trace-go.

FYI it's not an issue for me anymore.
I've implemented my own tracing for Redis, using opentracing.
(I'm still using Datadog as "tracer")

The version 8 of the Redis library helped me a lot, because it's possible to have "hooks" for each commands,
and all the functions now accept a context argument.

from dd-trace-go.

new WrapClient method was added in v1.29.x (#808) and I believe this issue can be closed.

from dd-trace-go.

@HurSungYun That looks correct. Closing this.

from dd-trace-go.