Skip to content
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

Merged
merged 23 commits into from
Apr 9, 2024

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Apr 3, 2024

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:

  • 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

These 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.

@timfish timfish marked this pull request as ready for review April 3, 2024 22:19
@AbhiPrasad AbhiPrasad self-requested a review April 3, 2024 22:26
@lforst lforst requested a review from mydea April 4, 2024 13:48
Copy link
Member

@Lms24 Lms24 left a 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.

@timfish
Copy link
Collaborator Author

timfish commented Apr 5, 2024

It looks like we can't round-trip serialise/parse envelopes in the browser tests because of this:
vitest-dev/vitest#4043 (comment)

This means that a Uint8Array created by jsdom is not an instanceof Uint8Array in node which means at the serialisation step, the buffer is converted to JSON.

It might be easier to add this as an integration test instead!

@Lms24
Copy link
Member

Lms24 commented Apr 5, 2024

It might be easier to add this as an integration test instead!

yup, sounds good!

@timfish

This comment was marked as outdated.

@timfish
Copy link
Collaborator Author

timfish commented Apr 5, 2024

Ah scrap that, I managed to get it working.

He says confidently and then it fails in CI

This comment was marked as outdated.

@timfish
Copy link
Collaborator Author

timfish commented Apr 5, 2024

The tests are finally passing!

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Thanks Tim!

@timfish timfish force-pushed the timfish/feat-ordered-offline-queue branch from 72fe05b to 1011f04 Compare April 8, 2024 13:29
@timfish
Copy link
Collaborator Author

timfish commented Apr 9, 2024

This is failing in CI due to some unrelated playwright test:
https://github.com/getsentry/sentry-javascript/actions/runs/8612639079/job/23602437605?pr=11413#step:10:128

Looking at the captures trace, it's failing due to CORS issues...

@Lms24 Lms24 merged commit 6c8cdcd into develop Apr 9, 2024
97 checks passed
@Lms24 Lms24 deleted the timfish/feat-ordered-offline-queue branch April 9, 2024 15:10
cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this pull request Apr 19, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

makeBrowserOfflineTransport not fully compatible with Replay
2 participants