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

Replace Map metric by a Metric class #271

Merged
merged 1 commit into from
Feb 21, 2020

Conversation

jpbempel
Copy link
Member

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.

@jpbempel jpbempel requested a review from a team February 14, 2020 08:38
@jpbempel jpbempel changed the title Replace Map metric to a Metric class Replace Map metric by a Metric class Feb 14, 2020
@jpbempel jpbempel force-pushed the jpbempel/newMetricClass branch from 71256ff to 95d59a6 Compare February 14, 2020 08:44
@truthbk truthbk added this to the 0.35.0 milestone Feb 18, 2020
@jpbempel jpbempel force-pushed the jpbempel/newMetricClass branch from 95d59a6 to 2fb7b10 Compare February 19, 2020 08:11
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.
@jpbempel jpbempel force-pushed the jpbempel/newMetricClass branch from 2fb7b10 to 6aafa60 Compare February 19, 2020 08:29
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 to me, indeed there seems to be some old code lying around we can clean up. No need to do in this PR.

// We need to edit metrics for legacy reasons (rename metrics, etc)
Map<String, Object> metric = new HashMap<String, Object>(m);
// TODO: There is no obvious reason to duplicate this object here. should be removed.
Metric metric = new Metric(m.getAlias(), m.getMetricType(), m.getValue(), m.getTags());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I see a reason for this allocation either, maybe we should remove it and just work with Metric m? It doesn't seem like the comment above is true anymore "we need to edit metrics for legacy reasons".

@truthbk
Copy link
Member

truthbk commented Feb 21, 2020

@jpbempel going to merge this. Please feel free to address any of the feedback, which didn't affect any of the logic introduced here, in a follow-up PR. Thank you! 🙇

@truthbk truthbk merged commit ce22836 into DataDog:master Feb 21, 2020
@jpbempel jpbempel deleted the jpbempel/newMetricClass branch February 24, 2020 09:39
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