Skip to content

feat(profiling): Drop profiler ids from spans no profile covers - #6015

Open
markushi wants to merge 8 commits into
mainfrom
feat/drop-invalid-profiler-ids-poll
Open

feat(profiling): Drop profiler ids from spans no profile covers#6015
markushi wants to merge 8 commits into
mainfrom
feat/drop-invalid-profiler-ids-poll

Conversation

@markushi

@markushi markushi commented Aug 27, 2026

Copy link
Copy Markdown
Member

📜 Description

Transactions and spans are tagged with the continuous profiler's profiler_id as soon as they start, but the OS only tells us later whether a Perfetto profile really exists. This PR lets anything tagged with an id that leads nowhere drop the reference before it is sent.

IContinuousProfiler gets one new method:

@NotNull ProfileRecordingState getProfileRecordingState(
    SentryId profilerId, SentryDate startTime, SentryDate endTime);

SentryTracer.finish() asks it once for the root span window and once per child span window, and removes the ProfileContext plus the profiler_id span data only on NOT_RECORDED. RECORDED and UNKNOWN change nothing, so an outcome we do not know never costs a valid link.

PerfettoContinuousProfiler answers from a bounded history of chunk records, guarded by a lock of its own so that a finishing transaction never waits for a chunk start, a chunk stop or an OS callback. The answer for a window follows one policy:

Scenario Answer
No chunk ever ran UNKNOWN
A chunk covering part of the window was recorded RECORDED
No covering chunk was recorded, but one has no outcome yet UNKNOWN
Every covering chunk failed, or no chunk covers the window NOT_RECORDED

The outcome of a chunk has a single owner on each side. PerfettoProfiler only ever reports a failure — an OS error code, a missing or empty trace file, or the result timeout — as soon as it knows, which is what makes the common rate-limit case cheap. PerfettoContinuousProfiler alone declares a chunk recorded, from the trace file it actually received, since the OS can report success and still leave nothing usable behind. NOT_RECORDED is final, so a result the OS delivers late cannot revive a chunk that was already given up on.

NoOpContinuousProfiler, AndroidContinuousProfiler and JavaContinuousProfiler answer UNKNOWN, which leaves every non-Perfetto path exactly as it was.

This is an alternative to #5993, which solved the same problem with callbacks from the profiler into the tracer. Pulling the state at send time removes the listener registration, the unregistration on finish, and the tracers that stay registered because they never finish.

💡 Motivation and Context

Rate limiting is the common case on API 35+, and the OS reports it roughly 1 ms after the request. Without this, a rate-limited session produces transactions that link to profiles the backend never receives, which shows up in the UI as dangling profile references. There is no backend logic that removes such references.

💚 How did you test it?

Unit tests, on three levels:

  • ChunkRecordTest — the state machine (NOT_RECORDED is final), whether a chunk has ended, and the window arithmetic, including the exact boundaries and a chunk that is still running.
  • PerfettoProfilerTest — the record outcome for an OS error, a null result path, a missing trace file, the result timeout, and a result that arrives after the timeout, plus the two cases where a successful result leaves the record untouched.
  • PerfettoContinuousProfilerTest — the answer for a running chunk, a chunk that is still being collected, a collected chunk, a failed chunk, a failed and an unknown chunk under one window, a window after the last chunk, a window that starts before the profiler did, per-chunk judgement across a failed and a recorded chunk, eviction, and close.
  • SentryTracerTest — dropped, kept and unknown outcomes, per-span judgement, a span that never finished, and a profiler_id the SDK did not write.

📝 Checklist

  • I added GH Issue ID & Linear ID
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.
  • Public API changes reviewed by another Mobile SDK team member or implemented according to the develop docs spec.

🔮 Next steps

Known limits, left out deliberately:

  • PerfettoProfiler.start(long) was public and is now package-private, since it returns the internal chunk record. The class is @ApiStatus.Internal and @RequiresApi(35), but the method was on the published surface in 8.51.0–8.53.0.
  • Only SentryTracer drops the id. OpenTelemetry spans take another path and are not covered. This has no effect today, as only the Android Perfetto profiler ever answers anything but UNKNOWN.
  • Chunks that Sentry itself drops after a valid trace file was produced — a client-side rate limit, or cache eviction while offline — still read as recorded. The scope here is the OS reporting that no profile exists.
  • The chunk history holds 10 chunks, so about 10 minutes. A transaction older than that is judged by the chunks that are left.
  • A profiler id from a profiler instance that a second Sentry.init replaced reads as NOT_RECORDED. Keeping such ids alive belongs to whatever owns the profiler lifecycle across inits, not to the chunk history.

