opensearch-project / flow-framework

OpenSearch plugin that enables builders to innovate AI apps on OpenSearch
Apache License 2.0
32 stars 35 forks source link

Replace String concatenation with Log4j ParameterizedMessage for readability #905

Open dbwiddis opened 3 weeks ago

dbwiddis commented 3 weeks ago

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: https://github.com/opensearch-project/flow-framework/blob/7a93d6c543738c7250313b0f66f3f794f42f5f1b/src/main/java/org/opensearch/flowframework/transport/ProvisionWorkflowTransportAction.java#L140-L144

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: https://github.com/opensearch-project/flow-framework/pull/898/commits/86ac8ea862ba59abcbdf036fd637159e751e3be4

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 commented 3 weeks ago

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 commented 3 weeks ago

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 <yourname@example.com> 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 commented 3 weeks ago

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 commented 3 weeks ago

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

dbwiddis commented 2 weeks ago

@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 commented 1 week ago

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

dbwiddis commented 1 week ago

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

KirrTap commented 1 week ago

Yes, I found it