Replace String concatenation with Log4j ParameterizedMessage for readability

[dbwiddis]

Is your feature request related to a problem?

Log4j includes parameter substitution which improves readability of log messages and has improved performance and stability over similar String.format():

logger.info("Updating {} with {}.", baz.getFoo(), baz.getBar());

This works well when we are only logging a message. However, we have many cases in our Transport Action exception handling where we use the same message in both the log and exception message (I know because I wrote them!). One such example:

flow-framework/src/main/java/org/opensearch/flowframework/transport/ProvisionWorkflowTransportAction.java

Lines 140 to 144 in 7a93d6c

	} catch (Exception e) {
	String errorMessage = "Failed to retrieve template from global context for workflow " + workflowId;
	logger.error(errorMessage, e);
	listener.onFailure(new FlowFrameworkException(errorMessage, ExceptionsHelper.status(e)));
	}

Log4J's ParameterizedMessage class provides a way to construct the message the same way for both cases.

What solution would you like?

import org.apache.logging.log4j.message.ParameterizedMessageFactory;

} catch (Exception e) {
    String errorMessage = ParameterizedMessageFactory.INSTANCE.newMessage(
        "Failed to retrieve template from global context for workflow {}",
        workflowId
    ).getFormattedMessage();
    logger.error(errorMessage, e);
    listener.onFailure(new FlowFrameworkException(errorMessage, ExceptionsHelper.status(e)));
}

Example commit inspiring this issue: 86ac8ea

What alternatives have you considered?

1. Keep the current concatenation (less readable)
2. Replace with StringBuilder implementation (less readable)
3. Replace with String.format() (performance, can throw exceptions, locale concerns)

Do you have any additional context?

There are lots of these. As a good first issue, doing any one of them, or perhaps a class at a time, would be a welcome contribution!

In addition, since most of them follow the same pattern, a new method (per class) of handleException(Exception e, String formatString, Object... args) would probably be useful.

[boooby19]

Hello ! I am new to open-source projects. I am working as junior dev in c#/React, also have 3 year experience with Java. Can I work on this issue? Can you give me some more info? Thanks ! :)

[dbwiddis]

Hi @boooby19! Welcome! If you have some experience in Java, this should generally be straightforward code-wise, as I've outlined the needed changes in the first comment.

Creating your first GitHub PR is a bit more of a challenge. I messed up my first GitHub PR, so I hope you don't follow in my footsteps! :D I linked the steps in another issue but I'll recreate them here. They're also in the CONTRIBUTING document on the main repo page.

We use a triangle workflow (you can google that) but basically:

1. you will fork this repository to your own GitHub repository (look for a "Fork" button in the upper right corner of the main page)
2. you will clone your personal repo to your local machine (clicking the dropdown on the "code" button on the main page gives you the git clone commands or other ways to do it)
3. you will start with an updated main branch (git pull upstream main)
4. create your own feature branch with whatever name you want (git checkout -b pick-a-name-here)
5. write your code!
6. write tests for your changes!
7. commit as often as you want as you complete portions of the work. Note you need to "sign" your commits using DCO (essentially adds a Signed-off-by: Your Name <[email protected]> line to your commit message). If using git command line, use the -s switch (lowercase). Most IDEs have a "signoff" button to click.
8. Eventually when you think you're ready to submit, format the code with ./gradlew spotlessApply
9. Also make sure you've done your javadocs ./gradlew javadoc
10. And make sure tests pass ./gradlew test

(In this specific case, you're refactoring existing lines so there shouldn't be any new tests needed (item 6) nor javadocs (item 9) so those can be skipped.)

If all that works, push (git push) to upload the code to your fork. The response from GitHub will give you a URL you can go to, OR you can go to the main repo and there's usually a pop up showing you recently updated a branch, OR you can go to your fork and look for the "contribute" button. Follow that dialogue to open a PR.

To respond to PR review comments, just edit your code, commit, and push (to your fork) and the PR will automatically get updated.

[KirrTap]

Hello ! I am also new to open-source projects. I would like to try to work on this issue. I am a student at the University and I have some experience with Java, Python, C# and React. Can I work on this issue?

[boooby19]

Hello @dbwiddis , @KirrTap is my friend and I agree to re-assign this issue to @KirrTap , is it possible please? :) thanks !

[dbwiddis]

@boooby19 @KirrTap there are several classes that include this pattern so you could both participate!

See https://github.com/search?q=repo%3Aopensearch-project%2Fflow-framework+%22logger.error%28errorMessage%22&type=code

[dbwiddis]

Hey @boooby19 and @KirrTap need any help on this? Let us know, we're happy to help.

[dbwiddis]

The DEVELOPER_GUIDE should give you commands you can run to fork/clone, build, and test.

[KirrTap]

Yes, I found it

[KirrTap]

I have replaced strings "logger.error(errorMessage" in this 40 files: https://github.com/search?q=repo%3Aopensearch-project%2Fflow-framework+%22logger.error%28errorMessage%22&type=code .

Can I create PR ?

[dbwiddis]

I have replaced strings "logger.error(errorMessage" in this 40 files: https://github.com/search?q=repo%3Aopensearch-project%2Fflow-framework+%22logger.error%28errorMessage%22&type=code .

Can I create PR ?

Feel free to create a PR at any time and we can discuss the code there.

Note that you do not need to replace ones that are not parameterized. The link you showed includes

String errorMessage = "There are no templates in the global_context";
logger.error(errorMessage);

That's totally fine as there are no substitutions. The main goal is to replace concatenation like this:

String errorMessage = "Failed to retrieve template (" + workflowId + ") from global context.";

with a parameterized string "Failed to retrieve template {} from global context."