Skip to content
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 force_flush to LogExporter #2276

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

ThomsonTan
Copy link
Contributor

Fixes #2261
Design discussion issue (if applicable) #

Changes

Added the force_flush interface to LogExporter and call it in the LogProcessor, right after flushing the export queue.

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)

@ThomsonTan ThomsonTan requested a review from a team as a code owner November 5, 2024 22:01
Copy link

codecov bot commented Nov 5, 2024

Codecov Report

Attention: Patch coverage is 96.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 79.4%. Comparing base (6d1a765) to head (71d4b40).

Files with missing lines Patch % Lines
opentelemetry-sdk/src/logs/log_processor.rs 95.4% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main   #2276   +/-   ##
=====================================
  Coverage   79.4%   79.4%           
=====================================
  Files        123     123           
  Lines      21479   21501   +22     
=====================================
+ Hits       17068   17089   +21     
- Misses      4411    4412    +1     

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

@lalitb lalitb added the integration tests Run integration tests label Nov 6, 2024
@lalitb
Copy link
Member

lalitb commented Nov 7, 2024

Can we also add a unit-test for batch processor, with mock-exporter, to validate that the exporter's force flush get's called.

@lalitb
Copy link
Member

lalitb commented Nov 28, 2024

We may need to add both sync and async versions for provider / processors and exporter for shutdown() and force_flush(), and so different pipelines based on what is invoked. I am looking into those aspects, but this PR change would be still required. @cijothomas @utpilla @TommyCpp if anyone has concern here.

/// such as when using some FaaS providers that may suspend the process after
/// an invocation, but before the exporter exports the completed logs.
///
/// This function SHOULD complete or abort within some timeout. This function can be
Copy link
Contributor

@utpilla utpilla Nov 28, 2024

Choose a reason for hiding this comment

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

Let's update the comments here to reflect the actual trait method definition instead of putting the spec guidelines for this method.

We could put something like:

/// This method will block the current thread until all pending log records are exported. If the export was not
/// successful, an error is returned.

/// a callback or an event. OpenTelemetry client authors can decide if they want to
/// make the flush timeout configurable.
fn force_flush(&mut self) -> ExportResult {
Ok(())
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should not have a default implementation for this method. Otherwise, we could run into a situation where an exporter user goes through the documentation and calls force_flush with certain expectations that haven't been agreed upon by the exporter author.

It looks like we have a default implementation for a few other methods as well such as shutdown and set_resource. We should revisit them based on what we decide.

Copy link
Member

Choose a reason for hiding this comment

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

Otherwise, we could run into a situation where an exporter user goes through the documentation and calls force_flush with certain expectations that haven't been agreed upon by the exporter author.

If the exporter author does not provide an implementation for flush, then they can document that right? Or the doc for Provider can be updated to merely state that it'll invoke flush on the processor, and the same for processor can state they will invoke the flush for exporter.

Default implementation seems reasonable to me, as not every exporter need to do something for flush.

@@ -278,7 +282,8 @@ impl<R: RuntimeChannel> BatchLogProcessor<R> {
&timeout_runtime,
logs.split_off(0),
)
.await;
.await
.and(exporter.as_mut().force_flush());
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this? export_with_timeout already calls exporter.export which is what is expected from force_flush.

@lalitb
Copy link
Member

lalitb commented Dec 27, 2024

@ThomsonTan can we address the comments, and resolve the conflicts, to be eventually merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration tests Run integration tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exporter::force_flush() interface missing for Logs
5 participants