Skip to content

Feature: Adding Metric Aggregation in the form of a StatisticSet #150

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

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

lksfrnz
Copy link

@lksfrnz lksfrnz commented Jul 2, 2024

Introduces a new metric type called StatisticSet which will aggregate a set of metrics into the following properties: maximum, minimum, count, and sum. Users can create and log their own StatisticSet metric with the StatisticSetBuilder class or specify a metric's method when putting it into the metricsLogger and the metricsLogger will handle all aggregation.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@lksfrnz lksfrnz force-pushed the StatisticSet branch 3 times, most recently from d1b2a49 to 683c7e1 Compare July 4, 2024 21:56
@lksfrnz lksfrnz marked this pull request as ready for review July 4, 2024 21:58

@Override
protected StatisticSet getMetricValuesOverSize(int size) {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we returning null here?


@Override
protected StatisticSet getMetricValuesUnderSize(int size) {
return this;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to return the own object here? Doesn't seem to actual implement what the method name suggests nor does it use the method's arguments.

@giancarlokc giancarlokc self-requested a review July 19, 2024 14:24
while (!metricQueue.isEmpty()) {
Metric metric = metricQueue.poll();
ArrayList<Queue<Metric>> remainingMetrics = new ArrayList<>();
PriorityQueue<Queue<Metric>> metricQueue =
Copy link
Author

Choose a reason for hiding this comment

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

Using a PriorityQueue here instead of just cycling through a queue is a slightly slower implementation but ensures that the minimum amount of metrics are flushed which the previous solution didn't.

For example:
let MAX_METRICS_PER_EVENT = 2
let MAX_DATAPOINTS_PER_METRIC = 1

lets say we have a queue of metrics represented as how data points are in each metric
{"M1" : 1, "M2" : 1, "M3" : 2}

The previous implementation would result in 3 flushes as follows
Flushed : ["M1", "M2"] Remaining : {"M3" : 2}
Flushed : ["M3"] Remaining : {"M3" : 1}
Flushed : ["M3"] Remaining : {}

The New implementation will result in 2 flushes as follows:
Flushed : ["M1", "M3"] Remaining : {"M2" : 1, "M3" : 1}
Flushed : ["M2", "M3"] Remaining : {}

@giancarlokc giancarlokc merged commit d8ae46b into awslabs:histogram_feature_branch Jul 25, 2024
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.

3 participants