Skip to content

New APIs to add/remove metric readers at run-time#4863

Open
JP-MY wants to merge 2 commits intoopen-telemetry:mainfrom
JP-MY:main
Open

New APIs to add/remove metric readers at run-time#4863
JP-MY wants to merge 2 commits intoopen-telemetry:mainfrom
JP-MY:main

Conversation

@JP-MY
Copy link

@JP-MY JP-MY commented Dec 18, 2025 •
edited
Loading

Description

This change adds two public functions to MeterProvider that allow registering and deleting metric readers at run-time.

Fixes #4818

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

  • It has been tested via new unit test added to ensure correct behavior for addition and removal of metric readers at run-time.

Does This PR Require a Contrib Repo Change?

  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@JP-MY JP-MY requested a review from a team as a code owner December 18, 2025 14:20
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 18, 2025 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

herin049
herin049 reviewed Jan 22, 2026
View reviewed changes
aabmass
aabmass reviewed Feb 5, 2026
View reviewed changes
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, sorry for the delay here.

@JP-MY JP-MY force-pushed the main branch 2 times, most recently from ef20f0e to 9bb1567 Compare February 13, 2026 07:58
@JP-MY
Copy link
Author

JP-MY commented Feb 14, 2026 •
edited
Loading

@aabmass @herin049 I'm not sure why the lint is failing for protected access, I can see the same protected access in the same file. Should I add a pylint: disable= rule or is there a config for this that I need to update? also the contrib test fails don't seem related (failed to get a specific commit?) but I'm not sure if I messed something up to cause that or not.

aabmass
aabmass approved these changes Feb 17, 2026
View reviewed changes
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the concurrency issues, LGTM!

opentelemetry-sdk/tests/metrics/test_metrics.py
Copy link
Member

@aabmass aabmass Feb 17, 2026

Choose a reason for hiding this comment

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

As a follow up to the thread safety issue you fixed, can you add a multithread test for this in a new integration test file? Here are some examples to make this pretty straightforward.

Copy link
Author

@JP-MY JP-MY Mar 19, 2026

Choose a reason for hiding this comment

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

I added a test which is deterministic (forces race conditions that would cause issues) but I'm not sure how great it is. I can also add a test with many threads that try to add / remove metric readers and assert counts (number of metric readers changed during iteration without raising an error) but I'm not sure how useful that is

aabmass
aabmass reviewed Feb 17, 2026
View reviewed changes
opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py
)
return self._meters[info]

def add_metric_reader(
Copy link
Member

@aabmass aabmass Feb 17, 2026

Choose a reason for hiding this comment

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

@xrmx Any concerns on new public API here?

@tammy-baylis-swi tammy-baylis-swi moved this to Reviewed PRs that need fixes in Python PR digest Feb 26, 2026
@tammy-baylis-swi tammy-baylis-swi moved this from Reviewed PRs that need fixes to Approved PRs that need fixes in Python PR digest Feb 26, 2026
@JP-MY JP-MY force-pushed the main branch 2 times, most recently from 99fff87 to 8a5c6ef Compare March 19, 2026 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aabmass aabmass aabmass approved these changes

@herin049 herin049 herin049 left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

Python PR digest
Status: Approved PRs that need fixes

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Exposing public APIs to for addition and removal of metric readers at run-time

4 participants

@JP-MY @aabmass @herin049 @tammy-baylis-swi