fix(profiling): Fix accidentally correct Android profile chunk parsing#6183
fix(profiling): Fix accidentally correct Android profile chunk parsing#61830xadam-brown wants to merge 1 commit into
Conversation
Fixes logic for parsing Android profile chunks that pass through AndroidOrV2ProfileChunk::parse (viz., Android trace profiles and Android ANR profiles). Prior to this commit, the ::parse implementation worked only because it had been tailored to the quirks of existing Android trace + ANR profiles. But that special-casing will break once the updates to ANR billing under [JAVA-548](https://linear.app/getsentry/issue/JAVA-548/bill-anr-profiles-as-ui-profile-hours-instead-of-continuous-profile0) are complete. This commit anticipates JAVA-548 by: 1. routing Android trace profile chunks by their payload shape, and allowing all ANR profiles to (rightly) be parsed as V2ProfileChunk's; and 2. serializing processed Android trace chunks with a new 2.android-trace version so downstream consumers can distinguish them from sample-format v2 chunks. (Note that item 2 is also needed by [the Perfetto Profiling project](https://linear.app/getsentry/project/perfetto-profiling-support-android-38fd133f557f/overview).) After this commit, we now reject v1 and versionless Android trace profile chunks. They were always unwanted (and never existed in our reference Android SDK implementation); now we make that explicit. Co-Authored-By: Codex <noreply@openai.com>
| // Destination type: V2ProfileChunk | ||
|
|
||
| // First handle Android trace profiles... | ||
| (_, true) => AndroidProfileChunk::parse(data) |
There was a problem hiding this comment.
is_android_trace_profile is true.
My research here indicates that should be fine for Sentry's own Android SDK implementation. In particular, see the "Relay profile payloads..." table:
Note that the approach in this PR is (AFAICT) correct in that it can handle all values that currently pass through AndroidOrV2ProfileChunk::parse (ie, everything in the red box, where any differences in non-Android profiles are without loss of generality b/c they'll be handled identically by our new and old implementations). My LLM also confirmed that the current approach can handle all historical profile types that i) would currently be routed through AndroidOrV2ProfileChunk::parse and ii) should be mapped to AndroidProfileChunk.
The question is whether we want to continue to support any implementations in the wild that i) differ from that table and ii) rely on the existing approach of parsing anything with an "android" platform as AndroidProfileChunk? We could easily do so by including a if (platform == android) => AndroidProfileChunk::parse(data) fallback after the (sample::Version::V2, _) check.
I've omitted the fallback because it'd leave a live code path "just in case", and I assumed we probably don't want to support speculative use cases. (I've added a callout in the CHANGELOG instead.) But chime in if you think otherwise and I can easily add it...
Warning
Not ready to merge. We need to update the monolith, vroom, and vroomrs to support the new 2.android-trace version before merging the changes in this PR.
See getsentry/sentry#118849, getsentry/vroom#672, and getsentry/vroomrs#93.
Overview
Fixes logic for handling Android profile chunks that pass through AndroidOrV2ProfileChunk::parse (viz., Android trace profiles and Android ANR profiles). Prior to this commit, the ::parse implementation worked only because it had been tailored to the quirks of existing Android trace + ANR profiles. But that special-casing will break once the updates to ANR billing under JAVA-548 are complete.
This commit anticipates JAVA-548 by:
routing Android trace profile chunks by their payload shape, and allowing all ANR profiles to (rightly) be parsed as V2ProfileChunk's; and
serializing processed Android trace chunks with a new 2.android-trace version so downstream consumers can distinguish them from sample-format v2 chunks.
(Note that item 2 is also needed by the Perfetto Profiling project.)
After this commit, we now reject v1 and versionless Android trace profile chunks. Our Android SDK implementation suggests they were always unwanted; now we make that explicit. (But see the comment here.)
JAVA-634
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.