Issue 7212 - Allow multiple YAML configuration files for JMX rules #7284
Conversation
| try { | ||
|
|
||
| JmxConfig config = loadConfig(is); | ||
| if (config != null) { | ||
| logger.log(INFO, "Found {0} metric rules", config.getRules().size()); | ||
| logger.log(INFO, id + ": found {0} metric rules", config.getRules().size()); |
There was a problem hiding this comment.
Can you use placeholders?
| logger.log(INFO, id + ": found {0} metric rules", config.getRules().size()); | |
| logger.log(INFO, "{0}: found {1} metric rules", new Object[] {id, config.getRules().size()}); |
| config.addMetricDefsTo(conf); | ||
| } | ||
| } catch (Exception exception) { | ||
| logger.log(WARNING, "Failed to parse YAML rules: " + rootCause(exception)); | ||
| logger.log(WARNING, "Failed to parse YAML rules from " + id + ": " + rootCause(exception)); |
There was a problem hiding this comment.
Same here, please use placeholders instead of glueing log messages manually
| String files = configProperties.getString("otel.jmx.config", ""); | ||
| String[] configFiles = files.isEmpty() ? new String[0] : files.split(","); |
There was a problem hiding this comment.
You can use ConfigProperties#getList() instead
| } catch (Exception e) { | ||
| JmxMetricInsight.getLogger().warning(e.getMessage()); | ||
| JmxMetricInsight.getLogger().warning(e.toString()); |
There was a problem hiding this comment.
This pretty much only logs the message; shouldn't we log the whole stacktrace here?
There was a problem hiding this comment.
Not in this case. An exception at this location indicates difficulties with reading the file (user error), and the message should be sufficient for troubleshooting.
There was a problem hiding this comment.
is there a specific exception that indicates parsing error (e.g. YamlException)? or is the code inside the try block narrow enough that there shouldn't be any other type of exception here?
There was a problem hiding this comment.
Parsing errors are handled by RuleParser, and an effort has been made to provide as useful contextual info there as possible. But again, without stack traces, as they would not get any value for the users, IMHO.
Only file access related exceptions are expected here.
| RuleParser parserInstance = RuleParser.get(); | ||
| try (InputStream inputStream = Files.newInputStream(new File(jmxDir.trim()).toPath())) { | ||
| parserInstance.addMetricDefsTo(conf, inputStream); | ||
| try (InputStream inputStream = Files.newInputStream(new File(configFile).toPath())) { |
There was a problem hiding this comment.
| try (InputStream inputStream = Files.newInputStream(new File(configFile).toPath())) { | |
| try (InputStream inputStream = Files.newInputStream(Paths.get(configFile))) { |
There was a problem hiding this comment.
Thanks for the review, @mateuszrzeszutek!
.../src/main/java/io/opentelemetry/instrumentation/javaagent/jmx/JmxMetricInsightInstaller.java
Outdated
Show resolved
Hide resolved
…emetry/instrumentation/javaagent/jmx/JmxMetricInsightInstaller.java Co-authored-by: Trask Stalnaker <[email protected]>
No description provided.