feat(redis): add opentelemetry instrumentation #131
Conversation
Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>
Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>
|
@philippgille please review |
There was a problem hiding this comment.
Thanks for the PR!
It's nice that the instrumentation can be added with so little code, but it's still adding several transitive dependencies even when not enabling the OTel options. Maybe some users use DataDog and prefer their telemetry library.
And it also leads to the question about instrumenting all the other storage clients.
I think we can be more flexible by just allowing users to pass their own configured/instrumented Redis client.
So for example currently in gokv/redis we have NewClient(...) and it creates and sets the Redis client here.
We could have a second constructor function that passes an existing Redis client. I think there's another ticket already for having this constructor for all gokv packages to allow maximum flexibility, but I couldn't find it just now. Maybe I kept that in a separate TODO list.
One question that brings up is how to configure the address, password and DB then.
- Either require the user that passes the client to already configure that? It would decrease the convenience that
gokvbrings. - Or overwrite those options, and document this to the user so they know they don't need to set it.
We could take some inspiration from other packages for this, for example the HTTP library Resty has New (here) and NewWithClient (here) where it allows to pass an already configured http.Client.
I think that's a good design as it makes things simple by default, and flexible when required, without having to introduce telemetry and specific dependencies in gokv itself.
| "github.com/redis/go-redis/extra/redisotel/v9" | ||
| "github.com/redis/go-redis/v9" |
There was a problem hiding this comment.
With these import lines the go.mod is out of sync and when you run go get ./... or go mod tidy this will add the v9 to the go.mod file and effectively include the dependency update in this PR.
Your plan was probably to have the other separate update PR merged first and then this one, but this PR's second commit contains a removal of the dependency lines from the go.mod.
I would suggest to treat this PR as a "stacked PR" building entirely on top of the other one, without removing anything from the other one. For any new commit you might add onto the other, you can rebase this branch on the other to include all the same commits. Then when the other PR is merged, this PR's diff should be automatically changed to be only the relevant ones (either out of the box, or after squashing the commits that are originally from the other branch).
I like this approach. Another approach is to create and Client but allow to access it and then add instrumentation outside of gokv |
Great idea as well! I think both are equally useful to even coexist, so we don't have to decide for one over the other. People with a more complex Redis setup can provide their own client, and people who prefer simplicity and only require the basics can use the gokv constructor to create the client, and then still make minor changes to it. Is it okay that I close this PR? |
Resolves #129
updated redis to v9 in separate PR #130