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

Replace String concatenation with Log4j ParameterizedMessage for readability #905

Open
dbwiddis opened this issue Oct 7, 2024 · 2 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@dbwiddis
Copy link
Member

dbwiddis commented Oct 7, 2024

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:

} 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.

@dbwiddis dbwiddis added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Oct 7, 2024
@dbwiddis dbwiddis removed the untriaged label Oct 7, 2024
@boooby19
Copy link

boooby19 commented Oct 8, 2024

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
Copy link
Member Author

dbwiddis commented Oct 8, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants