Replace gorilla/context with stdlib context

sourcegraph / appdash

Application tracing system for Go, based on Google's Dapper.

https://sourcegraph.com

Other

1.72k stars 137 forks source link

Replace gorilla/context with stdlib context #178

Closed nothingmuch closed 7 years ago

nothingmuch commented 8 years ago

This PR changes the httptrace API by requiring *http.Request to be returned from SetContextSpan. To avoid breaking ani API,this change can be avoided by overwriting the fields of the request structure directly, dereferencing the shallow clone returned by WithContext.

nothingmuch commented 8 years ago

Not sure what to do about 1.6. The context setting stuff could be split up with over two files with build tags, with gorilla/context as a fallback. This could also be done using a runtime check for

_, hasStdlibcontext := request.(interface { WithContext() *http.Request })

slimsag commented 8 years ago

This looks really good, thanks for the PR! A couple of thoughts:

Can you update .travis.yml to add Go 1.7?
I'm OK dropping support for Go 1.6, if https://github.com/go-kit/kit doesn't need it, otherwise your suggested approach with build tags sounds OK.
Adding the return value of *http.Request to MiddlewareConfig SetContextSpan looks OK, but just to be on the safe side let's confirm go-kit won't break by merging this.

@basvanbeek maybe you can confirm the last two (that this won't affect go-kit)?

basvanbeek commented 8 years ago

Go kit is currently tested with 1.5+ and we are very conservative with forcing our consumers to the latest and greatest without necessity. If I were an Appdash consumer with potential legacy code forcing me to stick with an older version of Go I would appreciate the effort of a library to remain backwards compatible where possible. I think the above mentioned build tags for a smooth migration path would be the best option.

Since the only touch points between Go kit and AppDash are initialization of the OpenTracing interface in one of our main examples, I can make it work if our CI would break because of this change.

cc @peterbourgon

slimsag commented 8 years ago

Makes perfect sense. Thanks for taking the time to respond @basvanbeek ! I agree with everything you have said.

@nothingmuch could you update this PR to:

add Go 1.7 to .travis.yml
Add <Go 1.6 backwards compatability via build tags

Or, if you prefer, just let me know and I can pick up the work / do the above before merging :)

nothingmuch commented 8 years ago

I would be happy to have another go at this, but I will probably need a few days more, I couldn't find the time this weekend, sorry.

slimsag commented 7 years ago

Closing due to inactivity + different solution @ https://github.com/sourcegraph/appdash/pull/182#issuecomment-262049135