Skip to content
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

Merged
merged 4 commits into from
Feb 21, 2020
Merged

Conversation

jpbempel
Copy link
Member

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

@jpbempel jpbempel requested a review from a team February 17, 2020 09:14
@truthbk truthbk added this to the 0.35.0 milestone Feb 18, 2020
@jpbempel jpbempel force-pushed the jpbempel/cacheMetrics branch from d4613c7 to 0e38c1a Compare February 19, 2020 08:12
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
@jpbempel jpbempel force-pushed the jpbempel/cacheMetrics branch from 0e38c1a to 7828b3e Compare February 19, 2020 08:36
Copy link
Member

@truthbk truthbk left a 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);
Copy link
Member

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);
Copy link
Member

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);
Copy link
Member

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.

Copy link
Member

@truthbk truthbk left a 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.

@truthbk truthbk merged commit c03bfff into DataDog:master Feb 21, 2020
@jpbempel jpbempel deleted the jpbempel/cacheMetrics branch February 24, 2020 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants