-
Notifications
You must be signed in to change notification settings - Fork 867
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
Issue 7212 - Allow multiple YAML configuration files for JMX rules #7284
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -59,7 +59,7 @@ private static void addRulesForPlatform(String platform, MetricConfiguration con | |||||
if (inputStream != null) { | ||||||
JmxMetricInsight.getLogger().log(FINE, "Opened input stream {0}", yamlResource); | ||||||
RuleParser parserInstance = RuleParser.get(); | ||||||
parserInstance.addMetricDefsTo(conf, inputStream); | ||||||
parserInstance.addMetricDefsTo(conf, inputStream, platform); | ||||||
} else { | ||||||
JmxMetricInsight.getLogger().log(INFO, "No support found for {0}", platform); | ||||||
} | ||||||
|
@@ -80,14 +80,15 @@ private static void buildFromDefaultRules( | |||||
|
||||||
private static void buildFromUserRules( | ||||||
MetricConfiguration conf, ConfigProperties configProperties) { | ||||||
String jmxDir = configProperties.getString("otel.jmx.config"); | ||||||
if (jmxDir != null) { | ||||||
JmxMetricInsight.getLogger().log(FINE, "JMX config file name: {0}", jmxDir); | ||||||
String files = configProperties.getString("otel.jmx.config", ""); | ||||||
String[] configFiles = files.isEmpty() ? new String[0] : files.split(","); | ||||||
for (String configFile : configFiles) { | ||||||
JmxMetricInsight.getLogger().log(FINE, "JMX config file name: {0}", configFile); | ||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the review, @mateuszrzeszutek! |
||||||
parserInstance.addMetricDefsTo(conf, inputStream, configFile); | ||||||
} catch (Exception e) { | ||||||
JmxMetricInsight.getLogger().warning(e.getMessage()); | ||||||
JmxMetricInsight.getLogger().warning(e.toString()); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This pretty much only logs the message; shouldn't we log the whole stacktrace here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
} | ||||||
} | ||||||
} | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -51,17 +51,18 @@ public JmxConfig loadConfig(InputStream is) throws Exception { | |||||
* | ||||||
* @param conf the metric configuration | ||||||
* @param is the InputStream with the YAML rules | ||||||
* @param id identifier of the YAML ruleset, such as a filename | ||||||
*/ | ||||||
public void addMetricDefsTo(MetricConfiguration conf, InputStream is) { | ||||||
public void addMetricDefsTo(MetricConfiguration conf, InputStream is, String id) { | ||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you use placeholders?
Suggested change
|
||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, please use placeholders instead of glueing log messages manually |
||||||
// It is essential that the parser exception is made visible to the user. | ||||||
// It contains contextual information about any syntax issues found by the parser. | ||||||
logger.log(WARNING, exception.toString()); | ||||||
|
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.
You can use
ConfigProperties#getList()
instead