logrus_sentry

logrus_sentry copied to clipboard

When enabling stack traces, logrus_sentry sets

packet.Culprit = err.Error()

On line 140.

This changes the default behaviour, where the logged error message is the "title" in the Sentry dashboard, to instead making the error into the title.

I suggest retaining the default behaviour, whether the uses wants to include a stack trace or not.

Available options;

• a) Add an flag into hook struct and if it is true then override packet.Culprit
• b) Add special field like culprit and if it is set then override packet.Culprit
• c) Set packet.Culprit when entry.Message is empty string

@belak Any thoughts?

I'm not particularly sure what the best solution would be... my main problem has been that all (unrelated) errors are getting logged (and grouped) under the place where they're logged, rather than by the error itself... And I'm not sure how much of that is because we're stuck on a super old version of sentry and how much is the library. We're part-way through that transition, but I'm afraid my comments won't be as useful until then.

At least for my use case, the optimal method would be to group by the Message, and then by err.Error() but I'm not sure how to accomplish that.

Sorry, that rambled a bit more than I wanted but I think you get the general idea.

I think I'm in favor of option #C.

Using err.Error() has made our logs much harder for us mere humans to read quickly.

In the end, though, I think this is a bug/misfeature in Sentry, not in this library.

Upon further reading, it does seem that logrus_sentry is improperly assigning culprit. It's intended to be a function name, not human readable text. See here.

Even so, that doesn't address the underlying issue; logrus_sentry could easily set it to the function which errored, and it would still override the 'message' as the page title.

I did this a while back and we're (currently) using this internally: https://github.com/belak/logrus_sentry/commit/060021c4a4a2cd92107a6674c272e4af3850473b

It seems to work pretty well because (at least for us) we use static messages and store all changing information in the fields... however this has the disadvantage that when looking at logs without the fields, they're pretty worthless.

Indeed, any change to this functionality will likely break someone. I propose that any change is done as an option, with the existing ("broken") behavior as the default.