-
Notifications
You must be signed in to change notification settings - Fork 37
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
Feature: Adding Metric Aggregation in the form of a StatisticSet #150
Conversation
d1b2a49
to
683c7e1
Compare
src/main/java/software/amazon/cloudwatchlogs/emf/logger/MetricsLogger.java
Outdated
Show resolved
Hide resolved
src/main/java/software/amazon/cloudwatchlogs/emf/model/StatisticSet.java
Outdated
Show resolved
Hide resolved
src/main/java/software/amazon/cloudwatchlogs/emf/model/MetricsContext.java
Show resolved
Hide resolved
src/main/java/software/amazon/cloudwatchlogs/emf/model/MetricsContext.java
Outdated
Show resolved
Hide resolved
|
||
@Override | ||
protected StatisticSet getMetricValuesOverSize(int size) { | ||
return null; |
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.
Why are we returning null here?
|
||
@Override | ||
protected StatisticSet getMetricValuesUnderSize(int size) { | ||
return this; |
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.
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.
src/main/java/software/amazon/cloudwatchlogs/emf/model/StatisticSet.java
Outdated
Show resolved
Hide resolved
src/main/java/software/amazon/cloudwatchlogs/emf/model/StatisticSet.java
Outdated
Show resolved
Hide resolved
src/main/java/software/amazon/cloudwatchlogs/emf/logger/MetricsLogger.java
Outdated
Show resolved
Hide resolved
src/test/java/software/amazon/cloudwatchlogs/emf/model/MetricDirectiveThreadSafetyTest.java
Outdated
Show resolved
Hide resolved
src/main/java/software/amazon/cloudwatchlogs/emf/model/StatisticSet.java
Outdated
Show resolved
Hide resolved
while (!metricQueue.isEmpty()) { | ||
Metric metric = metricQueue.poll(); | ||
ArrayList<Queue<Metric>> remainingMetrics = new ArrayList<>(); | ||
PriorityQueue<Queue<Metric>> metricQueue = |
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.
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 : {}
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 ownStatisticSet
metric with theStatisticSetBuilder
class or specify a metric's method when putting it into themetricsLogger
and themetricsLogger
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.