8000 refactor(counter analytics): enforce (namespace,name) pair uniqueness [DAT-145] by vittoriopolverino · Pull Request #12687 · localstack/localstack · GitHub
[go: up one dir, main page]

Skip to content

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

Merged

Conversation

vittoriopolverino
Copy link
Member
@vittoriopolverino vittoriopolverino commented May 29, 2025

Motivation

Currently, namespace is treated as an optional field, which can result in naming collisions between services.

Changes

  • Make namespace a required argument for CounterMetric* classes.
  • Ensure namespace is a mandatory parameter when instantiating Counter classes.

All counters were already defined with a namespace, even though it was optional, so this change shouldn’t cause any issues or break anything.

@vittoriopolverino vittoriopolverino added this to the 4.5 milestone May 29, 2025
@vittoriopolverino vittoriopolverino self-assigned this May 29, 2025
@vittoriopolverino vittoriopolverino added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label May 29, 2025
@vittoriopolverino vittoriopolverino changed the title refactor: enforce metric namespace uniqueness [DAT-145] refactor(analytics-counter): enforce namespace,name uniqueness [DAT-145] May 29, 2025
@vittoriopolverino vittoriopolverino changed the title refactor(analytics-counter): enforce namespace,name uniqueness [DAT-145] refactor(analytics,counter): enforce namespace,name uniqueness [DAT-145] May 29, 2025
@vittoriopolverino vittoriopolverino changed the title refactor(analytics,counter): enforce namespace,name uniqueness [DAT-145] refactor(analytics,counter): enforce namespace,name pair uniqueness [DAT-145] May 29, 2025
@vittoriopolverino vittoriopolverino changed the title refactor(analytics,counter): enforce namespace,name pair uniqueness [DAT-145] refactor(analytics,counter): enforce (namespace,name) pair uniqueness [DAT-145] May 29, 2025
@vittoriopolverino vittoriopolverino changed the title refactor(analytics,counter): enforce (namespace,name) pair uniqueness [DAT-145] refactor(analytics): enforce metric (namespace,name) pair uniqueness [DAT-145] May 29, 2025
@vittoriopolverino vittoriopolverino changed the title refactor(analytics): enforce metric (namespace,name) pair uniqueness [DAT-145] refactor(analytics): enforce (namespace,name) pair uniqueness [DAT-145] May 29, 2025
Copy link
github-actions bot commented May 29, 2025

Test Results - Preflight, Unit

21 580 tests  +1   19 928 ✅ +1   6m 16s ⏱️ +2s
     1 suites ±0    1 652 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit e8ebc9d. ± Comparison against base commit 79b46be.

♻️ This comment has been updated with latest results.

Copy link
github-actions bot commented May 29, 2025

Test Results (amd64) - Acceptance

7 tests  ±0   5 ✅ ±0   3m 34s ⏱️ +25s
1 suites ±0   2 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit e8ebc9d. ± Comparison against base commit 79b46be.

♻️ This comment has been updated with latest results.

Copy link
github-actions bot commented May 29, 2025

Test Results - Alternative Providers

597 tests   420 ✅  14m 57s ⏱️
  4 suites  177 💤
  4 files      0 ❌

Results for commit e8ebc9d.

♻️ This comment has been updated with latest results.

Copy link
github-actions bot commented May 29, 2025

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 43m 19s ⏱️ -5s
4 469 tests ±0  4 080 ✅ ±0  389 💤 ±0  0 ❌ ±0 
4 471 runs  ±0  4 080 ✅ ±0  391 💤 ±0  0 ❌ ±0 

Results for commit e8ebc9d. ± Comparison against base commit 79b46be.

♻️ This comment has been updated with latest results.

Copy link
github-actions bot commented May 29, 2025

Test Results (amd64) - Integration, Bootstrap

    5 files      5 suites   2h 20m 27s ⏱️
4 824 tests 4 282 ✅ 542 💤 0 ❌
4 830 runs  4 282 ✅ 548 💤 0 ❌

Results for commit e8ebc9d.

♻️ This comment has been updated with latest results.

@vittoriopolverino vittoriopolverino marked this pull request as ready for review May 29, 2025 14:31
@vittoriopolverino vittoriopolverino requested a review from thrau as a code owner May 29, 2025 14:31
@vittoriopolverino vittoriopolverino requested a review from joe4dev May 29, 2025 14:34
@vittoriopolverino vittoriopolverino changed the title refactor(analytics): enforce (namespace,name) pair uniqueness [DAT-145] refactor(counter analytics): enforce (namespace,name) pair uniqueness [DAT-145] May 29, 2025
@vittoriopolverino vittoriopolverino marked this pull request as draft May 30, 2025 13:12
@vittoriopolverino vittoriopolverino marked this pull request as ready for review May 30, 2025 13:35
Copy link
Member
@joe4dev joe4dev left a 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)
Copy link
Member

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 {
Copy link
Member

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

Copy link
Member Author
@vittoriopolverino vittoriopolverino Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, exactly 👍🏼

@vittoriopolverino
Copy link
Member Author

notice: The PR also introduces new typings in addition to validating name uniqueness within the namespace context.

Thanks for pointing that out 🙏🏼 I’ve added a note about the new typings to the PR description as well

@vittoriopolverino vittoriopolverino merged commit 0b05553 into master Jun 3, 2025
57 checks passed
@vittoriopolverino vittoriopolverino deleted the DAT-145-enforce-metric-namespace-uniqueness branch June 3, 2025 09:33
@vittoriopolverino
Copy link
Member Author

FYI @localstack/data-engineers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0