evalphobia / logrus_sentry

sentry hook for logrus
MIT License
194 stars 78 forks source link

Stacktrace the culprit should be the received error not the cause #45

Closed bradleyfalzon closed 7 years ago

bradleyfalzon commented 7 years ago

Previously, when stacktrace was enabled, the culprit would be set to the first error that did not implement Cause (as detected by errors.Cause(err)), this had the affect of not showing any additional context added to the error.

This was also inconsistent with the same behaviour when stacktrace was disabled.

This change attempts to unify that behaviour, as well as to not discard the context added to the errors.

Example program (snippet):

// url is being set to https://www.google.com.au
func (p *Poller) Poll(ctx context.Context, url string) {
        ticker := time.NewTicker(time.Second)
        defer ticker.Stop()

        for {
                select {
                case <-ctx.Done():
                        return
                case <-ticker.C:
                        err := poll(ctx, url)
                        if err != nil {
                                p.logger.WithError(err).Error("could not poll url")
                        }
                }
        }
}

func poll(ctx context.Context, url string) error {
        err := io.EOF
        return errors.Wrapf(err, "could not poll %v", url)
}

Current behaviour with Stacktrace enabled (notice no could not poll %v - we've lost the context):

screen shot 2017-04-21 at 3 36 29 pm

Current behaviour without Stacktrace enabled (we have our context):

screen shot 2017-04-21 at 3 39 17 pm

Proposed behaviour (but this still doesn't unify the behaviour with or without stack trace):

screen shot 2017-04-21 at 3 40 38 pm

Alternative (by removing err := errors.Cause(err) completely):

screen shot 2017-04-21 at 3 42 35 pm

So, I'm happier with the proposed behaviour, as I now have the context around the error again, but it's still not the same as the behaviour with or without stacktrace being enabled. Thoughts, do they need to be the same? Does this break Sentry's ability to group errors if the context changes slightly but the root cause is the same?

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.04%) to 83.702% when pulling dd6096a85398c4dbba7ed2f1eac67b79c5da1ba0 on bradleyfalzon:stack-culprit into 7c1b7faba7ed5772effc03db8e1faa8c90624660 on evalphobia:master.

evalphobia commented 7 years ago

@bradleyfalzon Sorry for my lateness. I don't use StackTraceConfiguration.Enable feature personally. But it's look good, so if no one oppose to it, I'll merge this.

bradleyfalzon commented 7 years ago

Thanks, I'll rebase in the next couple days (not near that branch atm).

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.05%) to 83.056% when pulling 22063753ec1cc64e93e2e589eae03a979bf5d998 on bradleyfalzon:stack-culprit into 9f8f2d05a621e616d9341ac75374eb8402ae0630 on evalphobia:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.05%) to 83.056% when pulling f8f4ac0ed0bd462946f7974b82a15849193b0b85 on bradleyfalzon:stack-culprit into 9f8f2d05a621e616d9341ac75374eb8402ae0630 on evalphobia:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.05%) to 83.056% when pulling f8f4ac0ed0bd462946f7974b82a15849193b0b85 on bradleyfalzon:stack-culprit into 9f8f2d05a621e616d9341ac75374eb8402ae0630 on evalphobia:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.05%) to 83.056% when pulling f8f4ac0ed0bd462946f7974b82a15849193b0b85 on bradleyfalzon:stack-culprit into 9f8f2d05a621e616d9341ac75374eb8402ae0630 on evalphobia:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.05%) to 83.056% when pulling 75d620cb7c37944f16e55e30e2992ceb5c57eed5 on bradleyfalzon:stack-culprit into 9f8f2d05a621e616d9341ac75374eb8402ae0630 on evalphobia:master.

bradleyfalzon commented 7 years ago

Sorry for the noise, rebased and ready for review.

evalphobia commented 7 years ago

@bradleyfalzon Thank you for your contribution 🥇 (and sorry for forgetting to merge this :persevere:)

Note: https://github.com/evalphobia/logrus_sentry/releases/tag/v0.4.2