New APIs to add/remove metric readers at run-time #4863
Conversation
|
|
Removed return values
|
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 |
| metric_reader._instrument_class_temporality, | ||
| metric_reader._instrument_class_aggregation, | ||
| ) | ||
| metric_reader._set_collect_callback(self.collect) |
There was a problem hiding this comment.
This _set_collect_callback call should ideally be made in the MeterProvider class.
| 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.
| with self._lock: | ||
| self._measurement_consumer.add_metric_reader(metric_reader) |
There was a problem hiding this comment.
We need to make sure the reader is properly added to _all_metric_readers and that the callback is set here instead of in the measurement consumer.
| metric_reader: "opentelemetry.sdk.metrics.export.MetricReader", | ||
| ) -> None: | ||
| with self._lock: | ||
| self._measurement_consumer.remove_metric_reader(metric_reader) |
There was a problem hiding this comment.
Similar to above, we need to make sure this reader is removed from _all_metric_readers. Furthermore, we will want to make sure that the reader is properly shutdown to prevent resource leaks.
| _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.
| ) | ||
| ), | ||
| resource=resource, | ||
| metric_readers=metric_readers, |
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?
| """Registers a new metric reader.""" | ||
| with self._lock: | ||
| if metric_reader in self._reader_storages: | ||
| _logger.warning("'%s' already registered!", metric_reader) |
There was a problem hiding this comment.
This should not be a warning, this really should through an exception or at the very least return early.
There was a problem hiding this comment.
exceptions can be more problematic so I think an early return makes more sense. Will update the PR.
There was a problem hiding this comment.
This is the pattern that is already established in the MeterProvider constructor. I figure that for consistency, the behavior should be the same in both places.
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: