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

[AMLII-1817] Added support for emitting logs with thread name #525

Merged
merged 3 commits into from
Jun 7, 2024

Conversation

carlosroman
Copy link
Contributor

No description provided.

@carlosroman carlosroman requested review from a team as code owners June 3, 2024 10:31
@carlosroman carlosroman force-pushed the carlosroman/adding-thread-info-to-logger branch 6 times, most recently from 7a7fd4d to cfc1bf7 Compare June 3, 2024 11:18
src/main/java/org/datadog/jmxfetch/util/CustomLogger.java Outdated Show resolved Hide resolved
description = "Logs the thread name with each message",
required = false)
@Builder.Default
private boolean logThreadName = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any downside to having this enabled by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is nothing wrong with enabling it by default or actually just always doing it and removing this flag. Only reason I didn't do it as was following the same pattern used by the milliseconds flag. I guess if someone is parsing the logs already then this would mean their parser breaks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think milliseconds are optional, to keep the default format aligned with the agent, which doesn't use millisecond timestamps.

same pattern used by the milliseconds flag

It's not quite the same pattern though: milliseconds option uses a java property as a toggle, not a command-line flag. This difference is somewhat of consequence, since users can set the properties via java_options, but IIRC there is no way to add arbitrary app-level flags (all existing flags are hardcoded in the jmxfetch.go).

@carlosroman carlosroman force-pushed the carlosroman/adding-thread-info-to-logger branch from cfc1bf7 to 0d9c91a Compare June 5, 2024 14:55
description = "Logs the thread name with each message",
required = false)
@Builder.Default
private boolean logThreadName = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might not work as expected. According to the docs, to negate a true-by-default boolean parameter, the parameter needs to be configured take the false value explicitly.

But if this is just an escape hatch, then perhaps configuring this via a property would be better. The agent doesn't allow users to set parameters for jmxfetch, so this can not be toggled without an explicit support in the agent. But the agent allows to pass options to the jvm, which can be used to change properties. WDYT?

@carlosroman carlosroman merged commit 89e8328 into master Jun 7, 2024
9 checks passed
@carlosroman carlosroman deleted the carlosroman/adding-thread-info-to-logger branch June 7, 2024 15:50
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