-
Notifications
You must be signed in to change notification settings - Fork 785
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
Add exporter ForceFlush #2525
Add exporter ForceFlush #2525
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2525 +/- ##
==========================================
- Coverage 80.58% 80.55% -0.03%
==========================================
Files 254 254
Lines 8507 8515 +8
==========================================
+ Hits 6855 6859 +4
- Misses 1652 1656 +4
|
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.
Public API files needed.
Does it have to be this PR? There are many Metrics APIs not included in the API list. I can do it in a separate PR. |
OpenTelemetry.Trace.BatchExportActivityProcessorOptions.BatchExportActivityProcessorOptions() -> void | ||
override OpenTelemetry.BaseExportProcessor<T>.OnForceFlush(int timeoutMilliseconds) -> bool | ||
override OpenTelemetry.BatchExportProcessor<T>.Dispose(bool disposing) -> void |
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.
Note that the Dispose
was added by some previous PR(s).
yea we can add entire Metrics API separately. I was only asking since this PR is not related to metrics alone. |
} | ||
catch (Exception ex) | ||
{ | ||
OpenTelemetrySdkEventSource.Log.SpanProcessorException(nameof(this.ForceFlush), ex); |
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 should probably be something generic rather than about spans - also this is an exporter not a processor. A lot of the events in OpenTelemetrySdkEventSource
are span-centric right now... a holistic review of the usage of OpenTelemetrySdkEventSource
could be useful - I can create an issue.
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.
Yes. this is something we need to tackle surely.
Related to this: #1529 as well
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 should probably be something generic rather than about spans - also this is an exporter not a processor. A lot of the events in
OpenTelemetrySdkEventSource
are span-centric right now... a holistic review of the usage ofOpenTelemetrySdkEventSource
could be useful - I can create an issue.
+1, I also want to solve this problem. The current internal diagnostic logs are misleading and most of them are not actionable.
Fixes #2523.
Changes
Added
ForceFlush
to exporters.For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes