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

Deprecate Message#getFormat() #2773

Merged
merged 2 commits into from
Jul 30, 2024
Merged

Deprecate Message#getFormat() #2773

merged 2 commits into from
Jul 30, 2024

Conversation

vy
Copy link
Member

@vy vy commented Jul 26, 2024

Message#getFormat() doesn't only have unclear semantics, it is also implemented very inconsistently. One might think it should return JSON by messages that want to format themselves in JSON, yet, thanks to many implementations forwarding getFormat() to message (yes, the arbitrary user-provided message!), this is not the case.

In short, it is an incomplete and failed attempt to allow messages to format themselves in one or more encodings. Good news is, we already have a working solution for that: MultiformatMessage.

This PR

  1. deprecates Message#getFormat()
  2. advocates MultiformatMessage

@vy vy added the api Affects the public API label Jul 26, 2024
@vy vy requested a review from ppkarwasz July 26, 2024 18:50
@vy vy self-assigned this Jul 26, 2024
Copy link
Contributor

@ppkarwasz ppkarwasz 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 to me.

There are also other call sites of Message.getFormat() that we could deprecate: the useRawMessage attribute of RegexFilter does not make much sense, since it uses getFormat().

@vy
Copy link
Member Author

vy commented Jul 27, 2024

the useRawMessage attribute of RegexFilter does not make much sense, since it uses getFormat().

This is a difficult one. That is, I can understand the rationale behind useRawMessage:

  1. Efficiency (no need to format parameters)
  2. Avoiding ambiguity – For instance, given logging statements LOGGER.info("foo {}", userProvidedField) and LOGGER.info("foo bar"), how can I only match the latter without using useRawMessage? That is, a malicious user can pass userProvidedField as bar.

In short, useRawMessage has value, but it is implemented incorrectly, IMO. Instead, I think, we should have introduced a RegexFilterableMessage contract and implement it in certain message types. Since this is rather a big undertaking, for the time being, I have improved RegexFilter in 33373f9 to specialize on certain message types. @ppkarwasz, is this okay with you?

@ppkarwasz
Copy link
Contributor

In short, useRawMessage has value, but it is implemented incorrectly, IMO. Instead, I think, we should have introduced a RegexFilterableMessage contract and implement it in certain message types. Since this is rather a big undertaking, for the time being, I have improved RegexFilter in 33373f9 to specialize on certain message types. @ppkarwasz, is this okay with you?

I guess for now it is enough. We might want to add a RegexFilterableMessage (or StringFormatMessage or whatever the appropriate naming is) in the future, but we'd better discuss the new interface on dev@logging first. For the deprecation no discussion is necessary IMHO: the facts speak for themselves, the getFormat() method is not correctly defined.

@vy vy added this to the 2.24.0 milestone Jul 30, 2024
@vy vy merged commit c162a09 into 2.x Jul 30, 2024
9 checks passed
@vy vy deleted the deprecate-Message-getFormat branch July 30, 2024 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants