-
Notifications
You must be signed in to change notification settings - Fork 516
fix: Adds shutdown function for spans #2812
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
base: main
Are you sure you want to change the base?
fix: Adds shutdown function for spans #2812
Conversation
4009de1
to
7a58462
Compare
After taking some inspirations from the logs PR https://github.com/open-telemetry/opentelemetry-rust/pull/2764/files |
1ed73b1
to
5be511b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2812 +/- ##
=======================================
- Coverage 80.9% 80.8% -0.1%
=======================================
Files 124 124
Lines 23726 23738 +12
=======================================
+ Hits 19197 19200 +3
- Misses 4529 4538 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
None => Err(OTelSdkError::AlreadyShutdown), // `inner` was already `None`, meaning it's already shut down. | ||
} | ||
fn shutdown(&self) -> OTelSdkResult { | ||
// match self.inner.take() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to make self mutable for this to work, shutdown for tonic needs to be implemented according to #2777
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a TODO here linking to the issue.
@cijothomas does this also require a change like this? https://github.com/open-telemetry/opentelemetry-rust/pull/2764/files#diff-7e030c09778110d005a4f1b3030b8318bffa2ca09a7d897877ec71a495a971f4R49 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Please add changelog entry marking breaking change for custom processor/exporter authors
a5430b7
to
ffd3244
Compare
opentelemetry-otlp/CHANGELOG.md
Outdated
@@ -17,6 +17,10 @@ | |||
`Error` which contained many variants unrelated to building an exporter, the | |||
new one returns specific variants applicable to building an exporter. Some | |||
variants might be applicable only on select features. | |||
- *Breaking* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not a breaking change for OTLP Exporter, just fixing bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to list the breaking change in the SDK changelog. (You can check the previous entries there that did similar breaking changes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://github.com/open-telemetry/opentelemetry-rust/pull/2812/files
Blocking only to prevent merge while changelog is fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, once changelog is fixed.
Signed-off-by: Shivanshu Raj Shrivastava <[email protected]>
Signed-off-by: Shivanshu Raj Shrivastava <[email protected]>
Signed-off-by: Shivanshu Raj Shrivastava <[email protected]>
99053b4
to
ca48261
Compare
Fixes: #2779