-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
refactor(counter analytics): enforce (namespace,name) pair uniqueness [DAT-145] #12687
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
refactor(counter analytics): enforce (namespace,name) pair uniqueness [DAT-145] #12687
Conversation
Test Results - Alternative Providers597 tests 420 ✅ 14m 57s ⏱️ Results for commit e8ebc9d. ♻️ This comment has been updated with latest results. |
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 2h 20m 27s ⏱️ Results for commit e8ebc9d. ♻️ This comment has been updated with latest results. |
…lex datatypes in collect() functions
…Collection in metrics module
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.
Good unit test coverage ✨
notice: The PR also introduces new typings in addition to validating name uniqueness within the namespace context.
|
||
self._registry[metric.name] = metric | ||
registry_unique_key = MetricRegistryKey(namespace=metric.namespace, name=metric.name) |
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.
notice: This is the main implementation ensuring uniqueness of the metric name
within the namespace
👍
""" | ||
Collects all registered metrics. | ||
""" | ||
return { |
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.
notice: I guess this API change from dict to list makes the collection more reusable and delegates JSON conversion to MetricPayload>as_dict
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.
Yep, exactly 👍🏼
Thanks for pointing that out 🙏🏼 I’ve added a note about the new typings to the PR description as well |
FYI @localstack/data-engineers |
Motivation
Currently, namespace is treated as an optional field, which can result in naming collisions between services.
Changes
All counters were already defined with a namespace, even though it was optional, so this change shouldn’t cause any issues or break anything.