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
joe4dev
left a comment
There was a problem hiding this comment.
Good unit test coverage ✨
notice: The PR also introduces new typings in addition to validating name uniqueness within the namespace context.
| raise ValueError("Metric 'namespace' must be defined and non-empty.") | ||
|
|
||
| self._registry[metric.name] = metric | ||
| registry_unique_key = MetricRegistryKey(namespace=metric.namespace, name=metric.name) |
There was a problem hiding this comment.
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.
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.
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.