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 log-formatting to the otel-LogHandler #3673
base: main
Are you sure you want to change the base?
Conversation
…before emitting it as the body, and retain all meaningful attributes of the original logrecord.
I think this is against the semantic conventions. To understand what I mean, see #3675 in otel we have standard semantic conventions for things like |
@bogdandrutu Yeah it is. Scanning over |
It is ok to create an issue, but would be worried merging this PR since then it becomes a breaking change. |
In that case I'll try to find all semantic conventions that exist for the standard logrecord attributes and add them with the right names at least. It might actually be just the |
Ok, I've updated my fork with your changes and suggestions. |
opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py
Outdated
Show resolved
Hide resolved
@bogdandrutu @mariojonke if it's not a bother, I'd like to ask if the PR in its current state could be approved, or if there is something left that I should improve on. Thank you for your time. |
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.
Overall LGTM, I have concerns about the extra attributes being added that don't have sem conv
"args", | ||
"msg", | ||
"message", | ||
"stack_info", | ||
"exc_info", | ||
"exc_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.
I guess we should still discard the attributes like taskName
, asctime
etc.. that don't have the mapping in sematic conventions. Not that they are not useful but we only emit the attributes that sem convs defined in SDK
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 don't have an overly strong opinion on this, but sway towards emit all, or at the very least some of them.
I can't find the source right now, but I think I've read that OTEL implementations should prioritize being written idiomatic within the language the implementation is written in over sticking to details of the general OTEL guidelines. I'd consider log-record attributes to fall into this category. As a python developer, I expect attributes like lineno, module, and funcName to exist on a logrecord.
Also, custom attributes already do get emitted.
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 don't think language idiomatic implementation suggestions should interpreted to associate with the attributes. This is an independent area. We are emitting some of them already by mapping to semconv attributes. And will emit the rest too but that will be after there is some spec guidelines about such cases.
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.
Overall LGTM, question about the excluded attributes
Thank you all for contributing to the fix. Having formatting work properly is really important. Could this change be reviewed and prioritized again? |
Description
Added support for log message formatters to
opentelemetry.sdk._logs._internal.LogHandler
.Fixes #3664
Type of change
added a
self.format
call on the originalLogRecord
to the code in the LogHandler'semit
method, the result is added as thebody
field of the otel-LogRecord
.removed code that filtered out all standard-
LogRecord
fields from getting added to the otel-LogRecord
. The only values that still get filtered are those that are considered "private" by theLogRecord
API (see a note "You shouldn’t need to format this yourself" in https://docs.python.org/3/library/logging.html#logrecord-attributes) or only get used to cache results.Bug fix (non-breaking change which fixes an issue)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update
The documentation should include an example which shows how exactly adding a formatter changes the result of what gets emitted to the receiver. I think there is some element of surprise here, because the standard procedure is that, if a formatter was set, the handler emits a string whose content is wholly dictated by said formatter.
The breaking change is due to me removing a check where a non-string
msg
attribute in the originalLogRecord
will be attached to the otel-LogRecord
as-is. Now, theformat
call will always return a string, even if no formatter was set andargs
is empty. I believe this is the right behavior, makes the codebase easier to maintain. Code that looked like thiscould before this change be interpreted as a complex data structure by some log-renderer. I'd argue that the "right way of logging" (even without the changes in this PR) would be the following:
How Has This Been Tested?
I wrote three new unit tests, and updated a few other ones that were broken after the change.
Does This PR Require a Contrib Repo Change?
Checklist:
Open Discussion Points
opentelemetry.sdk._logs._internal.LogRecord.body
toOptional[str]
now? I believe not as it would violate theLogRecordAPI
, but would like to make sure