sdk: Enable configurable buffer exhausted policy for TrackEvent#5409
sdk: Enable configurable buffer exhausted policy for TrackEvent#5409bsarden wants to merge 1 commit intogoogle:mainfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
I think the reason for this behaviour is pure oversight. Configurable buffer exhausted modes were only introduced signifcantly after track event and I think we never thought about retroactively applying that to TrackEvent. @primiano @sashwinbalaji would be the right people to confirm the above. |
|
So I did some digging in #1312 and the internal bug b/384007571 I think this feature was introduced to allow SDK users that implement data sources to make it configurable by calling PerfettoDsSetBufferExhaustedPolicyConfigurable(true) I think back then nobody thought about track event, it was focused only on the DS layer. Now there is a legitimate question that is: should TrackEvent itself be configurable? I think depends on whether you want: A) TrackEvent in an app you control to be configurable: I'd be absolutely positive about this (But shoudl be done in a different way than this PR) B) TrackEvent to be always configurable. I'd be a bit more careful here. My problem here is the fact that trak event is used in key system services (e.g system_server). Making this universally configurable (incl system_server) means making it so that if somebody copy/pastes the wrong flag into the config without knowing what they are doing, they will cause a device soft-reboot, which is unideal So if you want to send out a PR for A, i'm more then happy to review, but needs to be something that a client of TrackEvent calls explicitly when initializing. |
17b4ae3 to
1e98bf9
Compare
Add two new APIs to TrackEvent and TrackEventDataSource: 1. SetBufferExhaustedPolicyConfigurable(true) — allows apps to opt in to runtime-configurable buffer exhausted policy (drop vs stall) via their trace config. 2. SetBufferExhaustedPolicy(policy) — sets a runtime default policy without enabling trace config override. Useful when an app wants a different default (e.g. kStall) but doesn't want external configs to change it. Both must be called before Register(). TrackEvent is used in critical system services (e.g., system_server) where an accidental kStall policy could cause a device soft-reboot. Rather than making configurability the default via the existing compile-time constant, these add per-app opt-ins. See google#1312 and b/384007571. The implementation adds runtime flags to DataSourceType that are OR'd with compile-time constants during registration.
1e98bf9 to
af66124
Compare
TrackEvent inherits from DataSource which defaults kBufferExhaustedPolicyConfigurable to false. This means the buffer exhausted policy (drop vs stall) is fixed at compile time and cannot be changed via the trace config at runtime.
Setting kBufferExhaustedPolicyConfigurable to true on TrackEventDataSource allows trace configs to override the default kDrop policy with kStall on a per-session basis. This is useful for scenarios where completeness of trace data is more important than avoiding writer stalls, such as deterministic testing or targeted traces where dropping events would invalidate results.
Without this change, users who need stall-on-full behavior for TrackEvent must create a custom data source wrapper just to flip this flag, which might be unnecessary boilerplate. The redundancy comes up when having to re-define all the
TRACE_macros to use the custom data source wrapper.I haven't found a reason for why TrackEvent buf exhausted policy is non-configurable upstream.
There might be a valid reason for this.
If so, what would the recommendation be for achieving 'lossless TrackEventDataSource' behavior?
cc: @dreveman