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.

Sign up for GitHub

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

Jump to bottom

Catch an exception due to incorrect pattern in Strings.format #87132

Merged
merged 4 commits into from
May 30, 2022
Merged

Catch an exception due to incorrect pattern in Strings.format #87132

merged 4 commits into from
May 30, 2022

Conversation

pgomulka
Copy link
Contributor

Strings.format method, which is used heavily in logging with
Supplier should handle exceptions when a format is incorrect.
This will prevent a hard to catch mistakes to blow up in server.
Those mistakes are especially hard to detect in logging when a
code to create a message might be only executed when logger is debug
or trace. Which is not always the case in CI.

relates #87077 (comment)

relates #86549

  • Have you signed the contributor license agreement?
  • Have you followed the contributor guidelines?
  • If submitting code, have you built your formula locally prior to submission with gradle check?
  • If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
  • If submitting code, have you checked that your submission is for an OS and architecture that we support?
  • If you are submitting this code for a class then read our policy for that.

@pgomulka
Catch an exception when formatting a string fails
4662eb4 
Strings.format method, which is used heavily in logging with
Supplier<String> should handle exceptions when a format is incorrect.
This will prevent a hard to catch mistakes to blow up in server.
Those mistakes are especially hard to detect in logging when a
code to create a message might be only executed when logger is debug
or trace. Which is not always the case in CI.

relates elastic#87077 (comment)

relates elastic#86549
@pgomulka pgomulka added >enhancement :Core/Infra/Logging Log management and logging utilities v8.4.0 labels May 25, 2022
@pgomulka pgomulka self-assigned this May 25, 2022
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label May 25, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticsearchmachine
Copy link
Collaborator

Hi @pgomulka, I've created a changelog YAML for you.

@pgomulka pgomulka changed the title Catch an exception when formatting a string fails Catch an exception due to incorrect pattern in Strings.format May 25, 2022
rjernst
rjernst reviewed May 25, 2022
View reviewed changes
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

A couple suggestions

libs/core/src/main/java/org/elasticsearch/core/Strings.java Outdated
return String.format(Locale.ROOT, format, args);
try {
return String.format(Locale.ROOT, format, args);
} catch (Throwable e) {
Copy link
Member

Choose a reason for hiding this comment

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

Why throwable? This should probably be just Exception. Let OOM/etc propagate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exception is definitely better

libs/core/src/main/java/org/elasticsearch/core/Strings.java
try {
return String.format(Locale.ROOT, format, args);
} catch (Throwable e) {
return format;
Copy link
Member

Choose a reason for hiding this comment

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

Can we not fail silently? We should somehow capture the actual problem, embedding it in the message. Also, add an assert here so that if we hit this in tests we can fail the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how should we capture it? Maybe we should log it (with additional Strings's logger)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, logging sounds good.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, not sure we can do logging, because this is in the core lib? It doesn't have any deps. Perhaps we could embed it in the message?

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds like a great idea, return "Error processing logging message: " + format, then the logger will log it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure.. This would be a good solution if we only use this in logging (we do, but the method is in core with no indication that this is only for logging).
I wonder if just returning the original pattern is not good enough. This is what log4j is doing when a pattern has more place holders.
I wonder if we could rather just try to use some static analysis to detect this
https://rules.sonarsource.com/java/RSPEC-2275
this looks promising but would have to be adopted to our needs

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm OK with leaving it as is if it matches what log4j does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I raised an issue to look for static analysis solution this #87166

grcevski
grcevski approved these changes May 26, 2022
View reviewed changes
Copy link
Contributor

@grcevski grcevski left a comment

Choose a reason for hiding this comment

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

LGTM!

libs/core/src/main/java/org/elasticsearch/core/Strings.java
try {
return String.format(Locale.ROOT, format, args);
} catch (Throwable e) {
return format;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm OK with leaving it as is if it matches what log4j does.

@pgomulka pgomulka requested a review from rjernst May 27, 2022 14:21
rjernst
rjernst approved these changes May 27, 2022
View reviewed changes
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM, we can iterate in the future.

@pgomulka pgomulka merged commit 416a1b3 into elastic:master May 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grcevski grcevski grcevski approved these changes

@rjernst rjernst rjernst approved these changes

Assignees
No one assigned
Labels
:Core/Infra/Logging Log management and logging utilities >enhancement Team:Core/Infra Meta label for core/infra team v8.4.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pgomulka @elasticmachine @elasticsearchmachine @rjernst @grcevski