Add support for context.Context

[gaffneyc]

Status

READY

Description

This is an attempt to fix #35 where the global context ends up being shared across multiple go routines / threads. I've tried to keep the change itself small but had to break the existing API. The way Go solves this generally is to pass a context.Context as the first parameter to every method that needs to build up the state or deal with cancellation. It's been an interesting process to watch it get adopted and duct taped into libraries.

This is a breaking change as I removed honeybadger.SetContext and Client.SetContext due to there being no way in Go to make them safe for use. Other languages (like Ruby) have thread local storage but it just isn't a reasonable option in Go. I think it's better to remove them and cause a little bit of trouble on updates rather than leave them in and causing undetected issues for people.

I am not very happy with FromContext returning nil since it will force users to check for it each time they load the context. An alternative would be to return a blank Context but it wouldn't be attached to the context.Context which could be confusing.

A third option could be something like this to let people know that the returned Context isn't attached.

hbCtx, found := honeybadger.FromContext(ctx)
if !found {
  ctx = hbCtx.WithContext(ctx)
}

It might be worth adding a note about migrating to the new API for anyone that updates and sees this break.

Todos

• [X] Tests
• [X] Documentation
• [X] Changelog Entry (unreleased)

[gaffneyc]

I've started to integrate it into a service today and have a few thoughts or questions about the API. Go's context.Context builds kind of a Russian doll where the context itself gets unrolled as the stack rolls back up.

For example:

func main() {
  ctx, done := context.WithTimeout(context.Background(), time.Minute)
  defer done()

  someFunc(ctx)      // This will timeout after 1 second since someFunc wraps ctx
  talkToService(ctx) // This will have the remained of the minute to run since we're using the 
                     // unwrapped Context
}

func someFunc(ctx context.Context) {
  ctx, done := context.WithTimeout(ctx, time.Second)
  defer done()

  talkToService(ctx) // This will timeout after 1 seconds
}

With this implementation we end up with one global honeybadger.Context once it is set on a context.Context which I'm not sure if it's okay or would be too surprising.

For example someone might expect the context set in an http.Handler (like below) to be reported by the honeybadger.Handler.

func (c *ChainedHandler) ServeHTTP(resp http.ResponseWriter, req *http.Request) {
  ctx := honeybadger.Context{ "user_id": "1234" }.WithContext(req.Context())

  err := someDatabaseThing(ctx)
  if err != nil {
    panic(err) // honeybadger.Handler receives the err and includes the user_id in the context
  }

  // We could do something like this to set the context on the error itself before it is panic'd
  if err != nil {
    panic(honeybadger.NewError(err, ctx))
  }
}

One other issue I'm wondering about is how to handle the actual nesting. Let's say we have something like this:

func main() {
  ctx := honeybadger.Context{ "process": "wrangler"}.WithContext(context.Background())

  err := getUser(ctx, "1")
  if err != nil {
    // Should this notify include the user_id? Probably since it was the context when 
    // the error was created.
    honeybadger.Notify(err, ctx)
  }
}

func getUser(ctx context.Context, userID string) error {
  // Is user_id merged into the existing context? Or does it overwrite the existing context?
  ctx := honeybadger.Context{"user_id": userID}.WithContext(ctx)

  user := ...
  err := sendEmail(ctx, user.email)
  return err
}

[joshuap]

I've started to integrate it into a service today and have a few thoughts or questions about the API. Go's context.Context builds kind of a Russian doll where the context itself gets unrolled as the stack rolls back up.

For example:

func main() {
  ctx, done := context.WithTimeout(context.Background(), time.Minute)
  defer done()

  someFunc(ctx)      // This will timeout after 1 second since someFunc wraps ctx
  talkToService(ctx) // This will have the remained of the minute to run since we're using the 
                     // unwrapped Context
}

func someFunc(ctx context.Context) {
  ctx, done := context.WithTimeout(ctx, time.Second)
  defer done()

  talkToService(ctx) // This will timeout after 1 seconds
}

With this implementation we end up with one global honeybadger.Context once it is set on a context.Context which I'm not sure if it's okay or would be too surprising.

For example someone might expect the context set in an http.Handler (like below) to be reported by the honeybadger.Handler.

func (c *ChainedHandler) ServeHTTP(resp http.ResponseWriter, req *http.Request) {
  ctx := honeybadger.Context{ "user_id": "1234" }.WithContext(req.Context())

  err := someDatabaseThing(ctx)
  if err != nil {
    panic(err) // honeybadger.Handler receives the err and includes the user_id in the context
  }

  // We could do something like this to set the context on the error itself before it is panic'd
  if err != nil {
    panic(honeybadger.NewError(err, ctx))
  }
}

