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

use structlog's get_logger if it is available #591

Merged

Conversation

beniwohli
Copy link
Contributor

If the user has set up structlog, this will add some additional context
to each log line, like level and timestamp. E.g.

{"event": "Read traceparent header 00-42d5718b9ac8289e439a688afb17cbae-d6f9a1dad4c90dec-01"}

becomes

{"event": "Read traceparent header 00-42d5718b9ac8289e439a688afb17cbae-d6f9a1dad4c90dec-01",
"timestamp": "2019-09-17T15:37:00.049627Z", "logger": "elasticapm.traces", "level": "debug"}

This can be overridden by setting an environment variable
ELASTIC_APM_DISABLE_STRUCTLOG to "true".

If the user has set up structlog, this will add some additional context
to each log line, like level and timestamp. E.g.

    {"event": "Read traceparent header 00-42d5718b9ac8289e439a688afb17cbae-d6f9a1dad4c90dec-01"}

becomes

    {"event": "Read traceparent header 00-42d5718b9ac8289e439a688afb17cbae-d6f9a1dad4c90dec-01",
    "timestamp": "2019-09-17T15:37:00.049627Z", "logger": "elasticapm.traces", "level": "debug"}

This can be overridden by setting an environment variable
`ELASTIC_APM_DISABLE_STRUCTLOG` to "true".
Copy link
Contributor

@basepi basepi left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me. Definitely needs to be documented, though.

Is there any way we can discover whether they are actually using structlog, as opposed to just have it installed? Having this on by default is definitely the user-friendly behavior (if they have it installed, they're probably using it) but having it off by default is probably the "safer" option.

@basepi
Copy link
Contributor

basepi commented Sep 24, 2019

I'm seriously considering flip-flopping on this again (after all my work on the log correlation testing).

On the one hand, off by default is safer. But if they have structlog installed it's not like they'll be surprised to see different style log messages from us. And if I'm doing structured logging it would sure be convenient for the apm agent to switch automatically.

Basically, what I'm trying to say is I am fine either way on this, but honestly I'm leaning toward your first implementation, on by default. ¯_(ツ)_/¯

@beniwohli
Copy link
Contributor Author

@basepi as a compromise, we could check structlog.is_configured(), but I'm not a terribly huge fan of that approach, because then the behavior becomes dependent of when exactly elasticapm is imported first. It would only use structlog if it is configured before elasticapm is imported. Or even worse, some of the loggers could use structlog, while others wouldn't, depending on when exactly each module was first imported

@basepi
Copy link
Contributor

basepi commented Sep 24, 2019

Yeah that seems like the worst of both worlds. What's the quote? "A good compromise is when both parties are dissatisfied"? ;)

@basepi basepi merged commit 7337ab1 into elastic:master Oct 2, 2019
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.

None yet

2 participants