-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat(core): Ensure replay envelopes are sent in order when offline #11413
Conversation
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.
Can we add a test for replays with the offline transport in our browser integration tests? I think there's already one for the offline transport with errors but it might make sense to cover replay events as well now. If it turns out to be flaky or too much of a hassle, no worries, we can leave it as-is.
It looks like we can't round-trip serialise/parse envelopes in the browser tests because of this: This means that a It might be easier to add this as an integration test instead! |
yup, sounds good! |
This comment was marked as outdated.
This comment was marked as outdated.
He says confidently and then it fails in CI |
This reverts commit ccbcb22.
dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts
Fixed
Show fixed
Hide fixed
dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts
Fixed
Show fixed
Hide fixed
dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts
Fixed
Show fixed
Hide fixed
dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts
Fixed
Show fixed
Hide fixed
dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts
Fixed
Show fixed
Hide fixed
dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts
Fixed
Show fixed
Hide fixed
dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts
Fixed
Show fixed
Hide fixed
dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts
Fixed
Show fixed
Hide fixed
This comment was marked as outdated.
This comment was marked as outdated.
dev-packages/browser-integration-tests/suites/replay/captureReplayOffline/test.ts
Dismissed
Show dismissed
Hide dismissed
The tests are finally passing! |
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.
Thanks Tim!
72fe05b
to
1011f04
Compare
This is failing in CI due to some unrelated playwright test: Looking at the captures trace, it's failing due to CORS issues... |
…etsentry#11413) Modifies `makeOfflineTransport` to allow replay envelopes to be queued when offline and to ensure they are always send in order. To do this we: - Modify the store interface so it has `push`, `unshift` and `shift` methods (BREAKING) - Always queue replay envelopes in the store so there is never more than one in-flight - Always `unshift` failed envelopes so they're always sent in the same order from the queue
Closes #11207
This PR modifies
makeOfflineTransport
to allow replay envelopes to be queued when offline and to ensure they are always send in order.To do this we:
push
,unshift
andshift
methods (BREAKING)unshift
failed envelopes so they're always sent in the same order from the queueThese changes do not ensure ALL envelopes are sent in order as the only way to do that would be queuing all envelopes and waiting on their completion before moving on to the next.
The major downside to queuing replay envelopes is that from replays perspective, they never fail to send and the retry logic is redundant.