Skip to content

Comments

feat: Offline Support#392

Merged
timfish merged 12 commits intogetsentry:masterfrom
timfish:feat/offline
Dec 15, 2021
Merged

feat: Offline Support#392
timfish merged 12 commits intogetsentry:masterfrom
timfish:feat/offline

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
Contributor

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

Copy link
Contributor

@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

@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
Contributor

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?

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

@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