Skip to content
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 loguru integration #1994

Merged
merged 14 commits into from
May 9, 2023
Merged

Add loguru integration #1994

merged 14 commits into from
May 9, 2023

Conversation

PerchunPak
Copy link
Contributor

@PerchunPak PerchunPak commented Apr 8, 2023

Actually, this is the solution in comments under #653 adapted to codebase and tested as well.
#653 (comment)

I also changed logging integration to use methods instead of functions in handlers, as in that way we can easily overwrite parts that are different in loguru integration. It shouldn't be a problem, as those methods are private and used only in that file.

Please let me know if I missed something, or you have more ideas how to test this.

Fixes #653.

Actually, this is the solution in comments under #653 adapted to
codebase and tested as well.
#653 (comment)

I also changed `logging` integration to use methods instead of
functions in handlers, as in that way we can easily overwrite parts
that are different in `loguru` integration. It shouldn't be a problem,
as those methods are private and used only in that file.

Please let me know if I missed something, or you have more ideas how to
test this.
So those will not be referenced in docs
@shanamatthews
Copy link

@antonpirker - want to take a look at this?

@antonpirker
Copy link
Member

I am a bit swamped right now, so this could take some time.

@antonpirker antonpirker self-assigned this Apr 17, 2023
@PerchunPak
Copy link
Contributor Author

I do not really understand the error. This doesn't look like my problem.

@antonpirker
Copy link
Member

Sometimes our tests are a bit flaky. I have updated your branch with the current master, so the tests will run again.

@antonpirker
Copy link
Member

Hey @PerchunPak

I finally had some time to play with this integration. Nice work!

I create a small test project for testing:

from loguru import logger as loguru_logger

import sentry_sdk
import sentry_sdk.integrations.loguru
sentry_sdk.init(
    dsn="...",
    integrations=[
        sentry_sdk.integrations.loguru.LoguruIntegration(),
    ],
    traces_sample_rate=1.0, 
    debug=True,
)

def main():
    loguru_logger.debug("Guru: I am ignored")
    loguru_logger.info("Guru: I am a breadcrumb")
    loguru_logger.error("Guru: I am an event", extra=dict(bar=43))
    loguru_logger.exception("Guru: An exception happened")    


if __name__ == "__main__":
    main()

So the error log entry creates an "information like" event in Sentry and the exception log entry creates an exception in Sentry. Very nice!

Screenshot 2023-04-26 at 12 03 54

The error message contains all the formatted Loguru message with all the information. This is not the best developer experience. Could you please change this, to only contain the message from the call to "loguru_logger.exception()"?

One other thing I noticed is, that loguru_logger.exception() does not attach an exception object, so this code is never reached: https://github.com/getsentry/sentry-python/blob/master/sentry_sdk/integrations/logging.py#L209-L213

Is this just how Loguru works, or am I doing something wrong?

@PerchunPak
Copy link
Contributor Author

PerchunPak commented Apr 26, 2023

The error message contains all the formatted Loguru message with all the information. This is not the best developer experience. Could you please change this, to only contain the message from the call to "loguru_logger.exception()"?

Unfortunately, looks like no. Loguru parses the message before it is passed to our handler, so we get only a formatted message. We can somehow cut unnecessary parts (e.g. regex or build the unnecessary parts of message by ourselves to cut it) or we can change the format of messages to contain only plain messages (loguru.add(..., format="{message}")). The second way will make it impossible to include loguru's data into messages.

image

Is this just how Loguru works, or am I doing something wrong?

Both, you don't catch any exception, so loguru doesn't provide it. This code works, and also doesn't send formatted messages by loguru in the report name.

try:
    1 / 0
except ZeroDivisionError:
    loguru_logger.exception("Guru: An exception happened")

If there is no exception, loguru prints None to the console (is it a bug?)

>>> from loguru import logger
>>> logger.exception("Guru: An exception happened") 
2023-04-26 21:29:02.780 | ERROR    | __main__:<module>:1 - Guru: An exception happened
NoneType: None

@antonpirker
Copy link
Member

or we can change the format of messages to contain only plain messages (loguru.add(..., format="{message}")).

We should not do this. We should never change the behavior of the users code, just add functionality on top of it.
As the users can set whatever formatting that she likes the regex approach seems not possible.

Can we "build the unnecessary parts of message" with the formatting the users has set so it always works, no matter what the configuration of the user looks like?

