Add logrus support

[flimzy]

trafficstars

This PR adapts my free-standing logrus hook (https://gitlab.com/flimzy/logrusentry) for inclusion in this repo. Closes #43

There's a good chance this hook doesn't take advantage of all available/relevant Sentry features. But it's been serving me well for several years now, at at least three different companies. Please advise if there are any functional or stylistic changes that are desired. I'm happy to do what I can to get this included in this repo.

[jeffwidman]

While this would be helpful in the short term for migrating a project from raven-go, in the long term it might be better to hold off on this until there's more clarity on the discussion about adding structured logging to the go std lib.

Because if that turns into a proposal that lands, then I suspect most applications will start migrating toward that or toward logging libraries that extend that. So usage of logrus would probably decline given that it's in maintenance mode so unlikely to be updated to that.

Wouldn't want to add the logrus support for a few months to a year, then have to do a full version bump simply to drop it again. I expect that the logging discussion should result in a decision either way within a few months, so even if that discussion gets rejected then the worst case is it only delays the inclusion of the logrus hook by a few months.

[flimzy]

Wouldn't want to add the logrus support for a few months to a year, then have to do a full version bump simply to drop it again.

IMO, it wouldn't make sense to drop it until logrus is no longer widely used, which will probably be 5-10 years, even if no new projects ever adopt it starting immediately.

[flimzy]

Rebased and force-pushed after recent changes to master.

[kamilogorek]

Wouldn't want to add the logrus support for a few months to a year, then have to do a full version bump simply to drop it again. I expect that the logging discussion should result in a decision either way within a few months, so even if that discussion gets rejected then the worst case is it only delays the inclusion of the logrus hook by a few months.

IMO, it wouldn't make sense to drop it until logrus is no longer widely used, which will probably be 5-10 years, even if no new projects ever adopt it starting immediately.

Agree that it's still fine to support libraries that are and probably will be very widely used for a long time.

If anything, I'd much rather drop it in the future, than not have support for it during that period.

We are also still pre-v1 release, so if it won't gain any traction, we'll always have the opportunity to not support it anymore.

[kamilogorek]

Did the initial pass, but will get back to it once changes are in place.

[flimzy]

Thanks for the review @kamilogorek . I've addressed most of your comments, and added a follow-up question.

[kamilogorek]

Thanks, there's one thing left. You did the reverse of what I'd prefer 😅 Here https://github.com/getsentry/sentry-go/pull/471#discussion_r998070949 instead of holding onto scope/client separately, keep the reference to hub itself, not the other way around.

It's because Hub already has references to its client and scope via hub.Scope() and hub.Client() methods.

[flimzy]

You did the reverse of what I'd prefer sweat_smile

Ha! Oops. Thanks for the explanation. Will follow up soon.

[flimzy]

Rebased and updated to use hub in place of client and scope.

[cleptric]

@flimzy We might want this to be its own module, to not add further deps to the main package.

cc @kamilogorek What do you think?

[flimzy]

We might want this to be its own module, to not add further deps to the main package.

I don't think it adds any deps anyway. The only change to go.mod is a promotion of logrus from an indirect to direct dependency, so won't actually change the deps that any consumer has to download.

[cleptric]

btw, @flimzy test failures are caused by a new option added via https://github.com/getsentry/sentry-go/pull/490

[flimzy]

btw, @flimzy test failures are caused by a new option added via #490

Thanks for the tip. Addressed in 95a2bee9d55132df3fa99b08dfe2b094a2062eb7

[cleptric]

Thanks, LGTM!

About the modules topic: We will start to move our integrations into their own modules soon. While module pruning works well, I rather have a more concise go.mod file for the core SDK.

So my idea was to already do this here. Not a biggie if we take care of this later, just something I wanted to add.

Let's wait for @kamilogorek to give his 👍 and we can merge.

PS: If I'd may ask, it would be awesome to have some basic examples for logrus in the examples folder 🙂

[flimzy]

We will start to move our integrations into their own modules soon

In that case, I have no objection (not that my vote would matter that much anyway :rofl:) if you wish to make this a separate module as well.

PS: If I'd may ask, it would be awesome to have some basic examples for logrus in the examples folder slightly_smiling_face

Sure, I can work on this!

[flimzy]

I've added an example. Feedback welcome. It's hard to know how detailed to be in an example.

[cleptric]

Thanks a lot, looks good to me!

[kamilogorek]

All requests from the previous review were applied, thanks! As for the separate package, I think we can postpone it with logrus, as it's already been our dep before, which you already notices. We can also test it out in the wild, and verify whether we should pull the support to v1 of the SDK once we go for it.

[kamilogorek]

I just noticed that filename is called logrusentry instead of logru*s*sentry haha. But I think it's actually a good wordplay, and it's not affecting the behavior, so we can leave it as is if you want 😅

[flimzy]

Re: logrusentry vs logrussentry, it was an intentional wordplay, but perhaps that violates the principle of least surprise? I can go either way ;) I guess it probably matters very little, as it's just a filename, and not an import name.

[kamilogorek]

Nah, I like the idea as well!