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

How to update the context in a middleware #62

Closed
rantav opened this issue Jan 12, 2020 · 10 comments
Closed

How to update the context in a middleware #62

rantav opened this issue Jan 12, 2020 · 10 comments

Comments

@rantav
Copy link
Contributor

rantav commented Jan 12, 2020

Some libraries, such as opencensus take advantage of the context, for example storing http scoped request tags into the context and then recording metrics based on this saved context in downstream handlers.

If you'd like at ochttp, an opencensus plugin for net/http you'd find extensive usage of context.Context, including creating a new context and passing this context to the next handler downstream. I think this pattern is common.

My question is how can we simulate such behavior with atreugo/fasthttp. The Value method is not enough and so is the the SetUserValue (I have read the response to #48 but I thing that won't cut it).

We need a way to update the internal context.Context and store it in atreugo.RequestCtx, does that make sense? (is there a way to do that currently)

Thank you

@savsgio
Copy link
Owner

savsgio commented Jan 13, 2020

Hi @rantav,

The *fasthttp.RequestCtx and *atreugo.RequestCtx implements the interface of context.Context, so you could use the own RequestCtx to create a new copy of the context and modify it or any other thing.

So I don't understand why it's not enought the ctx.SetUserValue() and ctx.UserValue(), it's the same to do r = r.WithContext(context.WithValue(r.Context(), addedTagsKey{}, &tags))

What do you need exactly? Because, you need to store custom values in context (RequestCtx) to use in another handlers/views/middlewares in the same request, right?

@savsgio
Copy link
Owner

savsgio commented Jan 13, 2020

The difference of ctx.SetUserValue() with context.WithValue(), is just the keys must be a strings not interfaces.
But i think these wouldn't be a problem.

@rantav
Copy link
Contributor Author

rantav commented Jan 13, 2020

Hi @savsgio, thanks for the quick response, but I'm afraid that would not be enough.
Here's the scenario. I use opencensus, which adds stats and tracing so I create a before and after middleware using server.UseBefore and server.UseAfter

In the before middleware I do:

newCtx, span = trace.StartSpan(ctx, name)

This operation creates a new span and returns a new context newCtx. This newCtx contains a interface{} key (out of my control) to the new span.
From now on downstream middlewares or views are supposed to use the newCtx and in this newCtx they expect to find the span.

Here's how you read the span from the context:

span = trace.FromContext(ctx) // ctx is supposed to be the newCtx here, otherwise it would not work

Reference: https://godoc.org/go.opencensus.io/plugin/ochttp#hdr-Tracing

So two pieces are missing:

  1. Be able to use an interface{} key
  2. Pass the new context to ctx.Next() when calling the next middleware or view.

For example see how the request r is being updated here https://github.com/census-instrumentation/opencensus-go/blob/643eada29081047b355cfaa1ceb9bc307a10423c/plugin/ochttp/server.go#L91

@savsgio
Copy link
Owner

savsgio commented Jan 13, 2020

Could you give me the trace.StartSpan(...) documentation please?

@savsgio
Copy link
Owner

savsgio commented Jan 13, 2020

Okay, i could see that trace.StartSpan(...) return a context.Context, so the new context you must store in request ctx with ctx.SetUserValue("traceCtx", newCtx) in before middleware and recover it in after middlewares with newCtx := ctx.UserValue("traceCtx").(context.Context) and use it in trace.FromContext(ctx).

Maybe i could do it something similar in atreugo to ease this functionality, trying to make it as close as possible to net / http

@rantav
Copy link
Contributor Author

rantav commented Jan 13, 2020

Your suggestion to store the newCtx using ctx.SetUserValue("traceCtx", newCtx) is indeed the workaround I employed. But this looks unnatural (storing a context onto the context feels like Yack shaving...) plus, still the call to trace.FromContext(ctx) would not work on the provided atreugo.RequestCtx, as most users would expect (it would silently fail, e.g. return nil b/c it can't find it there), rather I"d have to:

// This returns nothing, which could be surprising to users
trace.FromContext(ctx)

// this workaround works, but is not obvious
newCtx  := ctx.UserValue("traceCtx").(context.Context)
// and only now: 
t := trace.FromContext(newCtx)

@savsgio
Copy link
Owner

savsgio commented Jan 14, 2020

I've been upload the fix, now you should do:

// Before middleware
newCtx, span = trace.StartSpan(ctx, name)
ctx.AttachContext(newCtx)


// After middleware or view
trace.FromContext(ctx)

I will release a new version throughout today.

@rantav
Copy link
Contributor Author

rantav commented Jan 14, 2020

Awesome, thank you!

@savsgio savsgio closed this as completed Jan 14, 2020
@rantav
Copy link
Contributor Author

rantav commented Jan 15, 2020

Hi, what happened to v10.1.0? Did you delete it?

@savsgio
Copy link
Owner

savsgio commented Jan 15, 2020

Ohh sorry, yes i deleted it after a few minutes of release, because i want to include the fix about the attached ctx.

I wil release it now.

And sorry again.

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

No branches or pull requests

2 participants