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

fix(metrics): flush upon a single metric 100th data point #1046

Merged
merged 1 commit into from
Mar 3, 2022
Merged

fix(metrics): flush upon a single metric 100th data point #1046

merged 1 commit into from
Mar 3, 2022

Conversation

knightjoel
Copy link
Contributor

@knightjoel knightjoel commented Feb 27, 2022

Description of changes:

Issue: #1062

Per the EMF spec, metric value arrays must not have more than 100 elements. This PR ensures that metrics are serialized when a single metric collects 100 values in addition to the existing behavior of serializing when 100 metrics are collected.

Note: as far as I can see, this same change needs to be brought to the other Powertools languages as well. I'm not equipped nor have the skill to submit a PR for any language other than Python.

Checklist

Breaking change checklist

RFC issue #:

  • Migration process documented
  • Implement warnings (if it can live side by side)

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

@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 27, 2022
@boring-cyborg
Copy link

boring-cyborg bot commented Feb 27, 2022

Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need.

@github-actions github-actions bot added the bug Something isn't working label Mar 2, 2022
@heitorlessa
Copy link
Contributor

cc @pankajagrawal16 @saragerion @flochaz - this is the bug we discussed on metric data points having a maximum of 100.

@heitorlessa heitorlessa linked an issue Mar 2, 2022 that may be closed by this pull request
35 tasks
@heitorlessa heitorlessa changed the title fix(metrics): emit metrics at 100th value fix(metrics): flush at a given metric 100th datapoint Mar 3, 2022
@heitorlessa
Copy link
Contributor

heitorlessa commented Mar 3, 2022

@jaredcnance thanks again for confirming this in the spec. Next release will have this fix on Lambda Powertools for Python.

and thank you very much @jonemo for spotting as well as fixing it. I didn't create an issue because it's in the Spec we missed

@heitorlessa heitorlessa changed the title fix(metrics): flush at a given metric 100th datapoint fix(metrics): flush upon a single metric 100th data point Mar 3, 2022
@pankajagrawal16
Copy link
Contributor

cc @pankajagrawal16 @saragerion @flochaz - this is the bug we discussed on metric data points having a maximum of 100.

@heitorlessa Since Powertools for Java internally uses https://github.com/awslabs/aws-embedded-metrics-java, this is fixed already in the lib as part of this issue via this PR. Let me know if I am missing something still that was fixed as part of this change. As far as I can see, Powertools for Java is covered.

@dreamorosi
Copy link
Contributor

dreamorosi commented Mar 3, 2022

@heitorlessa thanks for flagging this (@saragerion reached out to me).

TypeScript also uses the AWS EMF library but I am not sure if that's handled by the library or if we should implement the behaviour described in @pankajagrawal16's issue.

I've opened an issue on TypeScript's side to track the effort to investigate & potentially implement a fix/document the limit.

EDIT: we don't use the EMF library so we might need to implement it

@heitorlessa heitorlessa merged commit 22f8b9a into aws-powertools:develop Mar 3, 2022
@boring-cyborg
Copy link

boring-cyborg bot commented Mar 3, 2022

Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants