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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tracing integration #79

Closed
wants to merge 1 commit into from
Closed

Conversation

mvlabat
Copy link

@mvlabat mvlabat commented May 11, 2022

Checklist

  • I have read the Contributor Guide
  • I have read and agree to the Code of Conduct
  • I have added a description of my changes and why I'd like them included in the section below

Description of Changes

This adds a new crate implementing integration with tracing. This will let puffin users benefit from other libraries that generate spans with the tracing crate, which I believe is quite extensively used in the Rust ecosystem.

I was initially going to publish it to a separate repository, but I thought it would probably be a better addition to this one, to make sure this integration is always up to date, advertise it as an official one if you see it fit here.. and pass the maintenance burden to you guys. 馃槃

I wrote up the documentation a bit hastily, I'm not sure how sufficient it is. So I'm open to suggestions (and I left the edits allowed as well).

And thank you for such an awesome tool. I'm also looking forward to it being used by Bevy, which also relies on tracing - so hopefully tracing support in puffin will make Bevy integration much closer to becoming real.

@mvlabat mvlabat requested review from emilk and VZout as code owners May 11, 2022 20:59
@repi repi self-requested a review May 11, 2022 21:30
@repi
Copy link
Contributor

repi commented May 11, 2022

oh interesting contribution, thank you!

there is also @aclysma 's profiling crate that is a layer across puffin (and multiple other profilers) as well as tracing, did you consider using that but it wasn't a fit for your use case?

not fully sure myself what would be the best setup and recommendation here, and also is a possibility that they both can co-exist with tracing support so users can pick the one they prefer (though ideally with some good docs/comparisons).

internally we have our own layer also across puffin & superluminal, and been considering adding tracing to that also, but would likely do it directly in that layer rather than through a separate crate mechanism.

think we have to discuss a bit internally and here in the PR of how to best proceed.

@IceSentry
Copy link

The main use case for me and @mvlabat is to use puffin in combination with bevy. Using profiling would be nice, but it would require internal changes to bevy since it currently uses tracing directly. Having this allows us to use puffin without needing to modify bevy.

@aclysma
Copy link
Contributor

aclysma commented May 12, 2022

I think it may be worth considering bevy-specific macros that resolve to whatever you end up wanting to use. That way if you change your mind later on what crate to use, you can change it in one place. (i.e. bevy_profile!() or similar). You could make such a macro closely fit tracing's API to ease the transition and make it a rote find/replace job.

(I would consider adding this abstraction even if you continue to just use tracing. It's very typical for game engines to define their own wrappers over something like this. And it ensures consumers of your engine who use your macros will get seamlessly transitioned as well any time the engine itself makes a transition.)

@mvlabat
Copy link
Author

mvlabat commented May 12, 2022

Thank you for the responses!

there is also @aclysma 's profiling crate that is a layer across puffin (and multiple other profilers) as well as tracing, did you consider using that but it wasn't a fit for your use case?

As @IceSentry noted, we are looking into adding puffin support into Bevy (either natively, or via a plugin). I don't have much knowledge on what specific use-cases we have for tracing, but it seems that tracing can give more detailed spans, it also has chrome and tracy integrations, so I believe it's just more convenient to use as a central backend and leave users the choice of how to consume the traces.

I think it may be worth considering bevy-specific macros that resolve to whatever you end up wanting to use.

I agree, this sounds like a good idea.

@ten3roberts
Copy link

Is there any progress on this issue, as it seems ready to merge?

@VZout
Copy link
Member

VZout commented Jul 20, 2022

We had a brief internal discussion I forgot to communicate here. We decided against merging this and recommend people to use profiling or engine specific abstractions instead! I'll close this PR for now.

@VZout VZout closed this Jul 20, 2022
@ten3roberts
Copy link

ten3roberts commented Jul 20, 2022

Alright, thanks for the quick reply.

So if using tracing and profiling mean this:

#[profiling::function]
#[tracing::instrument]
fn example () {
    ...
}

@ten3roberts
Copy link

Or an altogether macro which does both, or calls puffin, or nothing depending on features

@VZout
Copy link
Member

VZout commented Jul 20, 2022

Alright, thanks for the quick reply.

So if using tracing and profiling mean this:

#[profiling::function]
#[tracing::instrument]
fn example () {
    ...
}

I'm not to familiar with profiling or tracing but it looks like profiling implements a abstraction for tracing's span! macros so the second macro attribute on your example function should be unnecessary if using profile-with-tracing as a feature flag on profiling.

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.

None yet

6 participants