Skip to content

Collect PerformanceCollectionData only in sampled transactions #4834

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
stefanosiano merged 7 commits into from
Oct 29, 2025
Merged

Collect PerformanceCollectionData only in sampled transactions #4834

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7946816
performance data collection is now started only when the transaction …
stefanosiano Oct 24, 2025
eaea0dd
updated changelog
stefanosiano Oct 24, 2025
f1b1540
fixed test
stefanosiano Oct 24, 2025
e53ad03
Merge branch 'main' into stefanosiano/enh/collect-performance-sampled…
stefanosiano Oct 29, 2025
bb0e201
updated changelog
stefanosiano Oct 29, 2025
3c35ad4
updated changelog
stefanosiano Oct 29, 2025
d7c8f8f
compositePerformanceCollector is now set only when transaction is sam…
stefanosiano Oct 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Changelog

## Unreleased

### Miscellaneous

- Collect PerformanceCollectionData only in sampled transactions ([#4834](https://github.com/getsentry/sentry-java/pull/4834))
Comment thread
stefanosiano marked this conversation as resolved.
Outdated

## 8.24.0

### Features
Expand Down
4 changes: 2 additions & 2 deletions sentry/src/main/java/io/sentry/SentryTracer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,8 @@ public SentryTracer(
}

// We are currently sending the performance data only in profiles, but we are always sending
// performance measurements.
if (compositePerformanceCollector != null) {
// performance measurements (frames data in spans).
if (compositePerformanceCollector != null && Boolean.TRUE.equals(isSampled())) {
Comment thread
stefanosiano marked this conversation as resolved.
Outdated
compositePerformanceCollector.start(this);
}
Copy link
Copy Markdown

@seer-by-sentry seer-by-sentry bot Oct 24, 2025

Choose a reason for hiding this comment

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

Bug: compositePerformanceCollector fails to start for transactions with deferred sampling decisions that are later marked as sampled.
Severity: CRITICAL | Confidence: 0.98

🔍 Detailed Analysis

When a transaction is created with a deferred sampling decision (samplingDecision = null), the SentryTracer constructor evaluates Boolean.TRUE.equals(isSampled()). Since isSampled() returns null, this condition is false, preventing compositePerformanceCollector.start() from being called. If the transaction is later sampled via setSamplingDecision(new TracesSamplingDecision(true)), the performance collector remains unstarted, resulting in a loss of performance data for that transaction. Additionally, compositePerformanceCollector.stop() is unconditionally called during finish(), potentially on an uninitialized collector.

💡 Suggested Fix

Modify the SentryTracer constructor to start compositePerformanceCollector if isSampled() is null, or add logic within setSamplingDecision() to start the collector if it wasn't previously started and the transaction becomes sampled.

🤖 Prompt for AI Agent 
Fix this bug. In sentry/src/main/java/io/sentry/SentryTracer.java at lines 94-96: When a
transaction is created with a deferred sampling decision (`samplingDecision = null`),
the `SentryTracer` constructor evaluates `Boolean.TRUE.equals(isSampled())`. Since
`isSampled()` returns `null`, this condition is `false`, preventing
`compositePerformanceCollector.start()` from being called. If the transaction is later
sampled via `setSamplingDecision(new TracesSamplingDecision(true))`, the performance
collector remains unstarted, resulting in a loss of performance data for that
transaction. Additionally, `compositePerformanceCollector.stop()` is unconditionally
called during `finish()`, potentially on an uninitialized collector.

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

Copy link
Copy Markdown
Contributor Author

@stefanosiano stefanosiano Oct 24, 2025

Choose a reason for hiding this comment

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

this is the only case why we didn't do it before
should we ignore it?

Copy link
Copy Markdown
Member

@romtsn romtsn Oct 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

hmm, yeah I think this is valid actually. Probably we can't simply just do that without a breaking change - but even then I think we'd have to discuss this. I'd imagine if we ever do tail-based sampling (and we probably will), setSamplingDecision will be useful for that. So maybe this PR has to be backlogged for now

Copy link
Copy Markdown
Contributor Author

@stefanosiano stefanosiano Oct 28, 2025

Choose a reason for hiding this comment

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

got it
on the other side, if the sample rate is like 0.01, we'd run the performance collection 10k times more than needed 😅
Let's discuss it in the next sync


Expand Down
6 changes: 6 additions & 0 deletions sentry/src/test/java/io/sentry/SentryTracerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1175,6 +1175,12 @@ class SentryTracerTest {
verify(fixture.compositePerformanceCollector).start(anyOrNull<ITransaction>())
}

@Test
fun `when transaction is created, but not sampled, compositePerformanceCollector is not started`() {
val transaction = fixture.getSut(samplingDecision = TracesSamplingDecision(false))
verify(fixture.compositePerformanceCollector, never()).start(anyOrNull<ITransaction>())
}

@Test
fun `when transaction is created and profiled compositePerformanceCollector is started`() {
val transaction =
Expand Down
Loading