Skip to content

refactor: PushMetricExporter interface #2921

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

fraillt
Copy link
Contributor

@fraillt fraillt commented Apr 9, 2025

Changes

Changed PushMetricExporer::export signature. fn export(&self, metrics: ResourceMetricsRef<'_>) -> .... It is very similar to LogExporter::export, with one key difference: I do not expose iter() method on items (metrics) (to create iterator), but directly provide Iterator over data. This means, that it's only possible to iterate everything once.
However, since I implement standard Iterator trait we still need to have all the items stored somewhere, so I'm not sure how much we can optimize internal implementation...

If I would change Iterator to LendingIterator, it would be possible to collect/export all metrics without any memory allocation at all!

So I have few questions for this PR:

  • Do we want to be able to implement collect/export without memory allocation (e.g. have one Metric instance on the stack (iterator itself) ?)
  • Regarding naming... if we optimize internal implementation it might be that ResourceMetrics and ScopeMetrics will not be present anymore, so I could use these names here (it sounds better than ResourceMetricRef...). In general, any suggestion for better naming is welcome :)

P.S. I wasn't able to provide set_resource(&mut self, resource: &Resource) on the PushMetricExporter (without making PeriodicReader very inefficient, and disabling async version of it) before MetricReader is refactored/improved #2917.

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

Sorry, something went wrong.

@fraillt fraillt requested a review from a team as a code owner April 9, 2025 06:14
Copy link

codecov bot commented Apr 9, 2025

Codecov Report

Attention: Patch coverage is 60.60606% with 104 lines in your changes missing coverage. Please review.

Project coverage is 81.2%. Comparing base (4ce7655) to head (fc6205d).

Files with missing lines Patch % Lines
opentelemetry-stdout/src/metrics/exporter.rs 0.0% 45 Missing ⚠️
opentelemetry-proto/src/transform/metrics.rs 0.0% 18 Missing ⚠️
opentelemetry-sdk/src/metrics/pipeline.rs 7.1% 13 Missing ⚠️
opentelemetry-sdk/src/metrics/periodic_reader.rs 82.0% 7 Missing ⚠️
opentelemetry-sdk/src/metrics/exporter.rs 86.6% 6 Missing ⚠️
.../src/metrics/periodic_reader_with_async_runtime.rs 81.4% 5 Missing ⚠️
opentelemetry-otlp/src/exporter/http/mod.rs 0.0% 2 Missing ⚠️
opentelemetry-otlp/src/exporter/tonic/metrics.rs 0.0% 2 Missing ⚠️
opentelemetry-sdk/src/metrics/mod.rs 90.4% 2 Missing ⚠️
opentelemetry-otlp/src/exporter/http/metrics.rs 0.0% 1 Missing ⚠️
... and 3 more
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #2921     +/-   ##
=======================================
- Coverage   81.3%   81.2%   -0.2%     
=======================================
  Files        126     126             
  Lines      24254   24334     +80     
=======================================
+ Hits       19736   19774     +38     
- Misses      4518    4560     +42     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

fraillt added 3 commits April 12, 2025 10:18
MetricReader.
* Renamed ResourceMetricRef->ResourceMetric and made that
iteration would borrow an item, which will allow improving
underlying metric collection without any allocations in the future
* Updated CHANGELOG.md
@fraillt
Copy link
Contributor Author

fraillt commented Apr 13, 2025

As discussed in slack with @cijothomas, the main change is that export doesn't allocate anything, but exposes borrowed data directly to PushMetricExporter::export. Full implementation is in expoter.rs file.
Old implementation with SdkProducer is still used by ManualReader, so it hasn't changed.

There's one thing worth discussing.
Current implementation exposes already borrowed data (which is behind MutexGuard) and simply provides a way to iterate over it. Because we already provide borrowed data to export function, it cannot be sent, so periodic_reader_with_async_runtime.rs is basically loose it's purpose (I cannot call export, because Runtime::spawn requires future to be Send.)

One option is to lock pipeline inside export function, but it's less user friendly experience, because we need to write extra boilerplate just to start collecting...

let mut collector = metrics.start_collect(); // acquire lock, which is borrowed from metrics
let mut scope_iter = collector.iter(); // acquire iterator, which is borrowed from collector (underlying structure is hashmap, so it's not really possible to iterate it efficiently without creating iterator.)
while let Some(scope_metric) = scope_iter.next_scope_metric() {
...
}

We could improve end user experience by removing one call, but that would require using self-referential struct (with Pin, and extra complexity to handle Drop properly, even though it would be hidden from end user, it's still unnecessary unsafe code).

Current approach is more straight forward

while let Some(scope_metric) = scope_iter.next_scope_metric() {
...
}

More importantly, since we acquire the lock before calling export, I’m fairly confident that with the improvements from MetricReader #2917, we could switch to export(&mut self, ...). This would be beneficial if the exporter wants to cache data before exporting—for example, OTLP exporters could reuse allocated data instead of rebuilding the request object each time.

For these reasons, I prefer sticking with the current solution for now and adding export(&mut self) in the future.

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.

None yet

1 participant