Skip to content

Conversation

@nss10
Copy link
Contributor

@nss10 nss10 commented Aug 10, 2025

Description about what this pull request does.

  • This PR updates the increment_counter method in cdispyutils/metrics.py to register counters with the instance’s CollectorRegistry instead of the default global registry.

Why this change is needed:

  • Without specifying registry=self._registry, the Counter is registered in the default global registry provided by prometheus_client.
  • When the same process (e.g., during tests, multi-threaded code, or repeated initialization in a long-running service) tries to create a counter with the same name in the default registry, Prometheus raises the following error:
ValueError: Duplicated timeseries in CollectorRegistry
  • More info : here
  • By passing registry=self._registry, each MetricsHandler instance maintains its own isolated CollectorRegistry. This prevents accidental clashes with other metrics in the default registry.
  • Why the error never happened in production but occurred in unit tests
    1. Production services usually initialize metrics once
      • In a live deployment, the process starts, creates its metrics in the default registry, and keeps running. No duplicate registration occurs unless the process restarts.
    2. Unit tests often create multiple instances in the same process
      • Many tests spin up fresh MetricsHandler objects without restarting Python.
      • If each test case creates counters with the same name in the global registry, duplicates are inevitable.

Please make sure to follow the DEV guidelines before asking for review.

New Features

Breaking Changes

Bug Fixes

  • Fix: Prevent duplicated timeseries in Prometheus metrics by using instance-specific registry

Improvements

Dependency updates

Deployment changes

@github-actions
Copy link

The style in this PR agrees with black. ✔️

This formatting comment was generated automatically by a script in uc-cdis/wool.

george42-ctds
george42-ctds previously approved these changes Sep 5, 2025
Copy link

@george42-ctds george42-ctds left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link

@george42-ctds george42-ctds left a comment

Choose a reason for hiding this comment

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

Looks good!

@nss10 nss10 merged commit f259c04 into master Sep 8, 2025
4 checks passed
@nss10 nss10 deleted the update_registry_in_constructors branch September 8, 2025 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants