mailgun / groupcache

Clone of golang/groupcache with TTL and Item Removal support
Apache License 2.0
484 stars 72 forks source link

GH-35: Initial proof of http request update feature #37

Closed mgale closed 2 years ago

mgale commented 2 years ago

Initial idea on how this feature could be implemented.

thrawn01 commented 2 years ago

Instead of overwriting the UpdateRequest what do you think of just adding the tracing info to the context.Context which is passed to Getter.Get(ctx context.Context, key string, dest Sink) error then makeRequest() will check for the tracing info in the context, if it exists it's injected into the header. Thoughts?

Baliedge commented 2 years ago

@thrawn01 With OpenTracing, your tracing lifecycle looks like:

  1. Start a span.
import "github.com/opentracing/opentracing-go"

tracer := opentracing.GlobalTracer()
span := tracer.StartSpan("My span name")
defer span.Finish()
  1. Embed it in the context.
ctx = opentracing.ContextWithSpan(ctx, span)
  1. Call your request function, passing ctx.
  2. In the request client, inject the span context into the HTTP request headers.
httpReq, _ := http.NewRequest("GET", "http://myservice/", nil)
if span := opentracing.SpanFromContext(ctx); span != nil {
    tracer.Inject(
        span.Context(),
        opentracing.HTTPHeaders,
        opentracing.HTTPHeadersCarrier(httpReq.Header))
}
  1. On the server side, extract the span context.
import (
    "github.com/opentracing/opentracing-go"
    "github.com/opentracing/opentracing-go/ext"
)

wireContext, err := tracer.Extract(
    opentracing.HTTPHeaders,
    opentracing.HTTPHeadersCarrier(req.Header))
serverSpan = opentracing.StartSpan(
    appSpecificOperationName,
    ext.RPCServerOption(wireContext))
defer serverSpan.Finish()
ctx := opentracing.ContextWithSpan(context.Background(), serverSpan)

See example: https://github.com/opentracing/opentracing-go#serializing-to-the-wire

mgale commented 2 years ago

I like that approach but I ran into the scenario of wanting to pass opentracing context plus custom data for my own purposes.

However, after thinking about it some more, I think I can just add the data I need to the span, maybe via bagged items or something: https://opentracing.io/docs/overview/tags-logs-baggage/

I will run some tests locally and hopefully, the above workflow will work for me.

Baliedge commented 2 years ago

@mgale Trace baggage seems like the right tool.

thrawn01 commented 2 years ago

I think we are going to pass on this PR for now. We should be able to do this by using the context or introducing trace support.