Skip to content

make force_flush() in PushMetricExporter synchronous #2663

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

Merged
merged 6 commits into from
Feb 15, 2025

Conversation

gruebel
Copy link
Member

@gruebel gruebel commented Feb 14, 2025

Changes

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)

@gruebel gruebel requested a review from a team as a code owner February 14, 2025 08:57
Copy link

codecov bot commented Feb 14, 2025

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 79.1%. Comparing base (dbd44a3) to head (b201972).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
opentelemetry-sdk/src/metrics/periodic_reader.rs 0.0% 2 Missing ⚠️
opentelemetry-otlp/src/metric.rs 0.0% 1 Missing ⚠️
...pentelemetry-sdk/src/metrics/in_memory_exporter.rs 0.0% 1 Missing ⚠️
opentelemetry-stdout/src/metrics/exporter.rs 0.0% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main   #2663   +/-   ##
=====================================
  Coverage   79.1%   79.1%           
=====================================
  Files        122     122           
  Lines      22560   22560           
=====================================
  Hits       17858   17858           
  Misses      4702    4702           

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

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

LGTM, the changelog need to be moved.

@lalitb
Copy link
Member

lalitb commented Feb 14, 2025

While this change is in right direction, and only affect the custom exporters, I am still thinking about the breaking changes with deprecating, if possible (specifically that we are 0.28 was RC), and concerns coming from #2645. Though it won't be trivial in this case, as we want to eventually keep the function name same. Which means, we need to deprecate the existing and bring new sync method under feature flag. Just for discussions - not to block this PR.

@cijothomas
Copy link
Member

specifically that we are 0.28 was RC

Good point! Same issue in #2654 too.
Here's my thinking:
RC has much higher bar to make a breaking change. In this PR and the one linked above, I consider this is okay to do. The disruption is very minimal, so it is not worth the effort to do this in back-compat way (if there is a way!)

(Things would have been different if 1.0 was released - we absolutely cannot make this change or the one from #2654 post 1.0.)

@cijothomas cijothomas closed this Feb 14, 2025
@cijothomas cijothomas reopened this Feb 14, 2025
@cijothomas cijothomas merged commit bc931b1 into open-telemetry:main Feb 15, 2025
20 of 21 checks passed
@gruebel gruebel deleted the sync-flush branch February 15, 2025 10:40
cijothomas added a commit to cijothomas/opentelemetry-rust that referenced this pull request Feb 18, 2025
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.

3 participants