fix(metrics): raise SchemaValidationError for >8 metric dimensions #1240
Conversation
pull-request-size
bot
added
the
size/XS
pull-request-size
bot
removed
the
size/XS
boring-cyborg
bot
added
the
documentation
pull-request-size
bot
added
the
size/S
|
I was wondering if this is not a breaking change. If EMF fails silently, we might have customers sending more than the allowed dimensions in theirs runtime (but failing silently). As soon as they bump the powertools version, their code will start to blowup at runtime instead of just not producing metrics dimensions. |
| @@ -178,7 +178,7 @@ def serialize_metric_set( | |||
| metadata = self.metadata_set | |||
|
|
|||
| if self.service and not self.dimension_set.get("service"): | |||
There was a problem hiding this comment.
Not specific from this PR, but won't the condition not self.dimension_set.get("service") blow up if the key is not present? Shouldn't it be something like not self.dimension_set.get("service", "")?
There was a problem hiding this comment.
no, because .get will return None as the default sentinel value, hence why it's suitable within the if condition here
There was a problem hiding this comment.
technically, and self.dimension_set.get("service") is not None would also work here ;-)
>>> a = {}
>>> not a.get("blah")
True
>>> a.get("blah") is None
True
My personal take is that this falls under a bug - we should've been catching that before (expected behaviour), and not fixing it can lead to data loss. If we were changing an expected behaviour then I'd consider it a breaking change. |
I don't think they are mutually exclusive, but I'm not familiar with the vision you have for this project regarding these kind of changes. We might have different takes on how a breaking change is defined, but, in any case, that would be a whole new discussion that does not belong here. The PR changes look good. ✅ |
|
absolutely @dnlopes, I'd go as far as suggesting it'd be great to jump on a call to discuss this. While we're making strides in creating a maintainers playbook and improve our definitions, there's so much more that we could do. Gonna proceed to releasing it now as a patch. |
heitorlessa
changed the title
|
Breaking changes for sure. Needed. But still a breaking change. |
Issue number: #1239
Summary
Changes
Lambda Powertools adds a
serviceCloudWatch Metric Dimension on top of user-defined dimensions. If customers add 9 user-defined dimensions, Powertools were previously addingservicedimension directly into the dictionary making EMF to not process these metrics - EMF is an async process managed by CloudWatch Logs and it fails silently, all validation must happen on the client side for any feedback.This PR addresses our documentation stating that the limit is 8 and ensures our
servicedimension addition goes through the same user-defined dimension validation process.User experience
Checklist
If your change doesn't seem to apply, please leave them unchecked.
Is this a breaking change?
RFC issue number:
Checklist:
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.