markushi and others added 2 commits August 27, 2026 10:24
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor
Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 60c5572

@sentry

sentry Bot commented Aug 27, 2026

Copy link
Copy Markdown

📲 Install Builds

Android

🔗 App Name App ID Version Configuration
SDK Size io.sentry.tests.size 8.54.0 (1) release

⚙️ sentry-android Build Distribution Settings

@markushi

Copy link
Copy Markdown
Member Author

@sentry review

@0xadam-brown 0xadam-brown left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Much easier to follow, IMO. A few initial comments. Haven't checked for any threading issues. But the APIs are looking clean 💯

Comment thread sentry-android-core/src/main/java/io/sentry/android/core/PerfettoProfiler.java Outdated
@markushi
markushi marked this pull request as ready for review August 28, 2026 08:08
@markushi markushi added the deep-dive PR needs a thorough review of design, behavior, and edge cases label Aug 28, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 07e9b69. Configure here.

@0xadam-brown 0xadam-brown left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor comments + worth addressing the SentryBot issue; otherwise lgtm

return startTimestamp;
}

public boolean overlaps(final @NotNull SentryDate startTime, final @NotNull SentryDate endTime) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth a quick Javadoc reminding folks that this logic should be kept in sync with the rules we use on the backend to decide which chunks to display for a given transaction – at least until our hoped-for tombstone protocol shows up 😄.

if (endTime.isBefore(startTimestamp)) {
return false;
}
// A chunk that is still running has no end yet, and covers everything from its start on

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// A chunk that is still running has no end yet, and covers everything from its start on
// A chunk that is still running has no end yet, and covers everything from its start onward

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Fwiw, at first I thought the sentence had been truncated.)

final @NotNull SentryId profilerId,
final @NotNull SentryDate startTime,
final @NotNull SentryDate endTime) {
return ProfileRecordingState.UNKNOWN;

@0xadam-brown 0xadam-brown Aug 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts about replacing with a default method on IConinuousProfiler that returns UNKNOWN (same throughout)?

}

// Every chunk overlapping the window failed, or no chunk ran during the window at all
return ProfileRecordingState.NOT_RECORDED;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: getProfileRecordingState() can incorrectly return NOT_RECORDED when a transaction's profile chunks are evicted from history after a profiler restart, causing profiler links to be dropped.
Severity: MEDIUM

Suggested Fix

Modify getProfileRecordingState() to return UNKNOWN instead of NOT_RECORDED when the chunkHistory is non-empty but no chunk matches the given profilerId. This prevents incorrectly dropping profiler links for chunks that may have been evicted from history.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location:
sentry-android-core/src/main/java/io/sentry/android/core/PerfettoContinuousProfiler.java#L266

Potential issue: In `getProfileRecordingState()`, if the `chunkHistory` is not empty but
contains no chunks matching a given `profilerId`, the method incorrectly returns
`NOT_RECORDED`. This scenario can occur if a profiler session restarts, generating a new
`profilerId`, and a long-running transaction from the previous session finishes after
its corresponding chunks have been evicted from the fixed-size history. As a result, the
system wrongly concludes the profile was never recorded, leading to the incorrect
dropping of valid profiler links from the transaction.

Did we get this right? 👍 / 👎 to inform future reviews.

@runningcode runningcode left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking on this complicated fix! Looks good! I left a two comments, one with a locking issue and the other with a clock issue. Let me know if you'd like to discuss either.

It's also valid to say they aren't blockers or to do them in a follow up so I'll approve this for now.

profilerId = new SentryId();
}

final @Nullable ChunkRecord chunkRecord =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's a gap here between when we call perfettoProfiler.start and when we call addChunkRecord which means that if a transaction finishes between those two events, it won't check the newest chunkRecord since it hasn't been added yet.
my clanker suggests the best fix is to write the chunkRecord first and then remove it if it fails.

}
// A chunk that is still running has no end yet, and covers everything from its start onward
final @Nullable SentryDate endTimestamp = this.endTimestamp;
return endTimestamp == null || !startTime.isAfter(endTimestamp);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

App start spans are back-dated with a SentryLongDate, so isBefore/isAfter fall back to the wall clock here and we know Android's clock steps backwards a few seconds after boot (from the tests we did a few weeks ago). A skewed miss reads as NOT_RECORDED and strips profiler_id off the app start transaction. I would instead suggest that "no covering chunk" return UNKNOWN instead? This way the profilder_id isn't stripped off.

I really need to get my clock changes...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deep-dive PR needs a thorough review of design, behavior, and edge cases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants