-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
There was a problem hiding this 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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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
There was a problem hiding this 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. |
There was a problem hiding this comment.
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.
What does this PR do?
Add new config options introduced by DataDog/jmxfetch#185