Skip to content

chore: Add spans to get_account stack#1936

Merged
sergerad merged 4 commits intomainfrom
sergerad-spans-v0.14
Apr 17, 2026
Merged

chore: Add spans to get_account stack#1936
sergerad merged 4 commits intomainfrom
sergerad-spans-v0.14

Conversation

@sergerad
Copy link
Copy Markdown
Collaborator

@sergerad sergerad commented Apr 13, 2026

Relates to #1920.

@sergerad sergerad added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label Apr 13, 2026
@sergerad sergerad requested review from bobbinth and igamigo April 13, 2026 19:44
@bobbinth
Copy link
Copy Markdown
Contributor

I think v0.14 "master" is currently in main - or are we keeping these two branches in sync?

@sergerad sergerad changed the base branch from release/v0.14 to main April 14, 2026 08:57
@sergerad
Copy link
Copy Markdown
Collaborator Author

I think v0.14 "master" is currently in main - or are we keeping these two branches in sync?

Pointing at main now. I will delete the release/v0.14 branch I made.

Comment thread crates/store/src/state/mod.rs Outdated

if !map_keys_requests.is_empty() {
let forest_guard = self.forest.read().await;
let forest_guard = self.forest.read().instrument(tracing::Span::current()).await;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why was this needed?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

To have a dedicated span and duration for reading this lock.

Copy link
Copy Markdown
Collaborator

@Mirko-von-Leipzig Mirko-von-Leipzig Apr 16, 2026

Choose a reason for hiding this comment

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

I think @SantiagoPittella means that this should already be auto-instrumented by the #[instrument] on the function. If it wasn't then this is.. unexpected.

If you want a dedicated span then we should give this a new span, not re-use the current one.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Sorry had meant to .instrument(tracing::info_span!("something")).

I think/hope we don't need instrument(tracing::Span::current()) on such futures to ensure they fall under the relevant span of the stack they are called from.

Copy link
Copy Markdown
Contributor

@bobbinth bobbinth 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! Thank you! I left a couple of small comments inline.

Comment thread crates/store/src/account_state_forest/mod.rs
Comment thread crates/store/src/account_state_forest/mod.rs
Copy link
Copy Markdown
Collaborator

@Mirko-von-Leipzig Mirko-von-Leipzig left a comment

Choose a reason for hiding this comment

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

Thanks @sergerad. Once we've figured out wtf is going on, we should drop these down to debug again. But we can has this sort of thing out in the #1949

@sergerad sergerad merged commit 88fbd26 into main Apr 17, 2026
18 checks passed
@sergerad sergerad deleted the sergerad-spans-v0.14 branch April 17, 2026 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no changelog This PR does not require an entry in the `CHANGELOG.md` file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants