Giter VIP home page Giter VIP logo

honeybadger-go's Issues

Move several types to pointers

There's a couple of spots where you are mixing pointers and value types for core types like Configuration and Context. For instance, your Configuration type is a pointer here but a value here, forcing you to awkwardly deference it. Unless you are explicitly trying for a functional-ish API (i.e. immutable), I'd suggest moving to use pointers exclusively for composite types; it ensures you aren't spending extra cycles copying data needlessly and can mutate method parameters.

Error Class

Many of the errors in our applications are errors.errorString struct. However, when the errors are sent to Honeybadger, they are all treated as the same error because they are of the same class.

Can Error.Class be updated to be the message if the type is errorString struct? It would make find errors easier, and not force us to create new error types.

Add config option to report errors synchronously

This feature will add a boolean Synchronous config option. When true, calls to Notify() will skip the worker and call the backend directly. The return value should still be a string UUID. When false (default), the behavior will be what it is now -- the notice is passed to the worker and the UUID is returned immediately.

See #22 for context around this feature request. PRs for this are welcome!

Concurrent map writes

Greetings,

Looks like there is an issue with type hash map[string]interface{} used in

type Context hash

// Update applies the values in other Context to context.
func (context Context) Update(other Context) {
	for k, v := range other {
		context[k] = v
	}
}

Maps are not safe for concurrent use as per https://golang.org/doc/faq#atomic_maps and so we often face the following error:

fatal error: concurrent map writes

goroutine 1387 [running]:
runtime.throw(0x9de572, 0x15)
	/usr/local/go/src/runtime/panic.go:605 +0x95 fp=0xc420049c58 sp=0xc420049c38 pc=0x42c115
runtime.mapassign_faststr(0x942180, 0xc42006db60, 0xc421a877c0, 0xd, 0x900480)
	/usr/local/go/src/runtime/hashmap_fast.go:861 +0x4da fp=0xc420049cd8 sp=0xc420049c58 pc=0x40e99a
github.com/honeybadger-io/honeybadger-go.Context.Update(0xc42006db60, 0xc4223a43f0)
	/go/src/github.com/honeybadger-io/honeybadger-go/context.go:9 +0xd7 fp=0xc420049d80 sp=0xc420049cd8 pc=0x7842b7
github.com/honeybadger-io/honeybadger-go.(*Client).SetContext(0xc4200894c0, 0xc4223a43f0)
	/go/src/github.com/honeybadger-io/honeybadger-go/client.go:39 +0x3c fp=0xc420049da0 sp=0xc420049d80 pc=0x7826dc
github.com/honeybadger-io/honeybadger-go.SetContext(0xc4223a43f0)
	/go/src/github.com/honeybadger-io/honeybadger-go/honeybadger.go:60 +0x37 fp=0xc420049dc0 sp=0xc420049da0 pc=0x784c37

Probably better replace with sync.Map or something like that.

Can't access the original error in before notify.

What are the steps to reproduce this issue?

honeybadger.Notice(MyErrorf("example"))

honeybadger.BeforeNotify(func(notice *honeybadger.Notice) error {
   log.Printf("%T\n", notice.Error) // is a honeybadger.Error which doesn't expose the underlying error.
   return nil
})

What happens?

before notify handlers receive a honeybadger.Notice, but the error being wrapped isn't accessible.

What were you expecting to happen?

I'd expect to be able to access the underlying cause using stdlib functions like unwrap, is, as. I'd also expect to be able to control the stack trace from error provided to the notice function.

What versions are you using?

Operating System: linux
Package Version: v0.6.0
Go Version: 1.21

Context is shared across goroutines

As described in #33, our context implementation in honeybadger-go is a bit naive; because it's global, it is prone to collisions when multiple goroutines update it (which is especially common in server implementations). Consider this scenario from @odarriba:

  1. request A starts (one goroutine) -> set context A
  2. request B starts (another goroutine) -> set context B
  3. request A fails -> sends to HB with context B

The way we solve this issue in Ruby is using a thread local variable which solves the concurrent access problem, and then we reset the context after each request which solves the local request context issue. We do something similar in Elixir--the context is stored in the process dictionary.

I've been reading up on how Go handles passing request context across goroutines, specifically:

https://blog.golang.org/context
http://www.gorillatoolkit.org/pkg/context

From what understand, Go does not provide anything similar to thread local variables or a process dictionary, preferring to pass a context value as described in the blog post.

There may be a way we can integrate with this, but I'm still wrapping my head around it, so I'm not sure what that might be yet. I'm open to suggestions!

