remove js payload limit #1823

djskinner · 2022-09-29T13:40:08Z

Goal

Remove the check that prevented attempting delivery of event and session payloads > 1MB
For delivery implementations that support retries, ensure that any failed event is not retried if > 1MB

Design

Remove code in json-payload that throws for oversized events thus allowing for attempted send
Check payload size of failed-to-send events in each delivery method and handle them accordingly: logging and ensuring they are not retried

Changeset

Testing

Manual tests
- Electron: Attempt to send over-sized (breadcrumb metadata) payload and verify log output and no retry
- Node
- x-domain-request: Tested on ie8/ie9 via Browserstack
- xml-http-request: Tested on Firefox, Chrome (latest), IE11 via Browserstack
- expo: tested with a local expo app force pulling a local version of @bugsnag/core with these changes in and verifying no issues upon the 400 response (though no logging as that only gets added in explicitly mark failed payloads >1MB as not retryable bugsnag-expo#65)
e2e test coverage for much of the above

packages/delivery-node/delivery.js

djskinner · 2022-09-29T13:42:56Z

packages/delivery-x-domain-request/delivery.js

    const req = new win.XDomainRequest()
    req.onload = function () {
      cb(null)
    }
+    req.onerror = function () {
+      client._logger.error('Event failed to send…\n')


node does logger.error(Event failed to send…` so I thought maybe the browser delivery methods should too

github-actions · 2022-09-29T13:47:14Z

`@bugsnag/browser` bundle size diff

	Minified	Minfied + Gzipped
Before	`43.19 kB`	`13.21 kB`
After	`43.43 kB`	`13.34 kB`
±	⚠️ `+239 bytes`	⚠️ `+135 bytes`

code coverage diff

<temporarily disabled>

Generated by 🚫 dangerJS against c455ae4

packages/delivery-electron/test/delivery.test-main.ts

CHANGELOG.md

gingerbenw

changelog wording seems fine to me, but @gareththackeray might have a better idea

Co-authored-by: Gareth Thackeray <gareth.thackeray@smartbear.com>

djskinner · 2022-12-01T10:14:22Z

test/electron/features/delivery.feature

+Feature: Delivery of errors
+
+    Scenario: Delivery for an oversized error is not retried
+        Given I launch an app with configuration:
+            | bugsnag | default |
+        And I set the HTTP status code for the next "POST" request to 400
+        And I click "oversized-notify"
+        Then I wait to receive an error
+
+        # Check that Bugsnag is discarding the event
+        And I wait to receive 3 logs
+        Then I discard the oldest log
+        Then I discard the oldest log
+        And the log payload field "message" equals "Event oversized (2.00 MB)"
+


@Cawllec this doesn't run because the steps are undefined for electron. Perhaps this is something we can add later as I'm happy the manual testing has shown no issues

djskinner · 2022-12-01T10:16:27Z

packages/core/lib/json-payload.js

@@ -15,13 +15,11 @@ Serialized payload was ${payload.length / 10e5}MB (limit = 1MB)
 metadata was removed`
    }
    payload = jsonStringify(event, null, null, { redactedPaths: EVENT_REDACTION_PATHS, redactedKeys })
-    if (payload.length > 10e5) throw new Error('payload exceeded 1MB limit')


the change can be summarised as: don't throw here so each delivery mechanism attempts the send. For delivery mechanisms that support retries we explicitly make it not retryable if the payload was above the threshold (regardless of 400 status code or not)

djskinner · 2022-12-01T16:10:26Z

@imjoehaines @gingerbenw I've requested a re-review. The implementation hasn't changed since you last looked but there has been some work on the e2e tests so I thought it would be good for some more eyes on it again before we merge

CHANGELOG.md

remove js payload limit

10f0707

djskinner commented Sep 29, 2022

View reviewed changes

packages/delivery-node/delivery.js Outdated Show resolved Hide resolved

djskinner commented Sep 29, 2022

View reviewed changes

djskinner added 2 commits September 29, 2022 17:21

remove js payload limit

e57249c

remove js payload limit

fa453ab

djskinner commented Sep 30, 2022

View reviewed changes

packages/delivery-electron/test/delivery.test-main.ts Show resolved Hide resolved

add json-payload unit tests

df3f6b5

djskinner commented Sep 30, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

djskinner added 5 commits October 3, 2022 13:14

dry up test suite

aa5aa7e

remove js payload limit

805cd29

remove js payload limit

051d36b

remove js payload limit

2e4daae

remove js payload limit

32481fa

djskinner marked this pull request as ready for review October 4, 2022 09:09

djskinner requested a review from gingerbenw October 4, 2022 09:10

gingerbenw approved these changes Oct 6, 2022

View reviewed changes

djskinner and others added 2 commits October 7, 2022 03:52

Update CHANGELOG.md

6d6b459

Co-authored-by: Gareth Thackeray <gareth.thackeray@smartbear.com>

add e2e coverage

1c8d135

djskinner force-pushed the plat-8731 branch 10 times, most recently from 77f3f51 to ee08e33 Compare October 14, 2022 09:56

gareththackeray approved these changes Oct 14, 2022

View reviewed changes

djskinner force-pushed the plat-8731 branch 10 times, most recently from cdec715 to 6ba92ad Compare November 30, 2022 17:28

use remote logging to assert event oversized log message

b1a23c8

djskinner force-pushed the plat-8731 branch from 6ba92ad to b1a23c8 Compare November 30, 2022 17:54

djskinner commented Dec 1, 2022

View reviewed changes

djskinner force-pushed the plat-8731 branch 2 times, most recently from 1ac085c to c336d30 Compare December 1, 2022 10:35

enhance node delivery test for future persistence functionality

af22e4c

djskinner force-pushed the plat-8731 branch from c336d30 to af22e4c Compare December 1, 2022 10:50

revert e2e tests for oversized delivery on electron

b9ed23a

djskinner requested review from imjoehaines and gingerbenw December 1, 2022 16:09

imjoehaines reviewed Dec 1, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Dan Skinner added 4 commits December 15, 2022 10:22

Merge branch 'next' into plat-8731

22557f5

update CHANGELOG.md

094c21a

Merge branch 'next' into plat-8731

aef0a36

revert to running Safari 16 tests on browser stack

c455ae4

djskinner merged commit aced3db into next Jan 9, 2023

djskinner deleted the plat-8731 branch January 9, 2023 16:29

gingerbenw mentioned this pull request Jan 25, 2023

Release v7.19.0 #1913

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove js payload limit #1823

remove js payload limit #1823

djskinner commented Sep 29, 2022 •

edited

Loading

djskinner Sep 29, 2022

github-actions bot commented Sep 29, 2022 •

edited

Loading

gingerbenw left a comment

djskinner Dec 1, 2022

djskinner Dec 1, 2022

djskinner commented Dec 1, 2022

remove js payload limit #1823

remove js payload limit #1823

Conversation

djskinner commented Sep 29, 2022 • edited Loading

Goal

Design

Changeset

Testing

djskinner Sep 29, 2022

Choose a reason for hiding this comment

github-actions bot commented Sep 29, 2022 • edited Loading

@bugsnag/browser bundle size diff

code coverage diff

gingerbenw left a comment

Choose a reason for hiding this comment

djskinner Dec 1, 2022

Choose a reason for hiding this comment

djskinner Dec 1, 2022

Choose a reason for hiding this comment

djskinner commented Dec 1, 2022

djskinner commented Sep 29, 2022 •

edited

Loading

github-actions bot commented Sep 29, 2022 •

edited

Loading

`@bugsnag/browser` bundle size diff