Skip to content

fix(browser): Fix bug causing unintentional dropping of transactions #12933

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 2 commits into from
Jul 17, 2024
Merged

fix(browser): Fix bug causing unintentional dropping of transactions #12933

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e96b4af
.
lforst Jul 17, 2024
90f86d3
math.max
lforst Jul 17, 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
8 changes: 7 additions & 1 deletion packages/browser-utils/src/metrics/browserMetrics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,13 @@ export function addPerformanceEntries(span: Span): void {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
performanceEntries.slice(_performanceCursor).forEach((entry: Record<string, any>) => {
const startTime = msToSec(entry.startTime);
const duration = msToSec(entry.duration);
const duration = msToSec(
// Inexplicibly, Chrome sometimes emits a negative duration. We need to work around this.
// There is a SO post attempting to explain this, but it leaves one with open questions: https://stackoverflow.com/questions/23191918/peformance-getentries-and-negative-duration-display
// The way we clamp the value is probably not accurate, since we have observed this happen for things that may take a while to load, like for example the replay worker.
Copy link
Member

@Lms24 Lms24 Jul 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

does the worker take a while to load though? I'd imagine creating the blob should be fairly fast since the code ships with the already loaded JS SDK bundle by default? My guess yesterday was the opposite: that it's too fast and therefore causing weird timing problems 😅
(maybe this is also blob-specific?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it loaded in something like 4ms when we tested it (which was locally in dev mode). So not too fast. Even if it loaded "too fast" a negative duration doesn't make sense in my eyes 🤔

// TODO: Investigate why this happens and how to properly mitigate. For now, this is a workaround to prevent transactions being dropped due to negative duration spans.
entry.duration < 0 ? 0 : entry.duration,
Copy link
Member

Choose a reason for hiding this comment

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

l: any reason for not using Math.max?

Suggested change
entry.duration < 0 ? 0 : entry.duration,
Math.max(0, entry.duration),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a theory that performance is better for the inline if but can change

);

if (op === 'navigation' && transactionStartTime && timeOrigin + startTime < transactionStartTime) {
return;
Expand Down
Loading