In the meantime, we have to live with the caveat that honeybadger.SetContext will not work reliably across goroutines.

Front logo Front conversations

errcheck lint warnings

This issue isn't necessarily actionable; it's just to provide some further documentation of lint warnings. Today I fixed all warnings but the following:

client.go:64:3:warning: error return value not checked (client.Notify(newError(err, 2))) (errcheck)
client.go:75:5:warning: error return value not checked (client.Notify(newError(err, 2), Params(r.Form), getCGIData(r), *r.URL)) (errcheck)
honeybadger.go:69:3:warning: error return value not checked (client.Notify(newError(err, 2))) (errcheck)
server.go:61:8:warning: error return value not checked (defer resp.Body.Close()) (errcheck)

In the first 3 I really don't want to do anything with an error if there is one (I'm already logging it to the logger, and in a recovered state I don't want to do extra error handling related to error notification). The last warning is because I am deferring resp.Body.Close(), which I took directly from the documentation for the http package; doesn't seem like it's a big deal.

Can't `go get` honeybadger-go

When I try to run go get honeybadger-io/honeybadger-go, I get

github.com/honeybadger-io/honeybadger-go/notice.go:101: undefined: load.LoadAvg

load is from shirou/gopsutil. I think the problem is that gopsutil recently updated to v2 and when doing so changed load.LoadAvg to just load.Avg, as seen in this file. It appears that honeybadger-go isn't using Godeps to vendor dependencies, so I think it was automatically trying to get the latest version of shirou/gopsutil, which now defines a new api. At least I think this could be what's going on, but I'm not positive.

Looking forward to using honeybadger on my go project!

Filtering sensitive data

We need a way to filter sensitive data, like we do for Ruby and Elixir:

https://docs.honeybadger.io/lib/ruby/getting-started/filtering-sensitive-data.html#disable-data-completely
https://docs.honeybadger.io/lib/elixir.html#configuration

Three potential features related to this:

  • List of keys to filter (see filter_keys in Elixir)
  • Programatically filter data (see filter and before_notify in Elixir and Ruby respectively)
  • Disable certain request data entirely (see disable_params, disable_session, etc. in Ruby)

Consider some mechanism for handling worker errors in the default client?

Hello!

First of all, thanks for the great service and the great Go client.

I have not been able to find an existing mechanism in the library to be able to take action in the case of a worker failure in the default client, for example due to network issues (say, my server is unable to connect to Honeybadger's API).

I am able to work around it with your current implementation, but my solution feels a bit hackish. I am wrapping the Notify method of the default server backend so that I can do something when it fails (since errors that get returned to workers just get logged).

So, I have two possible proposals:

  • Boolean "Synchronous" field in the Configuration struct
    • In the default Client's Notify method, check client.Config.Synchronous
      • If false: use the same behavior as now (which prevents me from using my own buffering and dealing with network failures, timeouts etc.)
      • If true: return the value of calling the backend's notify directly
    • Pros:
      • Backward compatible
      • Lets users implement their own queuing, circuit breakers, additional logic on failure & success
    • Cons:
      • User has to implement their own queuing/workers if they use it
      • This doesn't take the metric collector's use of asynchronous workers into consideration-- since they don't use the Client, and since their use of Notify is invisible to end-users, it might be confusing to have the Synchronous value impact error reporting in the metrics collector. I think this is a solvable problem, but I have not solved it (I am not currently using the metrics collector).
  • Optional "WorkerErrors" channel on the client, which, if it is set, the worker writes to when it runs into issues with its work
    • Pros:
      • Backwards compatible
      • User doesn't have to implement their own workers
      • The metrics collector gets it for free
    • Cons:
      • User has less control over how the library talks to the API

I don't have a strong opinion—I didn't want to just write a feature request without trying to think it through. Either of these would solve the issue I'm having, and I think that it would be nice if the library provided access to worker errors in some fashion.

Thanks!

Publicizing stack frame generation

Hi! We currently use Logrus + Hooks to dispatch errors to Honeybadger using the package https://github.com/bobbytables/logrus-honeybadger

We've noticed that the Class when using Go typically defaults to *errors.errorString, usually because of this line: https://github.com/honeybadger-io/honeybadger-go/blob/master/error.go#L50

This can be solved by calling Notify on the client with the honeybadger.Error type where you can set the Class struct property manually. However, getting the frames for the current stacktrace is an unexported function in this package.

So what I'm proposing is to export the function defined here https://github.com/honeybadger-io/honeybadger-go/blob/master/error.go#L55

This would allow generating the Error type very easy to pass into the Notify function.

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.