Skip to content

Metric Value List can go above MetricDefinition limits, causing CW missing metrics #72

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

Closed
axeliux opened this issue Mar 29, 2021 · 5 comments

Comments

@axeliux
Copy link

axeliux commented Mar 29, 2021

Apparently if you call MetricsLogger.putMetric() more than 100 times, it will keep adding values to the metric causing to reach out of it's limits. This would cause missing metrics as the log entry would not be processed.

Reference:

Metrics— An array of MetricDefinition objects. This array MUST NOT contain more than 100 MetricDefinition objects.

https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch_Embedded_Metric_Format_Specification.html

@axeliux
Copy link
Author

axeliux commented Mar 29, 2021

Maybe we are not limiting the size of the metric value at all.

@msailes
Copy link
Collaborator

msailes commented Mar 30, 2021

It's not just the number of metrics, it's all validation.

Size of numbers, infinity...

@axeliux
Copy link
Author

axeliux commented Mar 30, 2021

I wonder if instead of having a new ArrayList<> to store the metric values, we could have something like ```EvictingQueue`` of a fixed size 100.

I understand that we could lose some metrics values, but given the current 100 limitation, it would be better than not having a metric at all.

https://guava.dev/releases/snapshot/api/docs/com/google/common/collect/EvictingQueue.html

@axeliux axeliux changed the title Metric Value List can go above it's limits. Metric Value List can go above its limits. Mar 30, 2021
@axeliux axeliux changed the title Metric Value List can go above its limits. Metric Value List can go above MetricDefinition limits, causing CW missing metrics Mar 30, 2021
@axeliux
Copy link
Author

axeliux commented Mar 30, 2021

Not quite the same issue, but this handle a limit on the number of metrics we can process per log:

https://github.com/awslabs/aws-embedded-metrics-java/pull/42/files/dc47697d0a860c46dbbc06669558452ffa28e117

@jaredcnance
Copy link
Member

jaredcnance commented Mar 30, 2021

This problem will likely be handled in a similar way as the one you linked. We will continue to accept additional metrics and at serialization time, split it across multiple events. You can see how this was implemented for python. Regarding @msailes's comment, yes we would like to add improved validation to ensure we only accept valid dimensions, metric names, namespaces, and metric values. Eventually, we'd also like to support client-side aggregation which would allow you to include StatisticSets and Values/Counts similar to MetricDatums.

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

No branches or pull requests

4 participants