-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(replay): Introduce ReplayDurationLimitError and enhance error ha… #18796
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.
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
base: develop
Are you sure you want to change the base?
Changes from all commits
342f849
89478a5
6de50dc
c80217d
878da7e
5b3a744
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,7 +55,7 @@ import { debug } from './util/logger'; | |
| import { resetReplayIdOnDynamicSamplingContext } from './util/resetReplayIdOnDynamicSamplingContext'; | ||
| import { closestElementOfNode } from './util/rrweb'; | ||
| import { sendReplay } from './util/sendReplay'; | ||
| import { RateLimitError } from './util/sendReplayRequest'; | ||
| import { RateLimitError, ReplayDurationLimitError } from './util/sendReplayRequest'; | ||
| import type { SKIPPED } from './util/throttle'; | ||
| import { throttle, THROTTLED } from './util/throttle'; | ||
|
|
||
|
|
@@ -1185,7 +1185,7 @@ export class ReplayContainer implements ReplayContainerInterface { | |
| // We leave 30s wiggle room to accommodate late flushing etc. | ||
| // This _could_ happen when the browser is suspended during flushing, in which case we just want to stop | ||
| if (timestamp - this._context.initialTimestamp > this._options.maxReplayDuration + 30_000) { | ||
| throw new Error('Session is too long, not sending replay'); | ||
| throw new ReplayDurationLimitError(); | ||
| } | ||
|
|
||
| const eventContext = this._popEventContext(); | ||
|
|
@@ -1218,7 +1218,14 @@ export class ReplayContainer implements ReplayContainerInterface { | |
| const client = getClient(); | ||
|
|
||
| if (client) { | ||
| const dropReason = err instanceof RateLimitError ? 'ratelimit_backoff' : 'send_error'; | ||
| let dropReason: 'ratelimit_backoff' | 'send_error' | 'sample_rate'; | ||
| if (err instanceof RateLimitError) { | ||
| dropReason = 'ratelimit_backoff'; | ||
| } else if (err instanceof ReplayDurationLimitError) { | ||
| dropReason = 'sample_rate'; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Incorrect drop reason for duration limit exceededMedium Severity The |
||
| } else { | ||
| dropReason = 'send_error'; | ||
| } | ||
| client.recordDroppedEvent(dropReason, 'replay'); | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| import { describe, expect, it } from 'vitest'; | ||
| import { | ||
| RateLimitError, | ||
| ReplayDurationLimitError, | ||
| TransportStatusCodeError, | ||
| } from '../../../src/util/sendReplayRequest'; | ||
|
|
||
| describe('Unit | util | sendReplayRequest', () => { | ||
| describe('TransportStatusCodeError', () => { | ||
| it('creates error with correct message', () => { | ||
| const error = new TransportStatusCodeError(500); | ||
| expect(error.message).toBe('Transport returned status code 500'); | ||
| expect(error).toBeInstanceOf(Error); | ||
| }); | ||
| }); | ||
|
|
||
| describe('RateLimitError', () => { | ||
| it('creates error with correct message and stores rate limits', () => { | ||
| const rateLimits = { all: 1234567890 }; | ||
| const error = new RateLimitError(rateLimits); | ||
| expect(error.message).toBe('Rate limit hit'); | ||
| expect(error.rateLimits).toBe(rateLimits); | ||
| expect(error).toBeInstanceOf(Error); | ||
| }); | ||
| }); | ||
|
|
||
| describe('ReplayDurationLimitError', () => { | ||
| it('creates error with correct message', () => { | ||
| const error = new ReplayDurationLimitError(); | ||
| expect(error.message).toBe('Session is too long, not sending replay'); | ||
| expect(error).toBeInstanceOf(Error); | ||
| }); | ||
|
|
||
| it('is distinguishable from other error types', () => { | ||
| const durationError = new ReplayDurationLimitError(); | ||
| const rateLimitError = new RateLimitError({ all: 123 }); | ||
| const transportError = new TransportStatusCodeError(500); | ||
|
|
||
| expect(durationError instanceof ReplayDurationLimitError).toBe(true); | ||
| expect(durationError instanceof RateLimitError).toBe(false); | ||
| expect(durationError instanceof TransportStatusCodeError).toBe(false); | ||
|
|
||
| expect(rateLimitError instanceof ReplayDurationLimitError).toBe(false); | ||
| expect(rateLimitError instanceof RateLimitError).toBe(true); | ||
|
|
||
| expect(transportError instanceof ReplayDurationLimitError).toBe(false); | ||
| expect(transportError instanceof TransportStatusCodeError).toBe(true); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'sample_rate'would not be the correct drop reason here (this is only used in case an event was dropped because of the configured sample rate).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it then if im not wrong then is
network_errorthe appropriate drop reason ?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, let's work with
ignoredfor now (We might add a new drop reason in the future!) – please also update the browser integration tests for thisThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense, I'll update it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@harshit078 actually we'll introduce a new reason right away so this stays consistant. Mind if I take this PR over? The rest looks good already