Skip to content

ref(browser): Ensure idle span ending is consistent #12310

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
merged 12 commits into from
Jun 3, 2024
Merged

ref(browser): Ensure idle span ending is consistent #12310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
06c9d35
test(browser): Fix flaky test
mydea May 31, 2024
921ab6a
WIP debug this...
mydea May 31, 2024
29fc11e
WIP run flaky detector in bundle mode...
mydea May 31, 2024
5253d18
more debug
mydea May 31, 2024
8c2e4d0
maybe fix it??
mydea May 31, 2024
dcb905f
Revert "WIP run flaky detector in bundle mode..."
mydea May 31, 2024
df9e5f5
Revert "more debug"
mydea May 31, 2024
2db2188
Revert "WIP debug this..."
mydea May 31, 2024
e9b0824
make more robust
mydea May 31, 2024
8cb3053
WIP debug flaky test?
mydea Jun 3, 2024
4fea67e
fix it ???
mydea Jun 3, 2024
6c12cd8
cleanup
mydea Jun 3, 2024
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
13 changes: 6 additions & 7 deletions ...ation-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/init.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,9 @@ Sentry.init({
});

const activeSpan = Sentry.getActiveSpan();
if (activeSpan) {
Sentry.startInactiveSpan({ name: 'pageload-child-span' });
} else {
setTimeout(() => {
Sentry.startInactiveSpan({ name: 'pageload-child-span' });
}, 200);
}
Sentry.startInactiveSpan({
name: 'pageload-child-span',
onlyIfParent: true,
// Set this to ensure we do not discard this span due to timeout
startTime: activeSpan && Sentry.spanToJSON(activeSpan).start_timestamp,
});
1 change: 1 addition & 0 deletions ...ation-tests/suites/tracing/browserTracingIntegration/pageloadWithChildSpanTimeout/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ sentryTest('should send a pageload span terminated via child span timeout', asyn
const eventData = envelopeRequestParser(req);

expect(eventData.contexts?.trace?.op).toBe('pageload');
expect(eventData.contexts?.trace?.data?.['sentry.idle_span_discarded_spans']).toBeUndefined();
expect(eventData.spans?.length).toBeGreaterThanOrEqual(1);
const testSpan = eventData.spans?.find(span => span.description === 'pageload-child-span');
expect(testSpan).toBeDefined();
Expand Down
8 changes: 5 additions & 3 deletions packages/core/src/tracing/idleSpan.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,9 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti
beforeSpanEnd(span);
}

const timestamp = args[0] || timestampInSeconds();
// Just ensuring that this keeps working, even if we ever have more arguments here
const [definedEndTimestamp, ...rest] = args;
const timestamp = definedEndTimestamp || timestampInSeconds();
const spanEndTimestamp = spanTimeInputToSeconds(timestamp);

// Ensure we end with the last span timestamp, if possible
Expand All @@ -130,7 +132,7 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti
// If we have no spans, we just end, nothing else to do here
if (!spans.length) {
onIdleSpanEnded(spanEndTimestamp);
return Reflect.apply(target, thisArg, args);
Copy link
Contributor

@lforst lforst May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is this change intended? Me idiot.

a bit scary though this change 🤔 Can we make sure with a type that we forward all the args in case the api is ever expanded.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hard to do with a type as stuff would always have to be optional, so this would always match - but I added code to ensure we also pass rest arguments, so that is forwards-compatible!

return Reflect.apply(target, thisArg, [spanEndTimestamp, ...rest]);
}

const childEndTimestamps = spans
Expand All @@ -152,7 +154,7 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti
);

onIdleSpanEnded(endTimestamp);
return Reflect.apply(target, thisArg, [endTimestamp]);
return Reflect.apply(target, thisArg, [endTimestamp, ...rest]);
},
});

Expand Down