connect flecs to tracing ecosystem #200
Conversation
Georgiy-Tugai
force-pushed
the
tracing
branch
from
f105e29 to
02eb5e3
Compare
waywardmonkeys
left a comment
waywardmonkeys
left a comment
There was a problem hiding this comment.
Will have more questions later, but this is a start.
| } | ||
|
|
||
| #[derive(Clone, Debug)] | ||
| pub(crate) struct MetadataRequest<'a> { |
There was a problem hiding this comment.
Going by code below, filename and line are only valid when kind == MetadataKind::Event, so it seems like MetadataKind::Event should be holding that data.
In that case, then the PartialEq and Hash would be derived and not have a manual implementation.
If there is a reason for not doing the expected thing (what I just described), then that should be documented with a comment somewhere.
There was a problem hiding this comment.
filename and line are valid for both Event and Span, but for Span they are not part of the hash key, since the push and pop events will come from different line numbers (& possibly different file names).
It's a moot point at the moment, though, since metadata for spans is currently not cached in the metadata registry, only in the span registry which uses the span name as the key. This may change in the future; an alternative design would have one metadata per ecs_os_perf_trace_push callsite and put the span names into values (like log messages).
flecs_ecs_tracing/src/perf_trace.rs
Outdated
| line: usize, | ||
| name: Cow<'_, str>, | ||
| ) -> (span::Id, &'static Metadata<'static>, bool) { | ||
| if let Some((existing, metadata)) = REGISTRY.read().unwrap().spans.get(name.as_ref()) { |
There was a problem hiding this comment.
This and another instance of a similar code pattern feel like they should be chained functions rather than an if / early return. But maybe not?
flecs_ecs_tracing/src/perf_trace.rs
Outdated
| struct Registry { | ||
| spans: HashMap< | ||
| String, | ||
| (tracing_core::span::Id, &'static Metadata<'static>), |
There was a problem hiding this comment.
This feels odd. Why's the Id separate from the Metadata? Or why's this a tuple and not some type?
And why can't someone legitimately own this metadata, like the registry? Is it because it is owned by the dynamic callsite, which you're leaking? If so, why not store that somewhere around here? But you're the one that decided to store it in the callsite, so ... ?
There was a problem hiding this comment.
tracing_core borrows Callsite for 'static and demands that Callsite can provide a reference to the Metadata (fn metadata(&self) -> &Metadata<'_>).
I guess I could store the MetadataRequest inside DynamicCallsite and do lookups in REGISTRY every time but I'm not seeing any advantages to doing that...
Georgiy-Tugai
force-pushed
the
tracing
branch
from
02eb5e3 to
380d0cf
Compare
2 participants
No description provided.