-
Notifications
You must be signed in to change notification settings - Fork 550
feat(loguru): Sentry logs for Loguru #4445
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
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #4445 +/- ##
==========================================
- Coverage 80.68% 80.60% -0.08%
==========================================
Files 142 142
Lines 15986 16030 +44
Branches 2732 2745 +13
==========================================
+ Hits 12898 12921 +23
- Misses 2231 2243 +12
- Partials 857 866 +9
|
@@ -283,3 +286,197 @@ def test_logging_dictionary_args(sentry_init, capture_events): | |||
== "the value of foo is bar, and the value of bar is baz" | |||
) | |||
assert event["logentry"]["params"] == {"foo": "bar", "bar": "baz"} | |||
|
|||
|
|||
def test_sentry_logs_warning(sentry_init, capture_envelopes): |
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.
These were just moved to this file from tests/test_logs.py
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.
thanks for letting me know, not checking them further in that case
Additional change: removed setting |
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.
Looks mostly good, I did have a PII concern though which is why I am not approving yet (please see the inline comment which mentions this, in the loguru.py
file).
The other comments are just minor suggestions/nits which you can feel free to ignore, once the PII thing is addressed I will approve.
sentry_sdk/integrations/loguru.py
Outdated
for py_level, otel_severity_number, otel_severity_text in [ | ||
(LoggingLevels.CRITICAL, 21, "fatal"), | ||
(LoggingLevels.ERROR, 17, "error"), | ||
(LoggingLevels.WARNING, 13, "warn"), | ||
(LoggingLevels.SUCCESS, 11, "info"), | ||
(LoggingLevels.INFO, 9, "info"), | ||
(LoggingLevels.DEBUG, 5, "debug"), | ||
(LoggingLevels.TRACE, 1, "trace"), | ||
]: | ||
if record_level >= py_level: | ||
return otel_severity_number, otel_severity_text | ||
|
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.
[suggestion for future PR] might be worth extracting this logic into a separate funciton, especially if we expect to add more logging integrations that would use it; seems to be verbatim repeated from the logging
integration.
We may also or alternatively want to make constants for the OTel severity numbers, since these are hardcoded in both locations (this can also be done in separate PR)
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.
It's not exactly the same as in the logging
integration, Loguru has two additional logging levels (SUCCESS and TRACE iirc). But it would definitely be nice to have the logic in a separate function and only define the mapping itself in each integration. Will do this.
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.
The brunt of the changes is in this commit
mypy is unhappy, fixing that separately
@@ -283,3 +286,197 @@ def test_logging_dictionary_args(sentry_init, capture_events): | |||
== "the value of foo is bar, and the value of bar is baz" | |||
) | |||
assert event["logentry"]["params"] == {"foo": "bar", "bar": "baz"} | |||
|
|||
|
|||
def test_sentry_logs_warning(sentry_init, capture_envelopes): |
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.
thanks for letting me know, not checking them further in that case
@@ -54,7 +59,7 @@ def test_just_log( | |||
formatted_message = ( | |||
" | " | |||
+ "{:9}".format(level.name.upper()) | |||
+ "| tests.integrations.loguru.test_loguru:test_just_log:52 - test" | |||
+ "| tests.integrations.loguru.test_loguru:test_just_log:57 - test" |
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.
It's kinda gonna be a pain to maintain this if we might add/remove imports in the future. Perhaps we could just check that the value here is an int
? Or maybe there is some fancy introspection logic we could use to compute the line?
We can also address this in a separate PR, I guess it's not exactly in scope here to fix
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 didn't add this, it was already there -- agree with you on it being annoying, but not in scope. We can improve this some other time.
Co-authored-by: Daniel Szoke <[email protected]>
(30, 13, "warn"), | ||
(20, 9, "info"), | ||
(10, 5, "debug"), | ||
(5, 1, "trace"), |
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 (5, 1, "trace")
is now missing from the new mapping in SEVERITY_TO_OTEL_SEVERITY
. There is no level 5
in the stdlib logging library
@@ -6,6 +6,16 @@ | |||
from sentry_sdk import get_client | |||
from sentry_sdk.utils import safe_repr | |||
|
|||
OTEL_RANGES = [ | |||
# ((severity level range), severity text) | |||
((1, 4), "trace"), |
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.
[optional suggestion] This is completely fine as is, but if you want, you could make all of these into ranges, then rather than manually checking if the severity number is in between the bounds, you would just check if it is in the range.
((1, 4), "trace"), | |
(range(1, 5), "trace"), |
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'd prefer to keep as is, since that way the ranges correspond to OTel levels exactly as they're written in code (i.e., you don't need to make the extra effort in your head to decrement the last number of each range by one to arrive at the actual range)
for (lower, upper), severity in OTEL_RANGES: | ||
if lower <= otel_severity_number <= upper: | ||
return severity |
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.
If you take my suggestion about ranges
for (lower, upper), severity in OTEL_RANGES: | |
if lower <= otel_severity_number <= upper: | |
return severity | |
for severity_range, severity in OTEL_RANGES: | |
if otel_severity_number in severity_range: | |
return severity |
lgtm! maybe check the codecov/patch failure before merging to see if test coverage should be expanded |
Added a small test, will merge now |
Allow to send Loguru logs to Sentry.
We can't parametrize them nicely, but this is a good first step.
Also:
@minimum_python_37
from some tests that don't need 3.7+._LoguruBaseHandler
)Closes #4151