Skip to content

fix: handle shutdown in logs exporter#3255

Merged
cijothomas merged 5 commits into
open-telemetry:mainfrom
stefanobaghino:2777-handle-shutdown
Dec 2, 2025
Merged

fix: handle shutdown in logs exporter#3255
cijothomas merged 5 commits into
open-telemetry:mainfrom
stefanobaghino:2777-handle-shutdown

Conversation

@stefanobaghino

@stefanobaghino stefanobaghino commented Nov 22, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes #2777

Changes

This follows the approach suggested here by @scottgerring:

The issue is in TonicLogsClient which uses tokio::sync::Mutex (async) but the LogExporter::shutdown_with_timeout() trait method is synchronous. This prevents calling .lock().await, so the current implementation has a TODO and just returns Ok() without actually shutting down the gRPC client.

Comparing shutdown on TonicMetricsClient , and in particular the type of the mutex used for the inner client and locked in the shutdown, seems to show a workable pattern.

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)

@linux-foundation-easycla

linux-foundation-easycla Bot commented Nov 22, 2025 •
edited
Loading

Copy link
Copy Markdown

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: cijothomas / name: Cijo Thomas (a92adb7)

@codecov

codecov Bot commented Nov 22, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.7%. Comparing base (53c9f47) to head (a92adb7).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
opentelemetry-otlp/src/exporter/tonic/logs.rs 0.0% 22 Missing ⚠️
opentelemetry-otlp/src/exporter/tonic/metrics.rs 0.0% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##            main   #3255     +/-   ##
=======================================
- Coverage   80.8%   80.7%   -0.1%     
=======================================
  Files        129     129             
  Lines      23203   23212      +9     
=======================================
  Hits       18750   18750             
- Misses      4453    4462      +9

☔ 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.

@stefanobaghino stefanobaghino changed the title Handle shutdown in logs exporter fix: handle shutdown in logs exporter Nov 22, 2025
@stefanobaghino stefanobaghino marked this pull request as ready for review November 22, 2025 14:10
@stefanobaghino stefanobaghino requested a review from a team as a code owner November 22, 2025 14:10
scottgerring
scottgerring approved these changes Nov 22, 2025
View reviewed changes

@scottgerring scottgerring left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks! LGTM

Comment thread opentelemetry-otlp/src/exporter/tonic/metrics.rs
}
None => Err(tonic::Status::failed_precondition(
"exporter is already shut down",
"metrics exporter is already shut down",

@scottgerring scottgerring Nov 22, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is helpful; I don't think the user would necessarily see which failed otherwise

scottgerring
scottgerring reviewed Nov 25, 2025
View reviewed changes
Comment thread opentelemetry-otlp/CHANGELOG.md Outdated
@stefanobaghino @scottgerring
Update opentelemetry-otlp/CHANGELOG.md
4df15de 
Co-authored-by: Scott Gerring <scottgerring@users.noreply.github.com>
@scottgerring

scottgerring commented Nov 25, 2025

Copy link
Copy Markdown
Member

@stefanobaghino that was a quick turnaround :D

cijothomas
cijothomas reviewed Nov 26, 2025
View reviewed changes
Comment thread opentelemetry-otlp/src/exporter/tonic/logs.rs
@stefanobaghino

stefanobaghino commented Dec 2, 2025

Copy link
Copy Markdown
Contributor Author

I'd be happy to take any additional feedback on this. If this is not a satisfactory approach, I also have nothing against closing this. Please let me know how to progress. Thanks.

@scottgerring

scottgerring commented Dec 2, 2025

Copy link
Copy Markdown
Member

I'd be happy to take any additional feedback on this. If this is not a satisfactory approach, I also have nothing against closing this. Please let me know how to progress. Thanks.

Hey @stefanobaghino should be fine; let's just wait on @cijothomas 's feedback! He ultimately holds the merge approval.

cijothomas
cijothomas approved these changes Dec 2, 2025
View reviewed changes

@cijothomas cijothomas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thank you!

@cijothomas cijothomas merged commit 483b420 into open-telemetry:main Dec 2, 2025
26 of 27 checks passed
@cijothomas cijothomas mentioned this pull request Dec 3, 2025
Closed
takumi-earth pushed a commit to earthlings-dev/opentelemetry-rust that referenced this pull request Jan 27, 2026
@stefanobaghino @scottgerring @cijothomas
fix: handle shutdown in logs exporter (open-telemetry#3255)
a34211b 
Co-authored-by: Scott Gerring <scottgerring@users.noreply.github.com>
Co-authored-by: Cijo Thomas <cijo.thomas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scottgerring scottgerring scottgerring approved these changes

@cijothomas cijothomas cijothomas approved these changes

Assignees

@scottgerring scottgerring

@stefanobaghino stefanobaghino

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

OTLP Stabilization: Handle shutdown in OTLP/gRPC

3 participants

@stefanobaghino @scottgerring @cijothomas