Move force_flush from tracer provider API to SDK #588 #658
Conversation
|
PR for #588 |
Codecov Report
@@ Coverage Diff @@
## main #658 +/- ##
=======================================
+ Coverage 69.6% 69.8% +0.1%
=======================================
Files 109 109
Lines 8824 8807 -17
=======================================
Hits 6150 6150
+ Misses 2674 2657 -17
Continue to review full report at Codecov.
|
|
/easycla |
|
@rust-approvers - PR is available for review. How do I mark this PR as available for review? |
|
I think I know why your CLA check failed. You have a commit associated with other account/email so that one also have to sign the CLA. |
|
@TommyCpp - have signed the cla from the account linked with the commit. i will retrigger cla bot again |
|
/easycla |
opentelemetry/src/global/trace.rs
Outdated
| @@ -370,6 +363,10 @@ impl GlobalTracerProvider { | |||
| provider: Arc::new(provider), | |||
| } | |||
| } | |||
|
|
|||
| fn force_flush(&self) -> Vec<TraceResult<()>> { | |||
| Vec::new() | |||
There was a problem hiding this comment.
We still need to ask the underlying TracerProvider to force push the spans instead of making it a noop here right?
There was a problem hiding this comment.
@TommyCpp - Yes. But since we removed force_flush from the TracerProvider trait, I didnt know where to place this pub fn
There was a problem hiding this comment.
Yeah, I guess we need to first figure out whether global is part of SDK or API. i.e should we allow users to provide their own implementation of TracerProvider
There was a problem hiding this comment.
I think we need to allow users to bring their own implementation and be able to use global. In that case, we need to downcast the Box<dyn TracerProvider>. That may cause some perf issue as it's dynamic reflection. @jtescher any thoughts?
There was a problem hiding this comment.
@TommyCpp - i believe we can ask the users to use TracerProvider::Create rather than providing option for users to create custom tracer providers because the issue linked was to decouple TracerProvider::Create from the underlying functionality.
Exposing ability to create custom TracerProviders also will include exposing traits that provide SDK functionality, which is opposite of what we have done as part of this issue.
There was a problem hiding this comment.
I believe the global is part of the API. As the API asks implementation to provide a way to register any TracerProvider in API.
Normally, the TracerProvider is expected to be accessed from a central place. Thus, the API SHOULD provide a way to set/register and access a global default TracerProvider
To that end, we should probably remove force_flush method from the global module. However, I believe many users want to use force_flush to make sure their spans are sent.
There was a problem hiding this comment.
Yeah let's remove it for now as it's not in the API spec
|
@rohithv1997 there was recently a split in the packages, you may need to rebase this |
|
@hdost - will do so. thanks! |
Co-authored-by: Rohith VenkataRamanaNanthan <rohith.v@thoughtworks.com>
|
Might be good to also update the changelog about the breaking change. |
Removed force_flush from TracerProvider trait in API and added it as pub fn in TracerProvider implementation in SDK