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

Add trace level logging to most API entry points #4183

Merged
merged 2 commits into from
Sep 28, 2023
Merged

Conversation

nical
Copy link
Contributor

@nical nical commented Sep 28, 2023

Checklist

  • Run cargo clippy.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Description

This PR adds trace-level logs to all or most of the entry points in wgpu-core. A fair amount of debug level logs were converted to trace in the process.

@nical
Copy link
Contributor Author

nical commented Sep 28, 2023

I didn't format many of the arguments in the log (mostly the ID of the object in question) in this PR, We should probably format more arguments (the descriptors may be a bit much but there's a fair amount of other useful info) Let me know what you think. I also didn't add much logging to the error paths, I can add them in this PR or a followup.

Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

Should these be trace as well?

wgpu-core/src/device/global.rs Outdated Show resolved Hide resolved
wgpu-core/src/device/global.rs Outdated Show resolved Hide resolved
wgpu-core/src/device/global.rs Outdated Show resolved Hide resolved
@nical
Copy link
Contributor Author

nical commented Sep 28, 2023

indeed!

Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

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

LGTM!

The use case I can imagine for the additions specifically appears to be adding context for other log messages. This sort of context-keeping is very similar in domain to tracing's "span"-based model. I wonder if this might be worth exploring for wgpu later; it might obviate things like this PR later, and avoid pitfalls when multiple threads are involved (which I think we do for wgpu right now?).

EDIT: olol, @teoxoy beat me to approval. 😆

@nical
Copy link
Contributor Author

nical commented Sep 28, 2023

My main motivation right now is to easily inspect what failing webgpu pages do in firefox's about:logging. If we can get that with spans then 👍

@nical nical merged commit 9a76c48 into gfx-rs:trunk Sep 28, 2023
20 checks passed
@nical nical deleted the log branch September 28, 2023 16:29
@JMS55
Copy link
Contributor

JMS55 commented Sep 28, 2023

Did wgpu ever compile-out its logs with a cargo feature? In the past, Bevy has noted that a significant amount of time is wasted in wgpu checking if logs are enabled or not at runtime: bevyengine/bevy#7223. I can't find an issue or PR about it in this repo though.

@nical
Copy link
Contributor Author

nical commented Sep 28, 2023

If you depend on the log crate with a feature flag like release_max_level_info, the trace level logs should be discarded at build time. You can do it in bevy directly and that should affect all of the dependencies that use the same version of log.

@nical nical mentioned this pull request Sep 29, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants