Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

mgale
Copy link

@mgale mgale commented Feb 7, 2022

Initial idea on how this feature could be implemented.

@thrawn01
Copy link
Contributor

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
Copy link

Baliedge commented Feb 10, 2022

@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
Copy link
Author

mgale commented Feb 10, 2022

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
Copy link

@mgale Trace baggage seems like the right tool.

@thrawn01
Copy link
Contributor

thrawn01 commented Jul 6, 2022

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.

@thrawn01 thrawn01 closed this Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants