Skip to content

Control span data through tracing Extensions #7239

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.

Sign up for GitHub

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

Jump to bottom
Merged
mergify merged 5 commits into from
Jul 22, 2025
Merged

Control span data through tracing Extensions #7239

mergify merged 5 commits into from
Jul 22, 2025
+230 −146

Conversation

@macladson
Copy link
Member

@macladson macladson commented Apr 1, 2025

Issue Addressed

#7234

Proposed Changes

Removes the Arc<Mutex<_> which was used to store and manage span data and replaces it with the inbuilt Extension for managing span-specific data. This also avoids an unwrap which was used when acquiring the lock over the mutex'd span data.

@eserilev
Copy link
Member

eserilev commented Apr 25, 2025 •
edited
Loading

This looks like a nice simplification to me. I'm trying to think through the different panic scenarios. If theres multiple threads accessing the same spans extensions_mut is there a race condition where get_mut returns None for both threads, but one of the threads ends up panicking? I think since we have span usage across async tasks, the race condition might be technically possible?

@macladson macladson mentioned this pull request May 6, 2025
Closed
ackintosh
ackintosh reviewed May 17, 2025
View reviewed changes
@jimmygchen jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels May 27, 2025
@macladson macladson added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jun 3, 2025
ackintosh
ackintosh approved these changes Jun 4, 2025
View reviewed changes
Copy link
Member

@ackintosh ackintosh 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 to me!

michaelsproul
michaelsproul approved these changes Jul 22, 2025
View reviewed changes
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

LGTM; didn't review

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jul 22, 2025
mergify bot added a commit that referenced this pull request Jul 22, 2025
@mergify
Merge of #7239
04fc866
@mergify mergify bot mentioned this pull request Jul 22, 2025
Closed
6 tasks
@mergify mergify bot merged commit e6089fe into sigp:unstable Jul 22, 2025
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelsproul michaelsproul michaelsproul approved these changes

+1 more reviewer

@ackintosh ackintosh ackintosh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

code-quality ready-for-merge This PR is ready to merge. UX-and-logs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@macladson @eserilev @ackintosh @michaelsproul @jimmygchen