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

Remove usage of ParameterizedMessage in server #86549

Closed
ChrisHegarty opened this issue May 9, 2022 · 2 comments
Closed

Remove usage of ParameterizedMessage in server #86549

ChrisHegarty opened this issue May 9, 2022 · 2 comments
Labels
:Core/Infra/Logging Log management and logging utilities Meta Team:Core/Infra Meta label for core/infra team

Comments

@ChrisHegarty
Copy link
Contributor

ChrisHegarty commented May 9, 2022 •
edited by pgomulka
Loading

A common use of ParameterizedMessage in the ES codebase is to facilitate lazy evaluation of message arguments. ParameterizedMessage predates some of the newer lambda shaped Supplier-accepting log variants. Quite often using a String or argument Supplier is just as good, more readably, and likely slightly more efficient.

Within server there are the following usages:

$ find server/src/main/java/ -name "*.java" -exec grep org.apache.logging.log4j.message.ParameterizedMessage {} \; -print | grep -e "\.java$" | sed "s/\/[a-zA-Z0-9]*.java$//" | sort | uniq | sed "s/^server/- [ ] server/"

for majority of use case a structured replace refactoring from intellij will be good option

relates #84478
pgomulka added a commit to pgomulka/elasticsearch that referenced this issue May 9, 2022
@pgomulka
Refactor ParametrizedMessage usages into message supplier
9aabb73 
relates elastic#86549
@pgomulka pgomulka mentioned this issue May 9, 2022
Merged
@rjernst rjernst added :Core/Infra/Logging Log management and logging utilities Meta labels May 9, 2022
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label May 9, 2022
@elasticmachine
Copy link
Collaborator

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

