Closed seanhagen closed 4 years ago
This should address #35
Not sure why this is failing or what can be done to fix:
1.48s$ make deps
# dependencies
go get github.com/pborman/uuid
# github.com/pborman/uuid
../../pborman/uuid/time.go:15: syntax error: unexpected = in type declaration
make: *** [deps] Error 2
@seanhagen super sorry for the delay in reviewing this--thanks for submitting it! I'm looking over it now. Prepare for some dumb questions; context passing in go is pretty new to me. :)
It sounds like this solves the problem of scoping context to the request when using the honeybadger.Handler
implementation. Is that correct?
My first question is how to get ctx
if I'm just reporting a regular error from say, an executable script (no concurrency, outside of a request). For instance:
honeybadger.Notify(ctx, err)
Where does ctx
come from? Looking at the test changes, it looks like I would have to do ctx := context.Background()
to get the context? If I want to use honeybadger.SetContext
in that same script, will it work with that context object?
If that's the case, is there anything we can do to smooth the user interface for users who don't want to think about shared context (i.e., say they just want to call honeybadger.Notify(err)
)? Or will they always have to grab a default context.Context
from somewhere, and pass it in as the first argument?
Yeah, this will prevent the honeybadger context ( ie, setting the information that should be reported if Notify
is called -- having two things named context gets confusing ) from getting clobbered when honeybadger is used within goroutines.
I also just updated the honeybadger.Handler
so that it adds some additional info.
Basically, think of the context.Context
as where the values get stored, instead of within a global state in the honeybadger package. So yeah, if you're writing a script, you'd use ctx := context.Background()
or context.TODO()
to get a context that you then pass into every call to honeybadger.SetContext
& to honeybadger.Notify
if an error occurs.
Unfortunately, in the case of the http handler, because the request context can't be replaced if SetContext
is within the handler ( or in a middleware lower down the chain ), the panic catching won't see any of that info. So fixing the "overwriting-context-in-goroutines" has the side effect of "the panic handler in honeybadger.Handler
doesn't see anything set via honeybadger.SetContext
once it calls h.ServeHTTP(w, req)
".
I don't think that's too clear, so here's a code example:
mux := http.NewServeMux()
mux.HandleFunc("/test", func(w http.ResponseWriter, r *http.Request){
ctx := honeybadger.SetContext(r.Context(), map[string]interface{}{"value": "something important"})
// important code
if err != nil {
// this error in honeybadger will have "value" = "something important" in the context
honeybadger.Notify(ctx, err)
}
})
mux.HandleFunc("/panics", func(w http.ResponseWriter, r *http.Request){
// this data won't be seen if the function panics
// because it's a new context, not the same one that the panic handler in client.go
// has access to
ctx := honeybadger.SetContext(r.Context(), map[string]interface{}{"value": "very important"})
// important code
if err != nil {
panic(err)
}
})
http.ListenAndServe(":8080", honeybadger.Handler(mux))
In the route /test
, we'll get all the context set in client.go in Handler ( line 121 ) when we call honeybadger.Notify(ctx, err)
, along with the {"value": "something important"}
context info.
However, in /panics
, it'll only have the context set within client.go, because it doesn't have the context returned from SetContext
It's a downside, for sure, but there's no way around it without global values -- and then we're right back to data getting clobbered when it's run in goroutines ( which is just unavoidable in web servers, http.ListenAndServe
uses goroutines to handle incoming requests, so do GRPC servers ).
Re: still being able to call honeybadger.Notify(err)
I could change the new Notify
to be named NotifyCtx
. It would mean that the new behaviour for Notify
would be "sends the error and only extra info passed in at the time of calling Notify
", because without the context.Context
it can't grab previously stored information. Can't put it into global state, because that's what was causing the "overwrite-context-in-goroutine" problem in the first place :disappointed:
I've been using my fork in production for a bit now, and it works fine and no data gets clobbered or shows up where it shouldn't in the Honeybadger app.
@seanhagen Take a look at how Bugsnag handles context--they do something similar to what you're doing here: https://docs.bugsnag.com/platforms/go/net-http/#manage-http-session-data-with-context-context
In their Notify
method, they use the trailing splat argument (rawData ... interface {}
) to collect extra (optional) data like we do, including ctx
(I'm going to refer to go-context as ctx
from now on, and Honeybadger context as context). They use the same approach in their AutoNotify
and Recover
methods (which are like our Monitor
method). https://github.com/bugsnag/bugsnag-go/blob/master/bugsnag.go#L70
I'd like to take the same approach (adding ctx
as an optional trailing argument to all methods) here. This way, existing users won't have to change their existing apps. To take advantage of context, they'll start passing ctx
as an optional end-argument to Notify
, Monitor
, and Context
. This should also make having a second notify method (like NotifyCtx
unnecessary).
What do you think?
What is the status of this PR? It would be really nice to get it merged soon.
I can't merge this PR as-is, unfortunately. See my comment here (and my review).
I don't have time to do it myself atm; PRs are welcome of course, but otherwise I could schedule this to happen in the next couple months. If anyone does want to work on this, comment here/on #35 to make sure that we don't duplicate effort.
Closing this in favor of #39 (which is also hanging atm, but a better direction).
Update
Client.SetContext
to requirecontext.Context
To keep the Honeybadger context scoped to a single request, store the*contextSync
value in acontext.Context
. This way there's no possibility of the request context data being overwritten due to the function being called in a concurrent setting.Update
Client.Notify
to requirecontext.Context
In order to get the context stored in theClient.SetContext
call, need to require a context as the first argument.Update Monitor to require
context.Context
Same as other functions,client.Notify
requires acontext.Context
as the first argument so Monitor has to be updated as well. Does mean that a panic won't have the context information unless it's called within the function that might panic instead of at the top level.Ie:
Won't be able to get the values set unless it's called like so:
No more need for
Client.context
, so removing itUpdate
Client.Handler
to user.Context()
Update functions to require
context.Context
Use
context.Background
inMonitor()
Version bump -- v0.5.0