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

[Merged by Bors] - Fix bevy_ecs::schedule::executor_parallel::system span management #2905

Closed
wants to merge 1 commit into from

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented Oct 2, 2021

Objective

Solution

  • Simply hoist span creation out of the threaded task
  • Confirmed to solve the issue locally

Now all events have the full span parent tree up through bevy_ecs::schedule::stage all the way to bevy_app::app::bevy_app (and its parents in bevy-consumer code, if any).

Or at least bevy_ecs::schedule::executor_parallel::system
Fixes bevyengine#2904
@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Oct 2, 2021
@CAD97
Copy link
Contributor Author

CAD97 commented Oct 2, 2021

This has the concrete benefit that you can now write a log filter [bevy_ecs::schedule::stage{name=PostUpdate}] to enable logs solely in PostUpdate, and it will include all of the systems, not just those running on the main thread.

@mockersf
Copy link
Member

mockersf commented Oct 2, 2021

Would this be fixed by #2423 , or is it another span that didn't have its parent? If they are the same fix, yours is much simpler 👍

@CAD97
Copy link
Contributor Author

CAD97 commented Oct 2, 2021

@mockersf these do seem to be the same issue! And yeah, I was glad to see that I could grab the TLS context so easily rather than having to thread the context through manually behind the cfg.

@NiklasEi NiklasEi added C-Usability A simple quality-of-life change that makes Bevy easier to use S-Needs-Review and removed S-Needs-Triage This issue needs to be labelled labels Oct 2, 2021
@NiklasEi NiklasEi added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review labels Oct 6, 2021
@mockersf
Copy link
Member

mockersf commented Oct 6, 2021

bors r+

bors bot pushed a commit that referenced this pull request Oct 6, 2021
)

# Objective

- Fixes #2904 (see for context)

## Solution

- Simply hoist span creation out of the threaded task
- Confirmed to solve the issue locally

Now all events have the full span parent tree up through `bevy_ecs::schedule::stage` all the way to `bevy_app::app::bevy_app` (and its parents in bevy-consumer code, if any).
@bors bors bot changed the title Fix bevy_ecs::schedule::executor_parallel::system span management [Merged by Bors] - Fix bevy_ecs::schedule::executor_parallel::system span management Oct 6, 2021
@bors bors bot closed this Oct 6, 2021
@CAD97 CAD97 deleted the parallel-ecs-span-management branch October 6, 2021 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Usability A simple quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bevy_ecs::schedule::executor_parallel::system tracing span should have an explicit parent
3 participants