-
Notifications
You must be signed in to change notification settings - Fork 437
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
[WIP] Remove usage of futures_executor::block_on
in WASM targets
#2048
base: main
Are you sure you want to change the base?
Conversation
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2048 +/- ##
=======================================
+ Coverage 77.1% 77.2% +0.1%
=======================================
Files 123 123
Lines 21103 21246 +143
=======================================
+ Hits 16275 16408 +133
- Misses 4828 4838 +10 ☔ View full report in Codecov by Sentry. |
@@ -57,13 +58,13 @@ pub trait LogProcessor: Send + Sync + Debug { | |||
/// # Parameters | |||
/// - `record`: A mutable reference to `LogData` representing the log record. | |||
/// - `instrumentation`: The instrumentation library associated with the log record. | |||
fn emit(&self, data: &mut LogRecord, instrumentation: &InstrumentationLibrary); | |||
async fn emit(&self, data: &mut LogRecord, instrumentation: &InstrumentationLibrary); |
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.
There is already a concern about forcing async
everywhere, even in places where it won't do any good, but actually lowers performance. #2031
Agree that the blocking is an issue that needs to be solved, but I am not convinced that this change is an acceptable solution :(
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.
Blocking the runtime in this method is very dicey and will at best have significant negative performance implications and at worst lock things up entirely, and should only be done if no other solutions are present.
Given that this has been observed causing production issues for folks, it's probably best to just take the approach that is correct and optimize it (if benchmarks even suggest that there is a problem) rather than do this trick.
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.
Besides, if AsyncDrop does land, it's very easy to just undo this change and make the drop async
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.
I am not sure I follow. What is the negative performance implication being referred to here? To be clear, BatchLogRecordProcessor
is the one that requires a runtime and benefit from async, but that is not the only way to export logs. We have exporters to export to Windows ETW, Linux user-events, where exporting is done synchronously, inline within the logging call itself, and has no need for async and runtimes.
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.
Agree with @cijothomas. Async runtime was an optional dependency till now - only required for batch*processor (in logs and traces) and periodic-reader (in metrics). And there was plan to have sync and async version of LogExporter::export() method to make it more complete. However, with this change, the runtime would be always required, even for custom synchronous processors.
See issue #2047 for discussion about this PR
Changes
This PR contains 5 changes:
LogExporter::export
function return a future that doesn't capture self's lifetime, this is similar to whatSpanExporter
does. This will be important for a follow up commitLogProcessor
async, this is where the first commit is relevant. As it allows me to get around std MutexGuard not being Syncblock_on
block_on
is used explicitly, this however should instead be done by making these two changed methods async, but I was unsure how to proceed with the fallout of that change on the codebase, so settled for this.Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes