bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Enable trace by default

Open mockersf opened this issue 3 years ago • 8 comments
trafficstars

Objective

  • Because spans are 🔥

Solution

  • Move some spans to debug to reduce the default volume of spans created
  • Span reduction is enough to change the tracy trace from 1 minute of breakout from 64MB to 22MB
  • The spans still at info level are: frame, stages, system, system commands. those should be the most useful to help debug issues as a user
  • if someone wants all the spans, they just need to set the log level to debug

mockersf avatar Jul 27 '22 16:07 mockersf

Hmmmm. I want to keep the render pass spans as they’re commonly useful for understanding performance. The rest I agree with.

superdump avatar Jul 27 '22 18:07 superdump

Frankly, I don't reallly like either option. This is consistent with wgpu, so it's probably fine; I certainly don't intend to block this. The fact that wgpu doesn't even expose a feature makes me happier.

The basic fact is that features are not well suited to this - because they are additive, and instead something like crate 'options' (which don't exist) would be nicer. This is broken if your dependencies use bevy with default features (which is, if not idiomatic, at least nearly inevitable as the number of dependencies goes up).

Additionally, needing to enable tracing/chrome integration in cargo.toml is not a good experience - I'd much rather it be a plugin to be consistent with everything else. However, that then becomes a problem in terms of binary size when it's unused - do we trust it to be properly tree-shaken?

All in all, this feels inevitably painful, unfortunately, but this is probably an improvement.

DJMcNab avatar Jul 27 '22 19:07 DJMcNab

Hmmmm. I want to keep the render pass spans as they’re commonly useful for understanding performance. The rest I agree with.

No...

The spans I left as info are the one that are visible when tracing is not used. Well most. Stages and Systems are visible when the user logs somethings, and system commands are visible when a command panic.

For more specialised usage of trace, I feel like we are soon reaching a point where we have too many spans anyway and will need to only enable those for the part we're looking at, so setting bevy_ecs logs to debug, or bevy_render, or...

mockersf avatar Jul 27 '22 20:07 mockersf

I'm pretty sure I'm on board for the debug! switch, but why enable trace by default when its something that:

  1. Generally shouldn't be deployed with released apps at all (yes we can cut down on this with debug, but why create this problem for ourselves?)
  2. Is something that a user needs to voluntarily build in to their workflows. Tracy is non-trivial to set up, especially for non-windows systems. Chrome tracing is also "weird" to integrate into your workflows (you need to worry about perfetto and stuff).
  3. Still requires manual intervention to enable a specific tracing backend.

I'd be more convinced if we had some sort of built-in profiler that needed to "just work" (ex: in the Bevy Editor). But even in the context of a Bevy Editor, we will likely have more control over development loops / features enabled / etc, so theres a chance we could abstract over this.

I think we should at least talk about what specific scenarios enabling this by default (but without a backend) enables. Is the goal to "plugin-ize" the backends / remove the need for specific cargo features? Is that worth the cost / risk of "always on" spans?

cart avatar Aug 01 '22 00:08 cart

@cart spans are not only useful for tracing, they also show up in logs, and the "span stack" is displayed by the panic hook

mockersf avatar Aug 01 '22 00:08 mockersf

@cart spans are not only useful for tracing, they also show up in logs, and the "span stack" is displayed by the panic hook

Spans showing up in logs isn't necessarily something we want in release builds by default. It leaks "internal implementation details" to users running apps in their console / fills it with extra noise. Spans feel like "debug information" generally.

"span stack" in the panic hook is potentially compelling though, if it provides additional useful context when users submit their crash reports. But even without spans, we'll get the stack trace, which will include system information. Id want to see some examples that illustrate the importance of including this information before using this as motivation for the change.

cart avatar Aug 01 '22 01:08 cart

Spans showing up in logs isn't necessarily something we want in release builds by default. It leaks "internal implementation details" to users running apps in their console / fills it with extra noise. Spans feel like "debug information" generally.

I've been an advocate of "you really should disable default features of Bevy and only enable those you need in a release" for a long time. We currently don't have a "how to build for release" doc though. At least most third party plugins already don't enable default features.

"span stack" in the panic hook is potentially compelling though, if it provides additional useful context when users submit their crash reports. But even without spans, we'll get the stack trace, which will include system information. Id want to see some examples that illustrate the importance of including this information before using this as motivation for the change.

The original goal behind adding those is panicking command, whose stack trace is quite unhelpful. With the span stack you know the originating system of the command

mockersf avatar Aug 01 '22 01:08 mockersf

@mockersf oh, I'd completely missed this was about enabling trace by default. Hmm, I guess it depends who is the receiver of the default configuration. I suppose when shipping an app, a developer would make and deploy/distribute a build and users will never have anything to do with production release builds. So I suppose it makes sense that default configuration targets developers. Still, I don't know that tracing should be enabled by default. I agree that the span stack in a panic is maybe nice, depending on how useful it is in practice. Hopefully it is an improvement to panic error output.

superdump avatar Aug 01 '22 07:08 superdump

I've been an advocate of "you really should disable default features of Bevy and only enable those you need in a release" for a long time. We currently don't have a "how to build for release" doc though. At least most third party plugins already don't enable default features.

This is worth doing, but I'm still of the stance that we should still assume normal release builds with default features will be the most common case, and we should construct the settings to make that case behave as well as we can.

Disabling default features also means you're responsible for knowing all of the features you actually need / how to express that. This is already non-trivial and as Bevy expands I think this user experience will continue to degrade. Manually specifying features also increases the likelihood that you will break, as it means any rephrasing of features is a breaking change (even if the underlying code doesn't change). I don't think its reasonable to foist this complexity on people as a general rule of thumb. We should quantify (in code size and compile times) what this will do for people for an "average game" before we encourage them to open this can of worms / build it into our recommendations.

The original goal behind adding those is panicking command, whose stack trace is quite unhelpful. With the span stack you know the originating system of the command

I'll poke around with this / get a feel for what it looks like. Command debugging is definitely a footgun / worth solving. But if we can solve that in a more scoped way, I think we should.

cart avatar Aug 04 '22 23:08 cart