Retrieve request context data through metrics callback

[aadedamola]

Problem

Contexts can be used to manage information about requests. Some of these request's information may require logging. It will be useful to get these additional information via the original context in the metrics callbacks.

Or is there any other way to go about this?

[aadedamola]

The context is being passed in here https://github.com/cep21/circuit/blob/b244db881625d4706bda7961cf2b5f3c8423985f/v3/circuit.go#L259

The idea is to propagate/extend it to the callbacks

[cep21]

I agree this is a reasonable idea. Do you have ideas how to do this without breaking backwards compatibility?

[aadedamola]

How about introducing additional callbacks that contain the context as one of each callback's parameters?

type RunMetrics interface { Success(context Context, now time.Time, duration time.Duration)

Perhaps, also adding a boolean flag in the ExecutionConfig or relevant Config returnContextToCallbacks to manage which callback is called. https://github.com/cep21/circuit/blob/b244db881625d4706bda7961cf2b5f3c8423985f/v3/config.go#L43

When the manager is initialised with configuration values, we can specify which callbacks we want to use by nature of the flag

[cep21]

RunMetrics is a public interface. If we add methods to it, it will cause people's code to not compile anymore if they have custom structs that implement it.

[aadedamola]

I took some time to go through the code. From my understanding, this can only be done in two ways:

• Creating a new version that is not backward compatible to allow for the context propagation.
• Creating new metrics and metric collectors that support context propagation. This procedure seems like massive duplication of code just to have context passed as a parameter.

What do you think? Is there any reason why a v4 cannot support context? I think it is an avenue to pass more information - which will come in useful

[aadedamola]

Also, do you think the other route is acceptable? Will that be accepted as a valid contribution?

[cep21]

Sorry for the delay. I don't think a new major version is workable. I think it's best to create a new interface type RunMetricsCtx interface, have all the built in stuff implement both, and have calls like c.CmdMetricCollector.ErrShortCircuit(startTime) try to cast to a RunMetricsCtx, and if it can, use that instead.

There are still some details to iron out, but I think it can work. My only concern would be to not hurt the efficiency of the existing code.

[aadedamola]

Ah I see, seems like a massive duplication but I think it will work.

[cep21]

I think we will probably do this as the v4 library. Discussing in https://github.com/cep21/circuit/issues/115

Will close as stale since it's been a while and probably won't make it into the current v3