With the one global context, would honeybadger.Handler report the context from the nested handler? I think that's what I'd expect it to do.

One other issue I'm wondering about is how to handle the actual nesting. Let's say we have something like this:

func main() {
  ctx := honeybadger.Context{ "process": "wrangler"}.WithContext(context.Background())

  err := getUser(ctx, "1")
  if err != nil {
    // Should this notify include the user_id? Probably since it was the context when 
    // the error was created.
    honeybadger.Notify(err, ctx)
  }
}

func getUser(ctx context.Context, userID string) error {
  // Is user_id merged into the existing context? Or does it overwrite the existing context?
  ctx := honeybadger.Context{"user_id": userID}.WithContext(ctx)

  user := ...
  err := sendEmail(ctx, user.email)
  return err
}

I think in this case we'd want to prefer merging user_id into the existing context, unless I'm missing something. Personally, I would expect the user_id to be in the honeybadger.Notify(err, ctx) call. I think it would also be the most useful approach, since if getUser returns an error, I definitely want to know what the user_id was.

[gaffneyc]

Sorry I haven't had a chance to circle back to this in a couple weeks but I plan to in the next couple days.

[gaffneyc]

@joshuap 👋 As you said at MicroConf, kids really do make it impossible to get anything done (she's wonderful and already 5 months old 😱).

I've rebased this PR and plan to review it this week. I've been thinking about this issue a lot as I've been fighting with the usefulness of errors in the service I'm working.

Go 1.13

Since I opened this PR, Go 1.13 was released which introduced error wrapping and unwrapping. There is a good blog post about it and this one as well. Basically, an error can wrap it's cause (and that error can wrap it's cause (and that error can wrap it's cause (and...))) this allows you to test to see if an error was caused by a type you can handle, even if it's come through an intermediary library.

Context

This whole Wrap process means that Go tends to recommend adding context on the way back up from an error.

In Ruby, we often build up the context as we dig further down into the stack. So when we get to an exception, we've built it up and the stack is quickly unwound to the exception handler that will send the error to Honeybadger.

class Controller
  def get
    Honeybadger.context(user: user.id)
    widget = get_widget(params[:id])
  end

  def get_widget(id)
    Honeybadger.context(widget_id: id)
    Widget.find!(id) # => raise ActiveRecord::NotFound
  end
end

Here is a roughly equivalent Go version where each method call will add it's context to the context.Context so it can be attached at the boundary between our code and the library (GetWidget). To make this work NewError would need to take a Context and it currently doesn't. Also, not every function that can return an error can take a context.Context, so this method gets tricky in some cases.

func (w *WidgetsController) Get(resp http.ResponseWriter, req *http.Request) {
  ctx := req.Context()
  ctx = honeybadger.Context{"user": w.User}.WithContext(ctx)

  // This probably doesn't actually work
  err := w.getWidget(ctx, req.URL.Query.Get("id"))
  if err != nil {
    honeybadger.Notify(err)
    // or panic(err) if using the http.Handler middleware
  }
}

func (w *WidgetsController) getWidget(ctx context.Context, id string) error {
  ctx := honeybadger.Context{"widget_id": id}.WithContext(ctx)
  err := w.database.GetWidget(ctx, id) {
    // Need to wrap the error in a honeybadger.Error to get the stack trace. This is also
    // the closest to the Ruby example since the Context is attached to the Error and can
    // be returned by any intermediary functions.
    return honeybadger.NewError(err,  ctx)
  }

The Go Way

With 1.13, Go recommends wrapping errors with a text context (below). I've been thinking of this akin to unstructured logging. It can be hard to parse and inevitably someone is going to get the format wrong. There also ends up being a lot of duplication in methods that can produce a lot of errors.

func main() {
  err := foo("1")
  if err != nil {
    honeybadger.Notify(fmt.Errorf("main=%w", err))
  }
  err := foo("5")
  if err != nil {
    honeybadger.Notify(fmt.Errorf("main=%w", err))
  }
}

func foo(id string) error {
  err := bar()
  if err != nil {
    return fmt.Errorf("foo user=%d: %w", id, err)
  }
  return nil
}

func bar() error {
  err := ioutil.ReadAll(...)
  if err != nil {
    return fmt.Errorf("bar IO was read?: %w")
  }
  return nil
}

I've been thinking that a better (different?) solution (at least with Honeybadger) is something like what is below. It leans into structured context and maintains (more or less) the Go way of adding context on the way back up the call stack

func main() {
  err := foo("1")
  if err != nil {
    honeybadger.Notify(err)
  }
  err := foo("5")
  if err != nil {
    honeybadger.Notify(err)
  }
}

func foo(id string) error {
  err := bar()
  if err != nil {
    return honeybadger.Wrap(err, honeybadger.Context{"user": id})
  }
  return nil
}

func bar() error {
  err := ioutil.ReadAll(...)
  if err != nil {
    return honeybadger.Wrap(err, honeybadger.Context{"msg": "IO was read?"})
  }
  return nil
}

Anyways... I'm not sure the big take away from this. I think that being able to add Honeybadger context to the context.Context as we call down the stack is useful and needs NewError to be able to take a Context.

[joshuap]

@joshuap 👋 As you said at MicroConf, kids really do make it impossible to get anything done (she's wonderful and already 5 months old 😱).

Wow, time flies... congrats! 😊 Mine are 3 and 2 now. 😬

I've rebased this PR and plan to review it this week. I've been thinking about this issue a lot as I've been fighting with the usefulness of errors in the service I'm working.

Sounds good on the review. Let me know if there's any additional specific input you need.

Go 1.13

Since I opened this PR, Go 1.13 was released which introduced error wrapping and unwrapping. There is a good blog post about it and this one as well. Basically, an error can wrap it's cause (and that error can wrap it's cause (and that error can wrap it's cause (and...))) this allows you to test to see if an error was caused by a type you can handle, even if it's come through an intermediary library.

Off topic: Cool! Ruby and Java (among others, probably) do this too, and we support displaying causes. If it's possible to unwrap an error into a list of causes, then we could display each cause individually. I haven't read the blog posts you linked to yet (I will), so I'm not sure if this is feasible.

Context

This whole Wrap process means that Go tends to recommend adding context on the way back up from an error.

In Ruby, we often build up the context as we dig further down into the stack. So when we get to an exception, we've built it up and the stack is quickly unwound to the exception handler that will send the error to Honeybadger.

class Controller
  def get
    Honeybadger.context(user: user.id)
    widget = get_widget(params[:id])
  end

  def get_widget(id)
    Honeybadger.context(widget_id: id)
    Widget.find!(id) # => raise ActiveRecord::NotFound
  end
end

Here is a roughly equivalent Go version where each method call will add it's context to the context.Context so it can be attached at the boundary between our code and the library (GetWidget). To make this work NewError would need to take a Context and it currently doesn't. Also, not every function that can return an error can take a context.Context, so this method gets tricky in some cases.

func (w *WidgetsController) Get(resp http.ResponseWriter, req *http.Request) {
  ctx := req.Context()
  ctx = honeybadger.Context{"user": w.User}.WithContext(ctx)

  // This probably doesn't actually work
  err := w.getWidget(ctx, req.URL.Query.Get("id"))
  if err != nil {
    honeybadger.Notify(err)
    // or panic(err) if using the http.Handler middleware
  }
}

func (w *WidgetsController) getWidget(ctx context.Context, id string) error {
  ctx := honeybadger.Context{"widget_id": id}.WithContext(ctx)
  err := w.database.GetWidget(ctx, id) {
    // Need to wrap the error in a honeybadger.Error to get the stack trace. This is also
    // the closest to the Ruby example since the Context is attached to the Error and can
    // be returned by any intermediary functions.
    return honeybadger.NewError(err,  ctx)
  }

The Go Way

With 1.13, Go recommends wrapping errors with a text context (below). I've been thinking of this akin to unstructured logging. It can be hard to parse and inevitably someone is going to get the format wrong. There also ends up being a lot of duplication in methods that can produce a lot of errors.

func main() {
  err := foo("1")
  if err != nil {
    honeybadger.Notify(fmt.Errorf("main=%w", err))
  }
  err := foo("5")
  if err != nil {
    honeybadger.Notify(fmt.Errorf("main=%w", err))
  }
}

func foo(id string) error {
  err := bar()
  if err != nil {
    return fmt.Errorf("foo user=%d: %w", id, err)
  }
  return nil
}

func bar() error {
  err := ioutil.ReadAll(...)
  if err != nil {
    return fmt.Errorf("bar IO was read?: %w")
  }
  return nil
}

I've been thinking that a better (different?) solution (at least with Honeybadger) is something like what is below. It leans into structured context and maintains (more or less) the Go way of adding context on the way back up the call stack

func main() {
  err := foo("1")
  if err != nil {
    honeybadger.Notify(err)
  }
  err := foo("5")
  if err != nil {
    honeybadger.Notify(err)
  }
}

func foo(id string) error {
  err := bar()
  if err != nil {
    return honeybadger.Wrap(err, honeybadger.Context{"user": id})
  }
  return nil
}

func bar() error {
  err := ioutil.ReadAll(...)
  if err != nil {
    return honeybadger.Wrap(err, honeybadger.Context{"msg": "IO was read?"})
  }
  return nil
}

Anyways... I'm not sure the big take away from this. I think that being able to add Honeybadger context to the context.Context as we call down the stack is useful and needs NewError to be able to take a Context.

I really like that last example; it's easy for my brain to grasp (maybe better than building up the context as you move down?) Attaching the context to the error seems like a pretty useful approach.

I'm going to pull @rabidpraxis in on this issue too since he has a little Go experience as well.

[matrixik]

Hello @gaffneyc are you still interested in pushing this forward?

[gaffneyc]

@matrixik I don't have any plans at returning to this PR at the moment. I no longer think that using context.Context is a good direction after having used this PR over the last year... errrrr.... two(!?) years.

Using context.Context directly doesn't work well since not every function or method that needs to provide error details will take a context.Context. It makes more sense to use Go's approach of storing the context directly on the error itself (similar to what fmt.Errorf does) but in a structured way.

I came back to this problem a month or two ago and ended up starting a new client from scratch. I wanted to explore some options in the context of a real app without having to worry about breaking backwards compatibility. I'm hoping to open source it eventually but don't know when that would happen.

[matrixik]

Thank you for sharing your experiences.

[joshuap]

@gaffneyc if you would be willing to give us some direction on what you're thinking, I could find someone (paid contractor) to integrate w/ our current client, if that would help get it done faster.

[gaffneyc]

@joshuap Yeah, that would be great. I'm not sure the best way to get started. I can pull the version I've been working on out of the project it's own repo and send you an invite. It would be easier to discuss my thoughts being able to see the code.

Would that work?

[joshuap]

@joshuap Yeah, that would be great. I'm not sure the best way to get started. I can pull the version I've been working on out of the project it's own repo and send you an invite. It would be easier to discuss my thoughts being able to see the code.

Would that work?

Yep, that's great. I could also create a repo and invite you if you want? Let me know which you prefer. Thanks!

[gaffneyc]

@joshuap Sorry it's taken a bit! There seems to always be something else pressing. I've invited you to the repository and tried to write up some general notes in the readme on what I have been thinking about with the repository. There are also copious amounts of TODOs and notes in the source.

Happy to help out in any way I can to help move it forward or get integrated into here.

[joshuap]

Thanks, @gaffneyc! I'll take a look.

[james-lawrence]

given lack of progress on this and the fact errors.As,errors.Is exist I don't think this is worth attempting to wire into honeybadger anymore. it can be easily added by clients and is better approached as a larger community thing.

type Contextual interface {
    Unwrap() error
    Context() map[string]any
}

type contextual struct {
    cause   error
    details map[string]any
}

func (t *contextual) Add(k string, v any) *contextual {
    t.details[k] = v
    return t
}

func (t contextual) Unwrap() error {
    return t.cause
}

func (t contextual) Error() string {
    return t.cause.Error()
}

func (t contextual) Is(target error) bool {
    _, ok := target.(Contextual)
    return ok
}

func (t contextual) As(target any) bool {
    if x, ok := target.(*contextual); ok {
        *x = t
        return ok
    }

    return false
}

func NewContext(cause error) *contextual {
    return &contextual{
        cause:   cause,
        details: make(map[string]any),
    }
}

func Context(cause error) map[string]any {
    var c contextual

    if errors.As(cause, &c) {
        return c.details
    }

    return make(map[string]any)
}

honeybadger.Notify(
  cause,
  honeybadger.Context(errorsx.Context(cause)),
)

essentially if someone wants to tag a context.Context with information and provide it to an error and then extract that information for reporting its fairly straight forward boiler plate and should be left up to them.

[joshuap]

@james-lawrence thanks for this example!

@gaffneyc does this make sense to you?

cc @stympy @rabidpraxis since they've been writing some Go at Honeybadger lately. I am pretty far out of this headspace personally.

[gaffneyc]

Yeah, that's similar to what I did in the client I wrote. The API is a little different but it's the same idea of storing context on the errors themselves instead of in a context.Context.

Happy to give anyone else who wants to look at the repo access and would love to see improvements to the official client. I've been using it for a while and it's held up reasonably well. Unfortunately I don't want to open source it since I don't have the time to maintain it for others.