-
Notifications
You must be signed in to change notification settings - Fork 568
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 some tracing and logging #1621
Conversation
Could you also remove all default features besides "fmt" from tracing subscriber? It wont do anything now but will have add compile time improvement later. |
Already done in a later commit, which had not yet pushed. But yeah, it's a good point. |
7353ab4
to
5b4ce67
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First: thank you or your patience so far, this has definitely been a trickier set of patches than we thought, at first.
Second: I don't think we're quite there, yet.
Having to instrument each widget independently feels awkward. Ultimately, widgets are generally not where the important stuff happens (the exception being the container widgets, and complex things like scrolling). Since all of the logic around invalidation and event delivery is scoped to the WidgetPod
, it feels to me like we should be able to get most of the benefits of this patch just be instrumenting WidgetPod
, and then also instrumenting the various Ctx
methods.
There's a method on the Widget
trait, type_name
. This returns &str
with a verbose type name, like: druid::widget::radio::Radio<druid::widget::flex::MainAxisAlignment>
. I think that we could use this method, in combination with WidgetPod
, to essentially instrument all widgets, in one place.
Did you consider this approach? Are there some limitations that I've overlooked?
Paraphrasing what I said on zulip for visibility: Okay, so I've tried the Basically, if I have This is because tracing only accepts string literals as span names; as I understand it, the goal is to cache is whether or not to use a given span at startup, to avoid doing string prefix comparison every time the program needs to compare a span against filter rules. So I'll keep the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay so:
Ultimately I think there are clear arguments for having most of the tracing being on WidgetPod
, but if that is too difficult then I understand wanting to prioritize getting something working now, that is reasonably effective.
Thank you for your patience @PoignardAzur!
It doesn't seem like a code path that warrants a warning message.
Copy-pasting from Zulip:
I'm still using
#[instrument]
because, as I've said earlier, this is far from the biggest compile bottleneck right now; (I'm going to do some tests again to measure the impact it has). Also, while I don't like the syntax instrument has right now (having to add skip(...) everywhere), apparently it's due to be changed in the next version of tracing.Right now I'm trying to address the use-case that made me want to add more logging: being able to tell when an internal event (warning, something being redrawn, etc) happens and why.
Part of that is adding spans everywhere; since druid, like basically every GUI, uses a variation on the visitor pattern, that means most spans are going be named after the Self type of the function they instrument; except for the root span. For instance, if you have Window::event calling Flex::event calling Button::event, the context chain is going to look like event:Flex:Button.
I feel like I should document that last part somewhere.