New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add logrus support #471
Add logrus support #471
Conversation
While this would be helpful in the short term for migrating a project from 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 Wouldn't want to add the |
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. |
Rebased and force-pushed after recent changes to master. |
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. |
Did the initial pass, but will get back to it once changes are in place. |
Thanks for the review @kamilogorek . I've addressed most of your comments, and added a follow-up question. |
Thanks, there's one thing left. You did the reverse of what I'd prefer 😅 It's because |
Ha! Oops. Thanks for the explanation. Will follow up soon. |
…with Go conventions
Rebased and updated to use |
@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? |
I don't think it adds any deps anyway. The only change to |
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 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 |
In that case, I have no objection (not that my vote would matter that much anyway 🤣) if you wish to make this a separate module as well.
Sure, I can work on this! |
I've added an example. Feedback welcome. It's hard to know how detailed to be in an example. |
Thanks a lot, looks good to me! |
All requests from the previous review were applied, thanks! |
I just noticed that filename is called |
Re: |
Nah, I like the idea as well! |
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.