Skip to content

Conversation

@ion-elgreco
Copy link
Collaborator

Description

The description of the main changes of your pull request

Related Issue(s)

Documentation

@codecov
Copy link

codecov bot commented Oct 23, 2025

Codecov Report

❌ Patch coverage is 91.66667% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.74%. Comparing base (acfaee5) to head (af21bd5).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/core/src/kernel/snapshot/mod.rs 70.00% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3894      +/-   ##
==========================================
- Coverage   73.75%   73.74%   -0.01%     
==========================================
  Files         151      151              
  Lines       39374    39374              
  Branches    39374    39374              
==========================================
- Hits        29040    29038       -2     
- Misses       9021     9025       +4     
+ Partials     1313     1311       -2     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

roeap
roeap previously approved these changes Oct 23, 2025
@chiragjn
Copy link

chiragjn commented Oct 23, 2025

I tried this PR, I still see disconnection after Snapshot::try_new_with_engine

I have forked kernel and added decorators on for_snapshot and read_metadata

image

I am mostly fumbling around at this point and my observation is for spawn_blocking, I need to wrap like this to make it work

let dispatch = tracing::dispatcher::get_default(|d| d.clone());
let span = tracing::Span::current();
let result = spawn_blocking(move || {
    tracing::dispatcher::with_default(&dispatch, || {
        let _enter = span.enter();
        // actual logic

@ion-elgreco
Copy link
Collaborator Author

@chiragjn hmm I see, I've updated the code to reflect that now, can you try again? I guess the key thing is that .instrument only enters the current span when polling the future but the dispatcher also needs to be re-used which doesn't happen with tracing-futures :(

@ion-elgreco ion-elgreco force-pushed the feat/tracing-spans-across-threads branch from 2893222 to 5cb8e3a Compare October 23, 2025 10:50
@chiragjn
Copy link

chiragjn commented Oct 23, 2025

Tried the PR again, works fine 🎉

I think a few places are missing - replay_for_scan_metadata from kernel shows up disconnected

I believe these are from scan.rs

let blocking_iter = move || {
for res in inner.scan_metadata(engine.as_ref())? {
if tx.blocking_send(Ok(res?)).is_err() {
break;
}
}
Ok(())
};
builder.spawn_blocking(blocking_iter);

I am able to nest correctly on my fork where I have changed all spawn_blocking in scan.rs

@ion-elgreco
Copy link
Collaborator Author

@chiragjn have added that as well, should be good to go now

@ion-elgreco ion-elgreco enabled auto-merge (squash) October 24, 2025 08:31
@ion-elgreco
Copy link
Collaborator Author

@roeap can you stamp it again? :)

Copy link
Member

@rtyler rtyler left a comment

Choose a reason for hiding this comment

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

I want to incorporate this in 0.29.2 which I'll be releasing today

@ion-elgreco ion-elgreco merged commit a22a97e into delta-io:main Oct 24, 2025
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

binding/rust Issues for the Rust crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants