-
Notifications
You must be signed in to change notification settings - Fork 550
fix(logging): Strip log record.name
for more robust matching
#4411
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
fix(logging): Strip log record.name
for more robust matching
#4411
Conversation
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 seems like a fair thing to do.
However, I would still recommend you avoid modifying the LogRecord
's name
in the formatter for the best experience when using Sentry. The (unstripped) name gets sent to Sentry, and we use it in some other conditional checks which you have not updated in this PR. I have never written a logging formatter before, but my guess is that it is probably best practice to not mutate the log record at all in the formatter, and instead just read the attributes from it and format it directly into a string.
The PR looks good to me though, I will just have another member of the team review before we merge.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4411 +/- ##
=======================================
Coverage 80.68% 80.68%
=======================================
Files 142 142
Lines 15986 15986
Branches 2732 2732
=======================================
+ Hits 12898 12899 +1
Misses 2231 2231
+ Partials 857 856 -1
|
Thank you for your review!
Agreed! Once we identified the issue, we reached a similar conclusion that treating the record name as immutable was a much better practice, both for the Sentry integration and overall, and we have since changed the formatter accordingly. Still, if we have fallen into that trap, maybe others will too. I see that a couple tests are failing in CI checks, apparently all due to a |
Thanks for the PR @romaingd-spi! The test failures were unrelated and should now be fixed on master. I've merged master into your branch so we should be green. If that's the case we'll merge! |
Hi! In the Python SDK, more specifically
sentry_sdk/integrations/logging.py
, a couple of loggers are ignored to avoid recursion errors.Log records from these loggers are discarded, using an exact match on
record.name
. Unforunately, this breaks if the user modifiesrecord.name
, e.g. for formatting, which is what we were doing for log display (before becoming aware that Sentry relied on it after investigating an infinite recursion issue).As you can see,
record.name
is right-padded with blank spaces. We have found a workaround since, but given that it has taken us quite some time to find the issue, I thought that maybe it could affect others. This PR proposes matchingrecord.name.strip()
instead for increased robustness.