feat(loguru): Sentry logs for Loguru #4445
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.
These were just moved to this file from tests/test_logs.py
There was a problem hiding this comment.
thanks for letting me know, not checking them further in that case
|
Additional change: removed setting |
There was a problem hiding this comment.
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.
[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.
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.
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.
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.
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.
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 <7881302+szokeasaurusrex@users.noreply.github.com>
| (30, 13, "warn"), | ||
| (20, 9, "info"), | ||
| (10, 5, "debug"), | ||
| (5, 1, "trace"), |
There was a problem hiding this comment.
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.
[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.
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.
If you take my suggestion about ranges
| for (lower, upper), severity in O 10000 TEL_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_37from some tests that don't need 3.7+._LoguruBaseHandler)Closes #4151