-
Notifications
You must be signed in to change notification settings - Fork 70
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
Cache Metric instances #273
Conversation
d4613c7
to
0e38c1a
Compare
During collection process, metrics are collected as Map<String, Object>, but keys are known and fixed so we can replace this map to a POJO which will be lighter in term of memory footprint and allocation.
Use Metric class as a cache and avoid recreating it for each collection. Only the value is updated. For simple & complex attribute Tags are also cached into the Metric instance. For complex & tabular attribute, cache is done per sub attribute into a new JMXSubAttribute abstract class Transfer checkName into the attribute to be inserted into the Metric instance at creation
0e38c1a
to
7828b3e
Compare
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.
Looks good, added some notes to myself for reference purposes, but didn't see anything that struck me as odd.
if (converted == null && getValueConversions(field).get("default") != null) { | ||
converted = getValueConversions(field).get("default"); | ||
} | ||
Map<Object, Object> valueConversions = getValueConversions(field); |
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.
logic refactor looks good 👍
} | ||
double value = castToDouble(getValue(), null); | ||
cachedMetric.setValue(value); | ||
return Collections.singletonList(cachedMetric); |
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.
Note: looks like this immutable list is fine, calling functions do not attempt to change it.
@@ -133,10 +135,12 @@ private void populateSubAttributeList(Object value) { | |||
String dataKey = entry.getKey(); | |||
List<String> subSub = entry.getValue(); | |||
for (String metricKey : subSub) { | |||
String alias = convertMetricName(getAlias(metricKey)); | |||
String alias = getAlias(metricKey); |
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.
🍰 this should be fine because getAlias()
already calls convertMetricName
, we were just duplicating work.
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.
Should be ready now, I had missed a small issue, perhaps introduced it myself even when addressing the merge conflicts.
I will probably merge this immediately given the CI looks good. Any follow-up could be tackled in future PRs.
Use Metric class as a cache and avoid recreating it for each
collection. Only the value is updated. For simple & complex attribute
Tags are also cached into the Metric instance.
For complex & tabular attribute, cache is done per sub attribute into
a new JMXSubAttribute abstract class
Transfer checkName into the attribute to be inserted into the Metric
instance at creation