jmx idiomatic library API - part 1 #15220
Conversation
SylvainJuge
changed the title
| private static String resourceFor(String platform) { | ||
| return "/jmx/rules/" + platform + ".yaml"; | ||
| } | ||
|
|
There was a problem hiding this comment.
[for reviewer] this part is now embedded directly into the library, so we don't expose internal details anymore, this also enables removing similar code in contrib jmx-scraper (or anything that does rely on this library).
There was a problem hiding this comment.
do we have a plan how to use declarative configuration instead of a separate yaml file in the future?
I bring it up because a library redesign would be a good opportunity to plan ahead for that
There was a problem hiding this comment.
That's a good point, we don't have any concrete plan for declarative config, but this is something that we need to think about in the future, I have already opened #14916 to not forget about it.
| } catch (IllegalArgumentException e) { | ||
| // for now only log JMX errors as they do not prevent agent startup | ||
| logger.log(Level.SEVERE, "Error while loading JMX configuration", e); |
There was a problem hiding this comment.
[for reviewer] for now an invalid configuration does not prevent the agent from starting, it just logs an error which was also the current behavior.
There was a problem hiding this comment.
The overall behavior does not change here, the agent currently starts with warnings in the logs when there are parsing errors, now an exception is thrown and it only logs the first error thanks to this catch statement.
| * @param conf the metric configuration | ||
| * @param is the InputStream with the YAML rules | ||
| * @param id identifier of the YAML ruleset, such as a filename | ||
| * @throws IllegalArgumentException when unable to parse YAML |
There was a problem hiding this comment.
[for reviewer] throwning an exception here allows to let the caller of the library how to handle parsing errors, for now the agent just logs the first that happen. Doing so also allows to validate existing yaml configuration in the "legacy" state that are not included in the library (but in the agent), all of that without relying on classes that will be moved to internal packages later.
| rule.buildMetricDef(); | ||
| } | ||
| // loading rules from direct file access | ||
| JmxTelemetry.builder(OpenTelemetry.noop()).addCustomRules(filePath); |
There was a problem hiding this comment.
[for reviewer] we have to load those files through direct file access because we can't resolve them directly because they are part of the unamed module and Class#getResource(...) does not work in this case, this should be fine because this test class will be removed later and everything will be tested through classpath when moved to library.
There was a problem hiding this comment.
it might be easier just to wait for the library release (that was my experience working with different java repos)
There was a problem hiding this comment.
I don't think the library release will change the resource visibility as both this test and the library are part of the same repository.
The goal of this test is just to ensure that there are no parsing errors, there aren't any dedicated assertions on the metrics definitions themselves.
2 participants
First part for #14674
Introduces idiomatic
Telemetry/TelemetryBuilderfor JMX metrics.JmxTelemetry/JmxTelemetryBuilderthat delegates to the existing implementation.