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

Always use LogRecord.getMessage to get the log body #4327

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pR0Ps
Copy link
Contributor

@pR0Ps pR0Ps commented Nov 27, 2024

See context in #4216

This is mostly a revert of #3343

Description

This change makes the log exporting always call LogRecord.getMessage() in order to format and return the record's body text. Previously in #3343 a special case was added so that the record.msg object was used directly if no args were provided. This special case was further scoped down and changed to call record.getMessage() to improve compatibility with other Python logging libraries in #4216 . This PR proposes doing away with the special case entirely and always calling record.getMessage() in order to get the body text.

The reason for preserving the original type of the record.msg given in #3343 was that the body field type in the OTel 1.22.0 Logs Data Model is supposed to be "any", not "str", so the record.msg should not be converted to a string before being set as the body.

However, this goes against the Python documentation that states that the record.msg is only supposed to be a format string[1] or an object to convert into a format string[2] and not an actual object to be logged.

[1]: From logging.Logger.debug(msg, *args, **kwargs) docs (emphasis mine):

The msg is the message format string, and the args are the arguments which are merged into msg using the string formatting operator. [...] No % formatting operation is performed on msg when no args are supplied.

[2]: From logging.LogRecord.getMessage() docs (emphasis mine):

If the user-supplied message argument to the logging call is not a string, str() is called on it to convert it to a string. This allows use of user-defined classes as messages, whose __str__ method can return the actual format string to be used.

Note that while the "Using arbitrary objects as messages" section of the logging HOWTO states:

You can pass an arbitrary object as a message, and its __str__() method will be called when the logging system needs to convert it to a string representation.

this doesn't mean that the msg itself should be an object to log out. The "when the logging system needs to convert it to a string" part happens when the LogRecord is processed by a handler, applies the logic in [2] and converts the msg to a format string that it then formats the record.args into (if any).

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

I've updated and run the unit tests, as well as run manual tests of the functionality (see #4216 for details)

Does This PR Require a Contrib Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@pR0Ps pR0Ps requested a review from a team as a code owner November 27, 2024 06:15
@pR0Ps pR0Ps force-pushed the bugfix/bracelogger-compatibility-alt branch from 5171c81 to 33d48a4 Compare December 6, 2024 03:00
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.

1 participant