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: Offline Support #392

Merged
merged 12 commits into from
Dec 15, 2021
Merged

feat: Offline Support #392

merged 12 commits into from
Dec 15, 2021

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Dec 2, 2021

Closes #369

  • Removes existing minidump queue from BaseUploader so there's only a single queue
  • Extends the JSON Store so it can parse Date objects
  • Adds PersistedRequestQueue which stores an array of queue item metadata in a Store and request bodies in separate files
    • Since the request body can be large and binary, we avoid serialisation
  • Adds an ElectronOfflineNetTransport that extends the ElectronNetTransport and makes it the default transport.
    • When sendRequest fails, it pushes the failed request into the queue.

Tests

Electron network emulation with { offline: true } doesn't appear to work from the main process and invalid URLs take too long to timeout for reliable tests. Instead we have special DSNs which the server responds differently to.

  • To test queueing, we return 429
  • To test that other status drop events we return 500

Still to do:

  • Consider data races with PersistedRequestQueue with overlapping add/pop
  • Unit tests for PersistedRequestQueue since it's difficult to test all its features via e2e
  • Which error should result in queuing and which should be dropped? Should any errors be left uncaught or only logged?
  • Fix logging. I've seen "minidump sent" when it got queued

Other

This PR also introduces caching for Electron binaries because we were getting 503 Egress exceeded errors.

@timfish timfish marked this pull request as ready for review December 5, 2021 13:10
@AbhiPrasad
Copy link
Member

Will review this in a bit. In the mean time, getsentry/sentry-dotnet#280 is probably useful to read through.

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Couldn't finish the review, but commenting what I have

src/main/fs.ts Outdated Show resolved Hide resolved
src/main/store.ts Show resolved Hide resolved
src/main/fs.ts Show resolved Hide resolved
src/main/transports/electron-net.ts Outdated Show resolved Hide resolved
src/main/transports/electron-offline-net.ts Show resolved Hide resolved
@timfish
Copy link
Collaborator Author

timfish commented Dec 7, 2021

I've got some more changes incoming because I have to correctly handle different statuses from the underlying transport.

Status.Success -> All went well
Status.RateLimit -> Queue
Any other Status -> Drop event
catch(Error) -> Most probably no connection - Queue

@AbhiPrasad
Copy link
Member

AbhiPrasad commented Dec 8, 2021

Will finish reviewing this PR by eow, asked some other folks at Sentry to take a look to make sure we are not missing anything here.

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

I didn't spot this piece in the PR but are the directories where the file dumps isolated per process instance? Is there a chance two instances would pick up anothers payloads?

Seems that the URL stored enodes the auth token (and project Id?) So even if they did, they'd just be uploading the dumps from another process, is that right?

src/main/fs.ts Outdated Show resolved Hide resolved
@timfish
Copy link
Collaborator Author

timfish commented Dec 8, 2021

I didn't spot this piece in the PR but are the directories where the file dumps isolated per process instance? Is there a chance two instances would pick up anothers payloads?

Electron doesn't support multiple instances using the same userData directory and since we use that path to store the queue, we're freed from multi process concerns.

test/e2e/context.ts Outdated Show resolved Hide resolved
@timfish timfish merged commit 12058e4 into getsentry:master Dec 15, 2021
@timfish timfish deleted the feat/offline branch December 20, 2021 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Offline integration does not seem to work
3 participants