feat: Redesigns the InMemoryMetricExporter
#3257
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3257 +/- ##
=======================================
+ Coverage 80.8% 80.9% +0.1%
=======================================
Files 129 129
Lines 23203 23342 +139
=======================================
+ Hits 18750 18892 +142
+ Misses 4453 4450 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| .exporter | ||
| .get_finished_metrics() | ||
| .expect("metrics expected to be exported"); | ||
| f: impl FnOnce(&MetricData<T>), |
There was a problem hiding this comment.
Also generally I think the callback here is nice way of dealing with borrowing access and avoiding cloning to the stored data, but perhaps there is an option i'm missing. We eat a little bit of complexity in the impl here, but the usage is clean.
scottgerring
left a comment
scottgerring
left a comment
There was a problem hiding this comment.
Hey @emadpres thanks for the PR! 🤝
Some comments inline; lmk what you think
| pub fn build(self) -> InMemoryMetricExporter { | ||
| InMemoryMetricExporter { | ||
| metrics: Arc::new(Mutex::new(VecDeque::new())), | ||
| metrics: self.metrics.expect("Metrics must be provided"), |
There was a problem hiding this comment.
Should we return a Result here rather than paninicng?
| /// This exporter is useful for testing and debugging purposes. It stores | ||
| /// metric data in a `VecDeque<ResourceMetrics>`. Metrics can be retrieved | ||
| /// using the `get_finished_metrics` method. | ||
| /// metric data in a user-provided `Vec<ResourceMetrics>`, from which the | ||
| /// exported data can be retrieved as well. |
There was a problem hiding this comment.
I think we should be more explicit here. Something like this perhaps?
| /// This exporter is useful for testing and debugging purposes. It stores | |
| /// metric data in a `VecDeque<ResourceMetrics>`. Metrics can be retrieved | |
| /// using the `get_finished_metrics` method. | |
| /// metric data in a user-provided `Vec<ResourceMetrics>`, from which the | |
| /// exported data can be retrieved as well. | |
| /// This exporter is useful for testing and debugging purposes. It stores | |
| /// metric data in a user-provided `Arc<Mutex<Vec<ResourceMetrics>>>`. Users | |
| /// retain a clone of this `Arc` and can access the exported metrics directly | |
| /// by locking the mutex. |
| .get_finished_metrics() | ||
| .expect("metrics expected to be exported"); | ||
|
|
||
| let resource_metrics = test_context.resource_metrics.lock().unwrap(); |
There was a problem hiding this comment.
| let resource_metrics = test_context.resource_metrics.lock().unwrap(); | |
| let resource_metrics = test_context.resource_metrics.lock().expect("lock poisoned"); |
| .expect("metrics are expected to be exported."); | ||
| assert!( | ||
| resource_metrics[0].scope_metrics[0].metrics.len() == 1, | ||
| let resource_metrics = metrics.lock().unwrap(); |
There was a problem hiding this comment.
I suggest using expect("lock poisoned") for the rest of these too - it captures intent and gives a clearer error message if it ever fails
|
|
||
| assert!( | ||
| self.resource_metrics.len() == 1, | ||
| assert_eq!( |
| assert!(resource_metrics.is_empty(), "no metrics should be exported"); | ||
| } | ||
|
|
||
| fn get_aggregation<T: Number>( | ||
| fn with_aggregation<T: Number>( | ||
| &mut self, |
There was a problem hiding this comment.
I don't think this needs a mutable ref anymore
| - **Breaking** Removed `Default` and `Clone` implementation from `InMemoryMetricExporter`. | ||
| - **Breaking** `InMemoryMetricExporterBuilder` requires mandatory `metrics` field to be set via | ||
| `.with_metrics` method. |
There was a problem hiding this comment.
I think we should phrase this so the user knows what the impact on them is. Something like this perhaps?:
| - **Breaking** Removed `Default` and `Clone` implementation from `InMemoryMetricExporter`. | |
| - **Breaking** `InMemoryMetricExporterBuilder` requires mandatory `metrics` field to be set via | |
| `.with_metrics` method. | |
| - **Breaking** Redesigned `InMemoryMetricExporter` to accept user-provided storage instead of managing it internally. | |
| Users now pass an `Arc<Mutex<Vec<ResourceMetrics>>>` via `InMemoryMetricExporterBuilder::with_metrics()` and access | |
| exported data directly through that reference. This removes the need to clone the exporter and aligns with how | |
| production exporters receive their sink. `Default` and `Clone` implementations have been removed. | |
| [#3257](https://github.com/open-telemetry/opentelemetry-rust/pull/3257) | |
2 participants
Redesigned
InMemoryMetricExporterFixes #2725.
Changes
Updates
InMemoryMetricExporter(similar to what was suggested in #2649) to accept and use a user-specified data collection to store the metrics data, rather than creating one internally and returning it eventually. This makes the exporter more consistent with the 'production' exporters, which are given their sink by the user (e.g., by being passed a gRPC channel to write to) - rather than owning where the data goes. This also means we no longer need to be able to clone the exporter simply to access the data back out in unit tests.If we are happy with this approach, we can extend it to the other in memory exporters -
InMemoryLogExporterandInMemorySpanExporter.API Changes
InMemoryMetricExporternow must be instanciated throughInMemoryMetricExporterBuilderand it no longer hasDefaultandCloneimplementations.InMemoryMetricExporterBuilderhas the mandatorymetrics: Arc<Mutex<Vec<_>>>field to be set.Merge requirement checklist
added/updatedCHANGELOG.mdfiles updated for non-trivial, user-facing changes