Redis: instrumentation hook for newrelic

[aep]

A generic hook to time queries would be great, so we can feed them to newrelic with https://github.com/yvasiyarov/gorelic
Is there any consent on how to do that?
something like (bad pseudcode ahead)

````go
type Instrumentation interface {
start(*Command) interface{}
end(interface{}, *Command)
}

func (a *gorelic.Agent, cmd *Command ) start() interface{} {
return a.Tracer.BeginTrace(cmd.String())
}

NewClient(&Options{Instrumentation: theGoRelicAgent....
````

[vmihailenco]

See https://github.com/go-redis/redis/pull/352

[aep]

@vmihailenco this is great! any chance to pass some context through to this? in my case the http request to attach the timing to.

[vmihailenco]

I would like to support contexts, but that means requiring Go 1.7 which looks like too big limitation at the moment. Do you have any ideas? The API I have in mind is very simple:

client.WithContext(context) - sets context
client.Context() - returns current context

[aep]

yeah, withcontext makes alot of sense.

func GetSomethingFromRedis(w http.ResponseWriter, r *http.Request) { ... redis.WithContext(w).HGetAll(name); }

gorelic and gorilla/handlers use a wrapper around ResponseWriter instead of Context, (maybe because Context is new?), so maybe this works for pre 1.7

func GetSomethingFromRedis(w http.ResponseWriter, r *http.Request) { ... redis.WithResponseWriter(w).HGetAll(name); }

Third option (and imo best), just make WithContext take interface{} and let the user later do type assertion inside WrapProcess.

````
func GetSomethingFromRedis(w http.ResponseWriter, r *http.Request) {
redis.WithTrackingContext(anything).HGetAll(name);
}

client.WrapProcess(func(oldProcess func(cmd redis.Cmder) error) func(cmd redis.Cmder) error {
return func(cmd redis.Cmder) error {
anything := cmd.TrackingContext().(*Anything)
````

[vmihailenco]

Context was added in https://github.com/go-redis/redis/pull/474 for 1.7+

[vctandrade]

@aep did you manage to get the context from within the WrapProcess method? I could not get it to work because WithContext makes a copy of the client, and the wrapper function does not have access to the Client object that called it.

[Gyllsdorff]

@vmihailenco
I have the same problem as vctandrade. The function in WrapProcess have no way to get access to the client so there is no way for it to know it's context.

Also, I think the WrapProcess on clients that have been copyied broke again in commit https://github.com/go-redis/redis/commit/d6cb688ea756d865f588932a322fc92e9368e338

[vmihailenco]

Did you consider saving context in function closure when wrapping process?

I also wonder what is your use case. What info do you save/use in context.Context? TBH I am not using context in my code so it is hard to predict how people are actually using it...

[Gyllsdorff]

Yes. But the WrapProcess call is for a entire client and so far I have only found one way to make a copy of the client which is by using the client.WithContext call. 😄

When I saw the WithContext method I thought things like this was the intended usage?

Right now I solved it like you suggested.

func GatherMetrics(logger *logger.Logger) func(old func(cmd goRedis.Cmder) error) func(cmd goRedis.Cmder) error {
    return func(old func(cmd goRedis.Cmder) error) func(cmd goRedis.Cmder) error {
        return func(cmd goRedis.Cmder) error {
            start := time.Now()
            err := old(cmd)
            duration := time.Since(start)
            logger.Info(fmt.Sprintf("Processed '%s', runtime: %s", cmd, duration.String()))

            return err
        }
    }
}

func LogMetrics(ctx context.Context, client *Client) *Client {
    clientType := client.Context().Value(clientTypeCtxKey).(string)

    logger := logger.
        GetLoggerFromContextOrNewLogger(ctx).
        With(
            zap.String("tag", "redis"),
            zap.String("redisType", clientType),
        )

    clonedClient := client.WithContext(client.Context())
    clonedClient.WrapProcess(GatherMetrics(logger))
    return clonedClient
}

LogMetrics(request.Context(), redisClient).HMGet("Stuff", "id")

I suspect I will get a lot of question about the client.WithContext(client.Context()) line in the code review. 😛

If the baseClient.clone method was made public I could use that instead, that would make it much more clear what I am trying to do. Right now I am relying on the side-effects of setting the context which is not really a great idea.

[Gyllsdorff]

I also wonder what is your use case. What info do you save/use in context.Context? TBH I am not using context in my code so it is hard to predict how people are actually using it...

For example the request ID so we can trace all DB/Redis/ReST call as they flow through the api:s, that and timeout values.

A incoming HTTP request is only allowed to take a certain amount of time so we can't use the timeout on the client connection since a request can generate several redis queries and the timeout is on the sum of the redis query + db query + http sub requests and not on how long a single redis query takes.

[nziebart]

The instrumentation wrapper needs access to the Context in a first class way. I think we need to introduce new versions of WrapProcess and WrapProcessPipeline that pass the Context object to the wrapper function. e.g.,

func (c *baseClient) WrapProcessContext(
    fn func(oldProcess func(cmd Cmder, ctx context.Context) error) func(cmd Cmder, ctx context.Contxt) error)

Currently the only alternative is to first call WithContext which creates a copy, then call WrapProcess and grab a reference to the client's context in the function closure. This is a brittle pattern and probably necessitates some utility function to perform these two steps together.

It's also unclear to me why creating a copy of the client needs to get rid of the associated process wrapper function. Maybe WrapProcess should be WithWrappedProcess and return a copy? The ideal way to use this would be:
1) attach instrumentation wrappers at client creation time
2) call WithContext in each request to scope the client to the request (but still maintain instrumentation)

[threez]

Hi, did run into the same issue for tracing and logging. I did a workaround as proposed above. It would be much easier if the Cmder type has a GetContext() method that returns the context that was previously assigned with WithContext(context.Context).

[dcormier]

The instrumentation wrapper needs access to the Context in a first class way. I think we need to introduce new versions of WrapProcess and WrapProcessPipeline that pass the Context object to the wrapper function.

An alternative might be to add the context to Cmder.

[taiyangc]

It would make much more sense if Cmder offers a context in the same way go-pg's orm.Query offers it. In that way we can share the same client instead of cloning and hogging unnecessary client connections just because we need to do a WrapProcess.

For now we can only resort to doing manual segment tracing before/after each call so we can keep re-using the same client. The LogMetrics approach suggested above will eventually exhaust system resources with too many concurrently cloned clients.

[aspacca]

I'm contributing to apm-agent-go with an auto instrumentation module for go-redis and the solution was to wrap the client in an instrumentation interface that complies with the UniversalClient interface and passing the context in the process wrapper (https://github.com/elastic/apm-agent-go/pull/505)

Since the main concern is to keep re-using the same, client wrapping and adding the process wrapper should be done only once

So far I used Jaeger and Elastic APM as tracing tools: they both comply with the OpenTracing project and it's just a matter of changing the wrapping of the go-redis client accordingly to switch between the two in a distributed tracing environment

[vmihailenco]

FYI I have plans to enable support for Go modules and in new major version add context.Context as first argument in all Redis commands, e.g.

Ping(context.Context) *StatusCmd
Exists(context.Context, keys ...string) *IntCmd

So all commands will require context and nil context should work fine. Existing users will update to the new major version as they have time.

The only alternative

PingContext(context.Context) *StatusCmd
ExistsContext(context.Context, keys ...string) *IntCmd

seems to be too heavy weight because it requires to have a context-aware copy of every Redis command...

Thoughts?

[aspacca]

Thoughts?

I would rather add a method to inject a context.Context in the next command call (without returning a new client) and leave the commands signature the same: it would require far less refactoring to upgrade to the new major version and have the same result

also if you don't really care for a context it would be better than forcing to pass nil as first param of every command

regardless of the solution, if commands will be context aware I think the process wrappers should be passed the context to: this will permit both lighter context aware instrumentation and the possibility to react to context.WithTimeout etc

[pranas]

@vmihailenco I think your approach is right 👍 Passing context as first argument is now a standard practice in Golang community. I don't think the migration/refactoring for the users would be hard, it could probably be done with smart mass-replacing on the code base. Looking forward to this.

[vmihailenco]

@aspacca we already something like that with db.WithContext(ctx).Ping(). The API is a bit strange and context is not really used anywhere...

[aspacca]

@vmihailenco I know, that's what I use for the apm-agent-go module for your library. but .WithContext(ctx) return a copied and resetted version of the client: I expect that when you'll support context thoroughly every command you won't copy the client each time :)

that's why I said what I care more about, regardless the solution chosen, is to have the wrapping processes aware of the context

[aspacca]

btw, I've just tested Watch instrumentation and I see it's not possible, since it creates a new Tx that I cannot inject the process wrapper in

[aspacca]

@vmihailenco is there any specific reason for this: https://github.com/go-redis/redis/blob/master/tx.go#L27?

[vmihailenco]

@aspacca yes, statefulCmdable accepts processor function that must do something with the commands.

[aspacca]

@vmihailenco , sorry, probably wrong line linked: I mean why don't you assign to the tx client the process/processpipeline of the original client?