-
-
Notifications
You must be signed in to change notification settings - Fork 523
implement stdlib logger support #2653
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
Conversation
sl0thentr0py
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a small comment, will leave a proper review to @solnic
|
|
||
| def add(severity, message = nil, progname = nil, &block) | ||
|
|
||
| return unless Sentry.initialized? && Sentry.get_current_hub |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need a super here and below since the original logger should not be affected
solnic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR! I've left some initial comments. Please also ensure that rubocop passes against the files you added and changed because there are issues with formatting 🙂
| self.traces_sampler = nil | ||
| self.enable_tracing = nil | ||
| self.enable_logs = false | ||
| self.send_stdlib_logs = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed, there's a config option called enabled_patches that should be used instead. The Logger patch should be registered as :logger and then the users will be able to configure it to be enabled by default.
You can see how :http patch is done for reference.
| return unless Sentry.initialized? && Sentry.get_current_hub | ||
|
|
||
| config = Sentry.configuration | ||
| return unless config.enable_logs && config.send_stdlib_logs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This guard won't be needed when you add patching based on our enabled_patches option like I mentioned in the other comment. When registering your patch via register_patch you'll be able to do something like:
Sentry.register_patch(:logger, Sentry::StdLibLogger, ::Logger) do |config|
if config.enable_logs
::Logger.prepend(Sentry::StdLibLogger)
else
Sentry.sdk_logger.warn(":logger patch enabled but `enable_logs` is turned off - skipping applying patch")
end
end| if !message.nil? && message != Sentry::Logger::PROGNAME && method = SEVERITY_MAP[severity] | ||
| Sentry.logger.send(method, message) | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this method should always call super and logs to Sentry as an additional side effect.
@sl0thentr0py wouldn't you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait nevermind @sl0thentr0py - I just noticed that you pointed it out as well above, sorry for the noise!
all comments have been addressed and PR updated. @solnic @sl0thentr0py |
|
Thanks for the PR and for following up on the reviews! 🙇🏻 I've opened a new PR that builds on top of yours because of time constraints, as we need to finish this feature this week and we may need more back and worth in the review process. See #2657 for more progress! Gonna close this one because of this, I hope that's OK! All your commits are in my branch of course :) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2653 +/- ##
==========================================
+ Coverage 96.98% 97.44% +0.45%
==========================================
Files 133 134 +1
Lines 5147 5167 +20
==========================================
+ Hits 4992 5035 +43
+ Misses 155 132 -23
🚀 New features to boost your workflow:
|
Totally fine! |
|
@Eazybright thanks again for helping with this. I just merged my PR with your work included. We're gonna ship it in the next release 🙂 |
Great work @solnic Thanks for the review, i learnt from it 🙏 |
|
thanks for the contribution @Eazybright! if you want a free Sentry t-shirt, send me an email on |
Will do, thanks! |
Description
Describe your changes:
This is a fun way of adding integration for stdlib Logger so that any usage of any logger instance would result in sending logs to Sentry too. issue
Sample Config
Sample Result on the UI