Skip to content

Parameterized logging jul #77

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

Merged
merged 4 commits into from
Jul 1, 2020
Merged

Parameterized logging jul #77

merged 4 commits into from
Jul 1, 2020

Conversation

NickWemekamp
Copy link
Contributor

The java util logging formatter logged the following method call:
logger.log(Level.INFO, "test {0}", "test1"); as "test {0}" in the message field of the json body whereas "test test1" is the intended behaviour.

By using the super.formatMessage method from the JUL Formatter the parameters do get placed into the curly brackets.

I have also added a .replace("\r\n", "\n") in a unit test because the unit test does not work on windows because the line endings are different.

@cla-checker-service
Copy link

cla-checker-service bot commented Jun 30, 2020

💚 CLA has been signed

@apmmachine
Copy link
Contributor

apmmachine commented Jun 30, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #77 updated]

  • Start Time: 2020-07-01T11:21:07.359+0000

  • Duration: 15 min 35 sec

Test stats 🧪

Test Results
Failed 0
Passed 104
Skipped 4
Total 108

Copy link
Member

@felixbarny felixbarny left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

public abstract void debug(String message);

public abstract void debug(String message, Object[] logParams);
Copy link
Member

Choose a reason for hiding this comment

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

Just an idea: may be a vararg so that callers can do debug("{} is not {}", 1, 2) vs debug("{} is not {}", new Object[]{1, 2})

Suggested change
public abstract void debug(String message, Object[] logParams);
public abstract void debug(String message, Object... logParams);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I agree, I've refactored it to varargs.

What do you think of the place of the testSimpleParameterizedLog method? I was not sure about whether I should place the test in the abstract base class AbstractEcsLoggingTest (because testSimpleLog is also already there), or make a test in each formatter project.

Copy link
Member

Choose a reason for hiding this comment

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

I really like the abstract way of handling this! IMO, the test classes in each formatted project should be as simple as possible and ideally only extend the abstract test and implement its abstract methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok nice! :)

@felixbarny felixbarny merged commit 4e690e0 into elastic:master Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants