RFC-0023: RFC: Updatable Track Events #5549
Replies: 4 comments 14 replies
-
|
One question: do you need to solve the two problems at the same time? or they are different instances that show up in different part of the codebase? My take is that we need independently both option B and C, as they don't sound alternatives to me. they serve different purpose. However, If you tell me you only need them together, then I would do B only for now, as it solves your problem (I think) and keep C to later on, if we ever need args-update for proper nested slices. Dynamic Argument and Flow Augmentation TRACE_EVENT_UPDATE_ARGS("navigation,rail", "child_frame_token", child_frame_token); There is a drawback though: this will cost more bytes in the SMB and in the trace buffer, as it will need to emit a new event with its own timestamp and its own protobuf headers and so on. If the event you want to agument is frequent, frankly I'd recommend you keep using your RAII helper as it saves trace bandwidth. If we go this way then I'd just ask you to look into the details and formulate a proposal that is sound and composes well with the rest of the APIs. Option B: Counter-like track with String (State Machines) We need to come up with a proposal on: how this would look like API-wise though (I'm for a new TRACE_EVENT_XXX macro for this purpose) |
Beta Was this translation helpful? Give feedback.
-
As long as we emit the timestamp of the start of the trace, there shouldn't be any problems here. I agree with Primiano that Option B we should solve earlier and we should try and defer C to some point later. I've seen B appear a lot in many different places, I'm much more skeptical on the wide usecase of C. |
Beta Was this translation helpful? Give feedback.
-
|
📝 RFC Document Updated View changes: Commit History |
Beta Was this translation helpful? Give feedback.
-
|
@LalitMaganti & @sashwinbalaji : @etiennep-chromium and I found an other interesting problem while discussing #6068 The problem is substantially this, how do we encode the state in the Track event proto? one option (no brainer) is adding TYPE_STATE=5 (in enum Type) creating a string field. But that should be an escape hatch that should be used as last resort, as strings are wasteful and also create potential PII problems. Instead we want to use enums as much as possible for the other cases. Now, enums should be extended and the open question is "how should trace processor know which extension are you using?" So the overall problem is this Option 1: TraceProcessor guessesIn my_extension.proto Then in the TrackEvent we would set So far so good, at this level it's all a canonical use of protobuf. Now think about TP parsing, how does TP know that it should use precisely extension 9900 to set the name of the counter? The logic could be "Find the first (and likely only) field set in the TrackEvent extension range which is an enum, and use that" PRO: very pragmatic and proto-conformant, likely JustWorks But then I'd claim that the TrackEvent extensions should be de-facto oneof when you use TYPE_STATE, you are not supposed to set >1 extension value (but who know what the future holds) Option 2: explicit, proto-conformant, larger and redundanttrack_event.proto schema as above but in the TrackEvent we set: Essentially we add a field that says "i'm using extension number 9900" PRO: very explicit, non ambiguous Option 3: explicit, non-redundant, abuses a bit extensions as a lookup dicttrack_event.proto schema the TracePacket payload becomes PRO: explicit, non-ambiguous, non-redundant encoding My preferenceIn order:
Not sure if there is something else i'm not considering |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
📄 RFC Doc: 0023-updatable-track-event.md
RFC: Updatable Track Events
Authors: @etiennep-chromium
Status: Decided
Problem
Currently, the Perfetto Track Event API provides
TRACE_EVENT_BEGINandTRACE_EVENT_ENDto define slices with duration. However, this model has limitations in a scenario common in Chrome:Late Tracing of Long-Running Operations (State Machines)
Current Chrome Pattern (
_END+_BEGIN):In performance_manager, a
TracedWrapperclass observes tracing session starts to ensure properties are traced and emitted.This is a single example, but this pattern is used by several components (about two dozen) throughout Chrome to emit long-running state.
When a new tracing session starts,
TrackEventSessionObserver::OnStartis called, which triggers a re-emission of the state by ending and restarting the slice.Drawbacks:
_ENDand_BEGINevents will sample two separate timestamps, even though there is conceptually only one event.Decision
Pending
Design
PoC: #5597
In practice, these state update don't need to support nested slices, so concerns about nesting are alleviated by explicitly choosing not to support it.
We introduce
TRACE_STATEthat takes a counter-like track argument,StateTrack, and thus cannot be mixed withTRACE_EVENT_BEGIN/END:flows, etc.) that other TRACE_ macros support.
nullptr) needs to be supported to show the empty track.state) that contains values, timestamps and durations.Pros:
Cons:
Alternatives considered
Option A:
TRACE_EVENT_STEP(Original Proposal)PoC: #4167
Introduce a new track event type,
TYPE_SLICE_STEP, and a corresponding macro,TRACE_EVENT_STEP.TRACE_EVENT_STEPacts as an "upsert" operation on the track's slice stack:Pros:
BEGINif no slice found).Cons:
TRACE_EVENT_STEPonly works for the leaf slice. This means we can't ensure multiple nested slices are present when starting a trace usingTRACE_EVENT_STEP.Open questions
💬 Discussion Guidelines:
Beta Was this translation helpful? Give feedback.
All reactions