@antonpirker
Copy link
Member

Or one other option: can we hook into something that is run before our handler, where we have access to the raw message?

@PerchunPak
Copy link
Contributor Author

loguru works different from logging module. In logging, you can set the format of message globally, while in loguru - every handler can have a customized message (handler added with logger.add). We can't get the user's message, because the user can configure multiple loggers, that will output to multiple terminals/files. E.g. the user wants to have logs in files too, so they will just do

loguru.add("logs.txt", format="some custom format")  # and maybe some rotation, doesn't matter

So if we will monkeypatch loguru's global handler - we will get duplicates because

logger.info("hi!")
# calls all handlers:
# 0 - default (stderr logger)
# 1 - what we added upper (log into file)
# 2 - sentry's breadcrumb handler   # only if INFO >= logging_level > ERROR
# 3 - sentry's event handler        # only if ERROR >= logging_level

All of them can have custom format, so we can't catch what the user actually sees in the terminal. BUT we can provide configuration options that we will send into loguru.


Also, as I understand, all info that provides loguru in the console, Sentry provides too in the UI.

>>> import sentry_sdk
... import sentry_sdk.integrations.loguru
... sentry_sdk.init(
...     dsn="...",
...     integrations=[
...         sentry_sdk.integrations.loguru.LoguruIntegration(),
...     ],
...     traces_sample_rate=1.0,
...     debug=True,
... )
 [sentry] DEBUG: Setting up integrations (with default = True)
 ...
>>> from loguru import logger
>>> import sys
>>> logger.add(sys.stdout, format="{time} {level} {message}")
>>> logger.info("hi")
2023-04-28T09:02:25.169723+0200 INFO hi
2023-04-28 09:02:25.169 | INFO     | __main__:<module>:1 - hi
>>> logger.error("test")
2023-04-28 09:02:36.507 | ERROR    | __main__:<module>:1 - test
 [sentry] DEBUG: Sending envelope [envelope with 1 items (error)] project:... host:....ingest.sentry.io
2023-04-28T09:02:36.507423+0200 ERROR test
 [sentry] DEBUG: [Tracing] Adding `sentry-trace` header ...-...- to outgoing request to https://....ingest.sentry.io/api/.../envelope/.

image


can we hook into something that is run before our handler, where we have access to the raw message?

I did this via appending this code to the end of our loguru integration module

import loguru._simple_sinks

class PatchedStandardSink(loguru._simple_sinks.StandardSink):
    def write(self, message):
        parsed_record = None
        actual_message = message.record["message"]

        def handler_hande(record):
            nonlocal parsed_record
            parsed_record = record

        original_handler_handle = self._handler.handle
        self._handler.handle = handler_hande
        original_write(self, message)

        parsed_record.message = actual_message
        parsed_record.msg = actual_message

        original_handler_handle(parsed_record)


original_write = loguru._simple_sinks.StandardSink.write
loguru._simple_sinks.StandardSink.write = PatchedStandardSink.write

But this gives absolutely the same result as just setting format="{message}".

image

Copy link
Member

@antonpirker antonpirker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. It does what it should and the mentioned problems we can not fix because of the nature of loguru, so this is good to be merged.

@antonpirker antonpirker merged commit 8a2b74f into getsentry:master May 9, 2023
244 checks passed
@antonpirker
Copy link
Member

Hey @PerchunPak !

Again: Great work, thanks again for this contribution!

We now need also some documentation for this. Could you please create a PR in our docs repo. You can just copy the Redis docs and change it to fit Loguru: https://github.com/getsentry/sentry-docs/blob/master/src/platforms/python/common/configuration/integrations/redis.mdx#L19

Thanks!

And if you want to have e small thank you gift please send me your shipping address and t-shirt size to anton.pirker@sentry.io

@PerchunPak
Copy link
Contributor Author

Thank you for the merge!

We now need also some documentation for this. Could you please create a PR in our docs repo.

It is already done here.

@antonpirker
Copy link
Member

Hey @PerchunPak !

Your contribution has been released: https://github.com/getsentry/sentry-python/releases/tag/1.23.0
Congrats!

If you want a little thank you for your contribution please send me your shipping address (and tshirt size) to anton.pirker@sentry.io!

PerchunPak added a commit to PerchunPak/2nd-circle-updates that referenced this pull request May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support of loguru logger.
3 participants