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

Handle batch log processing in a dedicated background thread #2436

Merged
merged 13 commits into from
Dec 19, 2024

Conversation

ThomsonTan
Copy link
Contributor

Fixes #2066 . This will replace #2096, with the feedback taken there and ported the change from scratch to avoid merging conflicts.

The replaces BatchLogProcessor depending on async runtime will be extracted out to a separated type under feature flag, in case it is still needed in some cases.

Design discussion issue (if applicable) #

Changes

Please provide a brief description of the changes here.

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 December 16, 2024 22:44
name: "BatchLogProcessor.Shutdown.Timeout",
message = "BatchLogProcessor shutdown timed out."
);
LogError::ExportTimedOut(self.shutdown_timeout)
Copy link
Member

Choose a reason for hiding this comment

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

this is shutdown timing out.

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

Looks good overall, left few comments.
We need to make it similar to #2403, and keep existing batch processor under separate feature flag before merge.

@lalitb
Copy link
Member

lalitb commented Dec 17, 2024

This looks in good shape at first glance. Thanks for the PR. Will like to do one final round of review, and some tests before approval today. Meanwhile we need to -

  • Ensure to bring back the existing runtime based batch processor as experimental feature.
  • Test this for OTLP scenarios, which is important factor. I believe @cijothomas has explored the feasibility with periodic reader, I can spend some time to test this for our batch scenarios.
  • The internal optimizations (e.g. memory copy while pushing to queue) can be ongoing improvements beyond this PR.

@lalitb
Copy link
Member

lalitb commented Dec 17, 2024

I tested the following OTLP export scenarios with this PR:

  1. reqwest-blocking client: Works when invoked from both non-tokio-main and tokio-main contexts (with otlp: spawn thread to create blocking reqwest client #2431 changes). (expected behavior)
  2. reqwest client: Does not work when invoked from tokio-main (expected behavior).
  3. hyper client: Does not work when invoked from tokio-main (expected behavior).
  4. tonic client(tokio-main): Works when invoked from tokio-main (expected behavior).
  5. tonic client (non-tokio-main): Works when init_logs is invoked within the tokio context, keeping the context active (consistent with Validate tonic can be used in blocking scenarios/non-tokio context #2402). (expected behavior)

I believe this behavior is consistent with what we expect, and observed with the Periodic exporter, which is positive.

@scottgerring
Copy link
Contributor

We should be able to add this to the integration test suite easily enough!

Copy link

codecov bot commented Dec 18, 2024

Codecov Report

Attention: Patch coverage is 67.42424% with 86 lines in your changes missing coverage. Please review.

Project coverage is 76.1%. Comparing base (0605341) to head (a04683a).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
opentelemetry-sdk/src/logs/log_processor.rs 68.1% 83 Missing ⚠️
opentelemetry-sdk/src/logs/log_emitter.rs 0.0% 2 Missing ⚠️
opentelemetry-appender-tracing/src/layer.rs 0.0% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #2436     +/-   ##
=======================================
- Coverage   76.8%   76.1%   -0.8%     
=======================================
  Files        122     122             
  Lines      21851   22056    +205     
=======================================
- Hits       16797   16790      -7     
- Misses      5054    5266    +212     

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

@cijothomas cijothomas added the integration tests Run integration tests label Dec 18, 2024
Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

LGTM. Integration tests are not working, but that is good to be handled in a separate PR.
Left a non blocking comment.

@cijothomas cijothomas merged commit 938893c into open-telemetry:main Dec 19, 2024
18 of 21 checks passed
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.

[Feature]: remove async runtime dependency in batch processors
4 participants