Context based error reporting

[seanhagen]

Update Client.SetContext to require context.Context
To keep the Honeybadger context scoped to a single request, store
the *contextSync value in a context.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 require context.Context
In order to get the context stored in the Client.SetContext call, need to
require a context as the first argument.

Update Monitor to require context.Context
Same as other functions, client.Notify requires a context.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:

func main(){
  defer honeybadger.Monitor()
  // set up http handlers and whatnot
}

Won't be able to get the values set unless it's called like so:

func handler(w http.ResponseWriter, r *http.Request){
  defer honeybadger.MonitorCtx(r.Context())
  // rest of handler
}

No more need for Client.context, so removing it

Update Client.Handler to use r.Context()

Update functions to require context.Context

Use context.Background in Monitor()

Version bump -- v0.5.0

[seanhagen]

This should address #35

[seanhagen]

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

[joshuap]

@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?

[seanhagen]

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 😞

[seanhagen]

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.

[joshuap]

@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?

[joshuap]

Adding this data is great, but we'll want to send it as cgi_data (with special keys) instead of context--that's what we use to display request data in error reports. For an example, search for "cgi_data" on this page: https://docs.honeybadger.io/api/exceptions.html#sample-payload

Here's how we construct it in Elixir, for instance: https://github.com/honeybadger-io/honeybadger-elixir/blob/94c6c1f29cef70ae754bf12aa99779849ddd0b70/lib/honeybadger/plug_data.ex#L49

I'd probably hold off on including this in the current PR (since it's kind of a separate feature) and then submit a subsequent PR after we get the new ctx API worked out.

[rzaharenkov]

What is the status of this PR? It would be really nice to get it merged soon.

[joshuap]

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.

[joshuap]

Closing this in favor of #39 (which is also hanging atm, but a better direction).