Always use LogRecord.getMessage to get the log body#4372
Always use LogRecord.getMessage to get the log body#4372pR0Ps wants to merge 3 commits intoopen-telemetry:mainfrom
LogRecord.getMessage to get the log body#4372Conversation
pmcollins
left a comment
There was a problem hiding this comment.
Thanks for doing this. I'm in favor of this change, but because it's breaking, it probably warrants a discussion in the weekly SIG meeting. @pR0Ps not sure if you attend these already, but you are welcome to join if you are interested.
MikeGoldsmith
left a comment
There was a problem hiding this comment.
Changes look good. I agree with @pmcollins the changelog needs to indicate this is a breaking change.
PS the description references the core PR (#4327) for context, but that PR itself links to additional background that's useful for reviewers:
open-telemetry/opentelemetry-python#4216— the original issue and discussion that motivated this changeopen-telemetry/opentelemetry-python#3343— the PR this is mostly reverting
Probably worth adding those directly to the description here so reviewers don't have to chase the chain 😄
|
Has this been discussed at a SIG call? I agree this probably the right thing to do, but would like other @open-telemetry/python-approvers to give input as it's a breaking change. |
Hey @MikeGoldsmith, ran out of time last week, but hope to discuss tomorrow. |
pmcollins
left a comment
There was a problem hiding this comment.
Hi -- discussed this in the SIG meeting and folks seem to be in favor of this change. The fact that it has been moved out of the SDK give us some additional flexibility. Added a suggestion about the changelog entry.
Description
This change makes the log exporting always call LogRecord.getMessage() in order to format and return the record's body text. This is a port of open-telemetry/opentelemetry-python#4327 to this new repo. See that pull request for more details and context.
Type of change
How Has This Been Tested?
I've updated and run the unit tests, as well as run manual tests of the functionality (see open-telemetry/opentelemetry-python#4216 for details)
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.