pgomulka added a commit that referenced this issue May 10, 2022
@pgomulka
Refactor ParameterizedMessage usages in action/admin/cluster/* to use…
65d9098 
… message supplier #86576

relates #86549
rjernst added a commit to rjernst/elasticsearch that referenced this issue May 10, 2022
@rjernst
Convert no arg parameterized message to direct strings
75af3ac 
This commit converts all ParameterizedMessages which take no arguments
to direct logging messages. There is no need for lazy evaluation since
these strings are static.

relates elastic#86549
@rjernst rjernst mentioned this issue May 10, 2022
Merged
rjernst added a commit that referenced this issue May 10, 2022
@rjernst
Convert no arg parameterized message to direct strings (#86631)
8e08c75 
This commit converts all ParameterizedMessages which take no arguments
to direct logging messages. There is no need for lazy evaluation since
these strings are static.

relates #86549
rjernst added a commit to rjernst/elasticsearch that referenced this issue May 10, 2022
@rjernst
Convert most single arg parameterized message to direct strings
cd3f546 
This commit converts most ParameterizedMessages which take one argument
to direct logging messages. This was done with regex search/replace in
IntelliJ. It does omits if those single arguments have any parenthesis
(cast or method call). Those will be done in a followup.

relates elastic#86549
@rjernst rjernst mentioned this issue May 10, 2022
Merged
rjernst added a commit that referenced this issue May 11, 2022
@rjernst
Convert most single arg parameterized message to direct strings (#86649)
7be2df9 
This commit converts most ParameterizedMessages which take one argument
to direct logging messages. This was done with regex search/replace in
IntelliJ. It does omits if those single arguments have any parenthesis
(cast or method call). Those will be done in a followup.

relates #86549
rjernst added a commit to rjernst/elasticsearch that referenced this issue May 12, 2022
@rjernst
Remove remaining single arg ParameterizedMessages
3cc2e6e 
This commit removes the remaining ParameterizedMessages that take a
single argument, this time where the argument contains method calls.
This was again done almost entirely through find/replace with regex in
IntelliJ.

relates elastic#86549
@rjernst rjernst mentioned this issue May 12, 2022
Merged
pgomulka pushed a commit that referenced this issue May 12, 2022
@rjernst
Remove remaining single arg ParameterizedMessages (#86715)
12b98b3 
This commit removes the remaining ParameterizedMessages that take a
single argument, this time where the argument contains method calls.
This was again done almost entirely through find/replace with regex in
IntelliJ.

relates #86549
@pgomulka pgomulka mentioned this issue May 19, 2022
Open
19 tasks
@pgomulka pgomulka mentioned this issue May 19, 2022
Closed
pgomulka added a commit to pgomulka/elasticsearch that referenced this issue May 19, 2022
@pgomulka
Replace Supplier of ParameterizedMessage and exception with String su…
44e069f 
…pplier

a continuation of elastic#86925
This is a result of structural search/replace in intellij. This only affects log methods with a signature
logger.level(Supplier<?>, Throwable)

<replaceConfiguration name="New expressions" text="$Instance$.$MethodCall$(()-&gt;new org.apache.logging.log4j.message.ParameterizedMessage($arg0$,$Argument$),$e$)" recursive="false" type="JAVA" pattern_context="default" reformatAccordingToStyle="true" shortenFQN="false" useStaticImport="true" replacement="$Instance$.$MethodCall$(()-&gt;String.format(java.util.Locale.ROOT, $Arg1$, $Argument$), $e$)">
  <constraint name="__context__" within="" contains="" />
  <constraint name="Argument" minCount="0" maxCount="2147483647" within="" contains="" />
  <constraint name="arg0" within="" contains="" />
  <constraint name="Instance" nameOfExprType="org\.apache\.logging\.log4j\.Logger" within="" contains="" />
  <constraint name="MethodCall" within="" contains="" />
  <constraint name="e" within="" contains="" />
  <variableDefinition name="Arg1" script="&quot;arg0.getText().replace(&quot;{}&quot;,&quot;%s&quot;)&quot;" />
</replaceConfiguration>
relates elastic#86549
pgomulka added a commit to pgomulka/elasticsearch that referenced this issue May 20, 2022
@pgomulka
Replace supplier of ParameterizedMessage with java.util.Supplier
ee55fb1 
This is a result of structural search/replace in intellij. This only affects log methods with a signature
logger.info(Supplier<?>) where level could be info/debug etc and supplier argument is in a form of
()-> new ParameterizedMessage

relates elastic#86549
@pgomulka pgomulka mentioned this issue May 20, 2022
Merged
pgomulka added a commit that referenced this issue May 23, 2022
@pgomulka
Replace supplier of ParameterizedMessage with java.util.Supplier<Stri…
24fa003 
…ng> (#86971)

This is a result of structural search/replace in intellij. This only affects log methods with a signature
logger.info(Supplier<?>) where level could be info/debug etc and supplier argument is in a form of
()-> new ParameterizedMessage

This commit also introduced a Strings utility class to avoid passing Locale.ROOT to every
String.format(Locale.ROOT, pattern, args)
relates #86549
@pgomulka pgomulka mentioned this issue May 23, 2022
Merged
pgomulka added a commit that referenced this issue May 24, 2022
@pgomulka
Refactor log4j supplier of ParameterizedMessage with throwable (#87022)
86142aa 
This is a result of structural search/replace in intellij. This only affects log methods with a signature
logger.info(Supplier<?>, Throwable) where level could be info/debug etc and supplier argument is in a form of
()-> new ParameterizedMessage

relates #86549
pgomulka added a commit to pgomulka/elasticsearch that referenced this issue May 24, 2022
@pgomulka
Replace new ParametrizedMessage() with java.util.Supplier<String>
8a071d5 
This is a result of structural search/replace in intellij. This only affects log methods with a signature
logger.info(ParametrizedMessage)
logger.info(ParametrizedMessage, Throwable)

relates elastic#86549
@pgomulka pgomulka mentioned this issue May 24, 2022
Merged
pgomulka added a commit to pgomulka/elasticsearch that referenced this issue May 25, 2022
@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 mentioned this issue May 25, 2022
Merged
pgomulka added a commit that referenced this issue May 26, 2022
@pgomulka
Replace new ParametrizedMessage() with java.util.Supplier<String> (#8…
43b69b6 
…7077)

This is a result of structural search/replace in intellij. This only affects log methods with a signature
logger.info(ParametrizedMessage)
logger.info(ParametrizedMessage, Throwable)

relates #86549
pgomulka added a commit to pgomulka/elasticsearch that referenced this issue May 26, 2022
@pgomulka
Refactor ParameterizedMessage used in lambda and casted to Supplier
e583cf3 
This is a result of structural search/replace in intellij. This only affects log methods with a signature
logger.info((Supplier<?>) ()-> ParametrizedMessage)
logger.info((Supplier<?>) ()-> ParametrizedMessage, Throwable)

relates elastic#86549
@pgomulka pgomulka mentioned this issue May 26, 2022
Merged
pgomulka added a commit that referenced this issue May 30, 2022
@pgomulka
Catch an exception due to incorrect pattern in Strings.format (#87132)
416a1b3 
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
pgomulka added a commit that referenced this issue May 31, 2022
@pgomulka
Refactor ParameterizedMessage used in lambda and casted to Supplier (#…
705b27a 
…87156)

This is a result of structural search/replace in intellij. This only affects log methods with a signature
logger.info((Supplier) ()-> ParametrizedMessage) logger.info((Supplier) ()-> ParametrizedMessage, Throwable)

relates #86549
pgomulka added a commit to pgomulka/elasticsearch that referenced this issue Jun 2, 2022
@pgomulka
Refactor ParametrizedMessage.getFormattedMessage into Strings.format
ad4bbf7 
ParametrizedMessage is part of log4j api and should not be used
in places where a simple String.format would be enough.
Many of usages like this are message formatting for exceptions being
thrown.

relates elastic#86549
@pgomulka pgomulka mentioned this issue Jun 2, 2022
Merged
pgomulka added a commit that referenced this issue Jun 13, 2022
@pgomulka
Refactor ParametrizedMessage.getFormattedMessage into Strings.format (#…
b123feb 
…87324)

ParametrizedMessage is part of log4j api and should not be used
in places where a simple String.format would be enough.
Many of usages like this are message formatting for exceptions being
thrown.

relates #86549
@pgomulka pgomulka mentioned this issue Jun 13, 2022
Merged
pgomulka added a commit that referenced this issue Jun 14, 2022
@pgomulka
Refactor various ParameterizedMessage usages into String.format (#87603)
8113c6f 
ParameterizedMessage is part of log4j api and should not be used
in places where a lambda and String.format would be enough.

This commit is the last batch of refactoring to get rid of ParameterizedMessage
in ES codebase and consists of various, not related usages.

relates #86549
@pgomulka pgomulka mentioned this issue Jun 14, 2022
Merged
pgomulka added a commit that referenced this issue Jun 14, 2022
@pgomulka
Add ParameterizedMessage to forbidden api (#87631)
db59a88 
ParameterizedMessage will not be part of the new ES logging API and therefore should not be used.
java.util.Supplier and String.format should be used instead.

this commit adds ParameterizedMessage to forbidden api

relates #86549
@pgomulka
Copy link
Contributor

the refactoring is now done

@ywangd ywangd mentioned this issue Nov 12, 2022
Merged
ywangd added a commit that referenced this issue Nov 14, 2022
@ywangd
Fix variable placeholder for Strings.format calls (#91531)
7b71c94 
The curly bracket placeholder works for LoggerMessageFormat.format and
ParameterizedMessage.format, but Not for Strings.format which requires
Java's string format syntax. This PR fixes the incorrect usages.

Relates: #86549, #87924
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
:Core/Infra/Logging Log management and logging utilities Meta Team:Core/Infra Meta label for core/infra team
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@rjernst @pgomulka @elasticmachine @ChrisHegarty