Use Lombok Builder for AppConfig, Formalize jvm_direct Connection #225
Conversation
tylerbenson
force-pushed
the
tyler/appconfig-builder
branch
from
3c2aa4f to
b259fdb
Compare
Expose new options for configuring JMXFetch with standard datadog-agent config files with `jvm_direct: true` set as an instance attribute (this will be ignored by the datadog-agent). For Example: * `dd.agent.conf.d=/opt/datadog-agent/etc/conf.d` * `dd.jmxfetch.configs=activemq.d/conf.yaml,jmx.d/conf.yaml` will load jmx configs in those two files that have `jvm_direct: true` in their `instance` setup. Environment variables can also be used: `DD_AGENT_CONF_D` and `DD_JMXFETCH_CONFIGS` Depends on a new release including DataDog/jmxfetch#225
| * If set to true, other instances will be ignored. | ||
| */ | ||
| @Builder.Default | ||
| private boolean allowDirectInstances = false; |
There was a problem hiding this comment.
The "allow" in the name doesn't convey that If set to true, other instances will be ignored..
Maybe something like targetDirectInstances?
There was a problem hiding this comment.
Yeah, I was having a hard time coming up with a better name... I like that better though. Thanks for the suggestion.
| @@ -41,9 +42,30 @@ public static void setup(Level level, String logLocation) { | |||
| } | |||
|
|
|||
| /** Laconic logging for reduced verbosity. */ | |||
| public static void laconic(Logger logger, Level level, String message, int max) { | |||
| public static void laconic(org.slf4j.Logger logger, Level level, String message, int max) { | |||
| @@ -1,7 +1,7 @@ | |||
| init_config: | |||
|
|
|||
| instances: | |||
| - process_name_regex: .*surefire.* | |||
| - jvm_direct: true | |||
There was a problem hiding this comment.
Aren't process_name_regex and jvm_direct slightly different? One will attach to a running JVM, while the other is launched as a -javaagent with the application? Would different test cases be needed?
There was a problem hiding this comment.
They are slightly different, but from this test perspective, they're mostly the same. There are plenty of other tests that verify the process_name_regex behavior, so I felt it was ok to change this one rather than add additional complexity in the tests.
(If we moved them all to jvm_direct we might be able to run the tests from within an IDE, rather than just via maven.)
| private final static Logger LOGGER = Logger.getLogger("Test Task Processor"); | ||
|
|
||
| class TestSimpleTask extends InstanceTask<Boolean> { | ||
| class TestSimpleTask extends InstanceTask<Boolean> { |
There was a problem hiding this comment.
nit: this doesn't look properly aligned?
There was a problem hiding this comment.
turns out there was a sneaky tab in there... 🤷♂
More consistent file config with existing format.
Also fix Instance.toString()
tylerbenson
force-pushed
the
tyler/appconfig-builder
branch
from
2bcfb19 to
633e6a2
Compare
Based on #223, so focus on last two commits for review.
Will be used in
dd-java-agent. A future release can completely remove themetricConfigFilessetting (but notmetricConfigResources).