New APIs to add/remove metric readers at run-time#4863
New APIs to add/remove metric readers at run-time#4863JP-MY wants to merge 2 commits intoopen-telemetry:mainfrom
Conversation
|
One thing to note is that the type of metric readers in sdk config is not very well defined (sequence) and is up to the users to decide; this makes it tricky to add or remove elements without checking for all known types (list, tuple, etc.). As a compromise I used |
opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement_consumer.py
Outdated
Show resolved
Hide resolved
| with self._lock: | ||
| if metric_reader in self._reader_storages: | ||
| _logger.warning("'%s' already registered!", metric_reader) | ||
| self._sdk_config.metric_readers += type( |
There was a problem hiding this comment.
This really isn't a good idea, we don't know that the type of self._sdk_config.metric_readers will even have a constructor that accepts an iterable.
There was a problem hiding this comment.
Any suggestions for a better alternative? given the Sequence type, this is the best I could come up with. It'd be better if the type is explicitly defined as something like Tuple | List but that would be a breaking API change
There was a problem hiding this comment.
I would recommend we remove the metric readers attribute entirely from that configuration object and let the measurement consumer accept a sequence of metric readers, allowing it to store them in whichever data structure it wants.
There was a problem hiding this comment.
I will defer to other contributors/maintainers on this one.
There was a problem hiding this comment.
The open comments were addressed with a second commit in case removing the metric_readers attribute from the SdkConfiguration object is not desired. @aabmass Please let me know if an alternative approach should be used.
opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py
Outdated
Show resolved
Hide resolved
| _logger.warning("'%s' has not been registered!", metric_reader) | ||
| self._reader_storages.pop(metric_reader, None) | ||
| metric_reader._set_collect_callback(None) | ||
| self._sdk_config.metric_readers = type( |
There was a problem hiding this comment.
Same deal here with the typing, we should not be doing this.
There was a problem hiding this comment.
To avoid some of the issues I presented, I would be in favor of removing the metric readers from the SdkConfiguration object entirely and instead make sure we pass metric readers explicitly to downstream components. @xrmx any thoughts here?
opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement_consumer.py
Outdated
Show resolved
Hide resolved
New APIs to add/remove metric readers at run-time Update measurement_consumer.py Removed return values
2 participants
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.
How Has This Been Tested?
Does This PR Require a Contrib Repo Change?
Checklist: