Make airbrake hook non-blocking #157

gravis · 2015-03-25T16:27:46Z

Logging should not wait for airbrake to be notified.

gravis · 2015-03-25T16:34:15Z

I'm not sure it's the right solution btw. Having a AddAsyncHook is probably better since we leave the decision to the user.

sirupsen · 2015-03-26T15:00:09Z

hooks/airbrake/airbrake.go

@@ -25,6 +25,11 @@ func NewHook(endpoint, apiKey, env string) *airbrakeHook {
 }

 func (hook *airbrakeHook) Fire(entry *logrus.Entry) error {
+	go hook.fire(entry) // Don't block logging


I think a better way to do this is that NewHook spawns a go routine that reads off a channel, avoiding spawning an unlimited number of goroutines. The channel should be buffered, and if fire() can't write to it it prints an error like the current one.

Good point!
I'll update that today.
Thanks

The fire() func is spawned only once and will loop over a buffered channel. The channel size can be set using graylog.BufSize, but the default 1024 should be more than enough. Remember the call to Fire will start to block if the channel is full (airbrake doesn't respond). Please not that tobi/airbrake-go *never* times out on sending the error to airbrake, so I think this is a timebomb. The official github.com/airbrake/gobrake has a default timeout of 10s, which is a lot better I think.

gravis · 2015-03-26T20:16:46Z

Done!
Please note that tobi/airbrake-go never times-out on sending the error to airbrake, so I think this is a timebomb.
The official github.com/airbrake/gobrake has a default timeout of 10s, which is a lot better I think.
I strongly suggest to use the official repo instead.

sirupsen · 2015-03-26T20:30:05Z

Ya, let's switch.

gravis · 2015-03-26T21:24:02Z

The api is slightly different:

airbrake.ApiKey = hook.APIKey
airbrake.Endpoint = hook.Endpoint
airbrake.Environment = hook.Environment

from github.com/airbrake/gobrake

airbrake = gobrake.NewNotifier(projectId, apiKey)
airbrake.SetContext("environment", "production")

There 2 options:

extract the projectId from the current endpoint param. (API stays the same, but the end isn't "gobrake" idiomatic, and the user could wonder why the endpoint is not working as expected)
modify the API of NewHook() to take a projectId directly. (API changes dangerously, as we still accept a string, so previous code with endpoint will still compile but won't work)

Any ideas here?

gravis · 2015-03-26T21:27:53Z

for the record, tobi's version is also using a old endpoint by default:

Endpoint    = "https://api.airbrake.io/notifier_api/v2/notices"

whereas gobrake is using:

"https://airbrake.io/api/v3/projects/%d/notices?key=%s",

sirupsen · 2015-03-27T14:01:53Z

Let's break the API. It's for the better long-term. Please update the README as well 👍

gravis · 2015-03-27T14:09:52Z

Thanks for the insight, will do that today.
I also agree it's better on the long term, especially before a 1.0 release.

gravis · 2015-03-27T15:30:31Z

Okkkk, so I just discover gobrake only works with http.Request:

func (n *Notifier) Notify(e interface{}, req *http.Request) error {
    notice := n.Notice(e, req, 3)
    if err := n.SendNotice(notice); err != nil {
        log.Printf("gobrake failed (%s) reporting error: %v", err, e)
        return err
    }
    return nil
}

That's lame... So we'll stick to tobi's version, and I'll create a PR in @tobi 's repo to make sure http requests don't stack to death if airbrake api is unreachable.

Sorry for the mess.

sirupsen · 2015-03-27T15:33:10Z

Can't you just pass it nil?

gravis · 2015-03-27T15:37:49Z

sounds like we can:

https://github.com/airbrake/gobrake/blob/master/notice.go#L55

will try that and let you know

- fixes examples in README.md that incorrectly state usage of RFC3339Nano format instead of RFC3339

I have updated the list to be a table instead. It's much more readable than the previous version, and used in a lot of places (ex: https://github.com/codegangsta/negroni/blob/master/README.md)

gravis · 2015-03-27T21:17:36Z

There you go!

gravis · 2015-03-27T21:34:14Z

The build is failing because of the import path:

# github.com/Sirupsen/logrus/hooks/airbrake
hooks/airbrake/airbrake_test.go:128: cannot use notice.Errors[0] (type gobrake.Error) as type *gobrake.Error in send

But I can ensure they are working:

ok      github.com/Sirupsen/logrus      0.021s
?       github.com/Sirupsen/logrus/examples/basic       [no test files]
?       github.com/Sirupsen/logrus/examples/hook        [no test files]
ok      github.com/Sirupsen/logrus/formatters/logstash  0.030s
ok      github.com/Sirupsen/logrus/hooks/airbrake       0.022s
ok      github.com/Sirupsen/logrus/hooks/bugsnag        0.027s
ok      github.com/Sirupsen/logrus/hooks/papertrail     0.017s
ok      github.com/Sirupsen/logrus/hooks/sentry 0.019s
ok      github.com/Sirupsen/logrus/hooks/syslog 0.016s
> echo $?
0

mattbostock · 2015-03-28T18:44:14Z

👍 to making the hook non-blocking, I think this is important.

I too like the idea of using the official Airbrake client, however that presents a problem in that:

it hardcodes the Airbrake endpoint which means that it doesn't work with API-compatible products such as Errbit
only supports version 3 of the Airbrake API (Errbit supports version 2)

gravis · 2015-04-09T14:10:17Z

I don't have a solution for this.
Only possibility is to create a pull request in tobi's repo to use a different http client (like the official gobrake). I have no idea if @tobi would accept something like that?
https://github.com/airbrake/gobrake/blob/master/notifier.go#L17

Thanks

tobi · 2015-04-09T15:30:23Z

Sure. Whatever is best

On Thursday, April 9, 2015, Philippe Lafoucrière notifications@github.com
wrote:

I don't have a solution for this.
Only possibility is to create a pull request in tobi's repo to use a
different http client (like the official gobrake). I have no idea if @tobi
https://github.com/tobi would accept something like that?
https://github.com/airbrake/gobrake/blob/master/notifier.go#L17

Thanks

—
Reply to this email directly or view it on GitHub
#157 (comment).

tobi
CEO Shopify

gravis · 2015-04-09T15:31:58Z

Thanks for the input, I'll propose a pull request then in the coming days.

sirupsen · 2015-05-08T14:20:22Z

@gravis do you have an update on this?

gravis · 2015-05-08T14:21:27Z

I'm fighting emergencies currently, and didn't find time to update tobi's version yet :(

otherwise, notifications (http post request to the API) can stack forever if the server is not responding. see sirupsen/logrus#157 (comment)

sirupsen · 2015-10-05T11:39:32Z

@gravis ping :)

gravis · 2015-10-05T12:01:56Z

@sirupsen, we're waiting for @tobi to merge:
tobi/airbrake-go#16
I would suggest to use my original version with the official airbrake (gobrake) implementation if you want to be solved faster.
Thanks

sirupsen · 2015-10-05T12:03:57Z

I'll send him an email and ask him to add me as contributor.

sirupsen · 2015-10-05T12:04:44Z

On the other hand, internally we don't have any use for a custom endpoints anymore going forward.

How about you rebase this and we get it in?

sirupsen · 2015-10-05T12:05:01Z

Also are you interested in taking over maintenance of the Airbrake hook under your personal Github to keep as much as possible out of core?

gravis · 2015-10-05T12:11:27Z

Sure. How would you see that? We're merging this, and then we create the new repo?

gravis · 2015-10-05T12:13:09Z

Ok, will do that today or tomorrow then.

sirupsen · 2015-10-05T12:13:43Z

We'll close this PR and merge a change that points to your repository in the README :)

tobi · 2015-10-05T12:34:31Z

I'm a crappy dependency for software projects these days. Happy to add someone else to that repo so you don't have to wait for me.

tobi
CEO Shopify

(mobile)

On Oct 5, 2015, 8:13 AM -0400, Simon Eskildsennotifications@github.com, wrote:

We'll close this PR and merge a change that points to your repository in the README :)

—
Reply to this email directly orview it on GitHub(#157 (comment)).

sirupsen · 2015-10-05T12:47:53Z

@tobi feel free to add me. We may go ahead with something else though.

gravis · 2015-10-05T19:06:53Z

Ok, the change has been merged in tobi's repo.
We can close this issue, since the official gobrake isn't going to be used.
I wonder now if we won't create 2 separate pluging repos:

one legacy airbrake (the current version)
one official, using airbrake v3 api.
We'll propose some pull requests this week.

Make airbrake hook non-blocking

2206439

sirupsen reviewed Mar 26, 2015
View reviewed changes

Tiffany Low and others added 4 commits March 27, 2015 17:17

allow custom time format string in TextFormatter

894fbb6

- fixes examples in README.md that incorrectly state usage of RFC3339Nano format instead of RFC3339

changelog: add #158

3d51ce3

Add graylog hook to Readme

66048db

I have updated the list to be a table instead. It's much more readable than the previous version, and used in a lot of places (ex: https://github.com/codegangsta/negroni/blob/master/README.md)

Switch to the officiel gobrake package

8fd0a67

Philippe Lafoucrière added 2 commits March 27, 2015 17:21

Fix example to user the new airbrake hook API

d632d77

Update README according to new api

4f2e161

gravis pushed a commit to gemnasium/airbrake-go that referenced this pull request Aug 13, 2015

Use a httpClient with a default timeout

66fcfd6

otherwise, notifications (http post request to the API) can stack forever if the server is not responding. see sirupsen/logrus#157 (comment)

gravis mentioned this pull request Aug 13, 2015

Use a httpClient with a default timeout tobi/airbrake-go#16

Merged

gravis closed this Oct 5, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make airbrake hook non-blocking #157

Make airbrake hook non-blocking #157

gravis commented Mar 25, 2015

gravis commented Mar 25, 2015

sirupsen Mar 26, 2015

gravis Mar 26, 2015

gravis commented Mar 26, 2015

sirupsen commented Mar 26, 2015

gravis commented Mar 26, 2015

gravis commented Mar 26, 2015

sirupsen commented Mar 27, 2015

gravis commented Mar 27, 2015

gravis commented Mar 27, 2015

sirupsen commented Mar 27, 2015

gravis commented Mar 27, 2015

gravis commented Mar 27, 2015

gravis commented Mar 27, 2015

mattbostock commented Mar 28, 2015

gravis commented Apr 9, 2015

tobi commented Apr 9, 2015

gravis commented Apr 9, 2015

sirupsen commented May 8, 2015

gravis commented May 8, 2015

sirupsen commented Oct 5, 2015

gravis commented Oct 5, 2015

sirupsen commented Oct 5, 2015

sirupsen commented Oct 5, 2015

sirupsen commented Oct 5, 2015

gravis commented Oct 5, 2015

gravis commented Oct 5, 2015

sirupsen commented Oct 5, 2015

tobi commented Oct 5, 2015

sirupsen commented Oct 5, 2015

gravis commented Oct 5, 2015

Make airbrake hook non-blocking #157

Make airbrake hook non-blocking #157

Conversation

gravis commented Mar 25, 2015

gravis commented Mar 25, 2015

sirupsen Mar 26, 2015

Choose a reason for hiding this comment

gravis Mar 26, 2015

Choose a reason for hiding this comment

gravis commented Mar 26, 2015

sirupsen commented Mar 26, 2015

gravis commented Mar 26, 2015

gravis commented Mar 26, 2015

sirupsen commented Mar 27, 2015

gravis commented Mar 27, 2015

gravis commented Mar 27, 2015

sirupsen commented Mar 27, 2015

gravis commented Mar 27, 2015

gravis commented Mar 27, 2015

gravis commented Mar 27, 2015

mattbostock commented Mar 28, 2015

gravis commented Apr 9, 2015

tobi commented Apr 9, 2015

gravis commented Apr 9, 2015

sirupsen commented May 8, 2015

gravis commented May 8, 2015

sirupsen commented Oct 5, 2015

gravis commented Oct 5, 2015

sirupsen commented Oct 5, 2015

sirupsen commented Oct 5, 2015

sirupsen commented Oct 5, 2015

gravis commented Oct 5, 2015

gravis commented Oct 5, 2015

sirupsen commented Oct 5, 2015

tobi commented Oct 5, 2015

sirupsen commented Oct 5, 2015

gravis commented Oct 5, 2015