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

[jmxfetch] add rmi registry ssl config option #2419

Merged
merged 4 commits into from
Oct 11, 2018
Merged

Conversation

arbll
Copy link
Member

@arbll arbll commented Oct 5, 2018

What does this PR do?

Add new config options introduced by DataDog/jmxfetch#185

@arbll arbll requested a review from a team as a code owner October 5, 2018 09:23
@arbll arbll added documentation changelog/no-changelog [deprecated] team/agent-core Deprecated. Use metrics-logs / shared-components labels instead.. labels Oct 5, 2018
@arbll arbll added this to the 6.6.0 milestone Oct 5, 2018
Copy link
Member

@olivielpeau olivielpeau left a comment

Choose a reason for hiding this comment

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

Thanks!

Left a few comments, let me know what you think.

Also, we shouldn't merge this until the Agent actually uses a new version of JMXFetch that supports these options

# # java_options: "-Xmx200m -Xms50m" # Optional, Java JVM options
# # trust_store_path: /path/to/trustStore.jks # Optional, should be set if ssl is enabled
# # trust_store_password: password
# # refresh_beans: 600 # Optional (in seconds), default is 600 seconds. Sets refresh period for refreshing matching MBeans list.
Copy link
Member

Choose a reason for hiding this comment

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

the double # here were meaningful to reflect that the options are optional. Just pointing that out, not sure it should make sense to keep things this way

Copy link
Member Author

Choose a reason for hiding this comment

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

Only host and port are really non-optional and we don't do this in most configuration so it looks inconsistent to me.

# # refresh_beans: 600 # Optional (in seconds), default is 600 seconds. Sets refresh period for refreshing matching MBeans list.
# java_bin_path: /path/to/java # Optional, should be set if the agent cannot find your java executable
# java_options: "-Xmx200m -Xms50m" # Optional, Java JVM options
# trust_store_path: /path/to/trustStore.jks # Optional, should be set if "com.sun.management.jmxremote.ssl" is set to true.
Copy link
Member

Choose a reason for hiding this comment

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

is set to true on the target JVM I assume?

may require a longer explanation on what this jks file is exactly (not necessarily here though, full explanation could be on docs website)

# key_store_path: /path/to/keyStore.jks # Optional, should be set if "com.sun.management.jmxremote.ssl.need.client.auth" is set to true.
# key_store_password: password
# rmi_registry_ssl: false # Optional, should be set to true if "com.sun.management.jmxremote.registry.ssl" is set to true.
# refresh_beans: 600 # Optional (in seconds), default is 600 seconds. Sets refresh period for refreshing matching MBeans list.
# # Decreasing this value may result in increased CPU usage.
Copy link
Member

Choose a reason for hiding this comment

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

inconsistent indentation

# java_options: "-Xmx200m -Xms50m" # Optional, Java JVM options
# trust_store_path: /path/to/trustStore.jks # Optional, should be set if "com.sun.management.jmxremote.ssl" is set to true.
# trust_store_password: password
# key_store_path: /path/to/keyStore.jks # Optional, should be set if "com.sun.management.jmxremote.ssl.need.client.auth" is set to true.
Copy link
Member

Choose a reason for hiding this comment

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

same comments as for trust_store_path

olivielpeau
olivielpeau previously approved these changes Oct 10, 2018
Copy link
Member

@olivielpeau olivielpeau left a comment

Choose a reason for hiding this comment

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

just an indentation nit, other than that LGTM 👍

# key_store_password: password
# rmi_registry_ssl: false # Optional, should be set to true if "com.sun.management.jmxremote.registry.ssl" is set to true on the target JVM.
# refresh_beans: 600 # Optional (in seconds), default is 600 seconds. Sets refresh period for refreshing matching MBeans list.
# # Decreasing this value may result in increased CPU usage.
Copy link
Member

Choose a reason for hiding this comment

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

let's indent this comment like the previous line, otherwise it looks like it applies to the option below it.

@arbll arbll merged commit 5192f83 into master Oct 11, 2018
@arbll arbll deleted the arbll/update-jmx-options branch October 11, 2018 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog [deprecated] team/agent-core Deprecated. Use metrics-logs / shared-components labels instead.. documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants