From 734e5c66a3601379554c3cbc8e2f12909241f4b3 Mon Sep 17 00:00:00 2001 From: Nathan Broadbent Date: Sun, 14 Oct 2018 01:54:21 +0700 Subject: [PATCH 1/2] If Sentry responds with 413 request too large, try to send the exception again without any state --- .eslintrc | 5 +++-- index.js | 44 ++++++++++++++++++++++++++++++++++++++++++++ index.test.js | 34 ++++++++++++++++++++++++++++++---- 3 files changed, 77 insertions(+), 6 deletions(-) diff --git a/.eslintrc b/.eslintrc index 5958d97..0d6f225 100644 --- a/.eslintrc +++ b/.eslintrc @@ -9,7 +9,8 @@ ], "rules": { "prettier/prettier": "error", - "prefer-arrow-callback": "error" + "prefer-arrow-callback": "error", + "prefer-reflect": "off", }, "parserOptions": { "ecmaVersion": 8, @@ -20,4 +21,4 @@ "node": true, "es6": true } -} \ No newline at end of file +} diff --git a/index.js b/index.js index de078f2..8345acb 100644 --- a/index.js +++ b/index.js @@ -32,6 +32,50 @@ function createRavenMiddleware(Raven, options = {}) { return original ? original(data) : data; }); + const retryCaptureWithoutReduxState = captureFn => { + Raven.setDataCallback((data, originalCallback) => { + Raven.setDataCallback(originalCallback); + const reduxExtra = { + lastAction: actionTransformer(lastAction), + state: "Failed to submit state to Sentry: 413 request too large." + }; + data.extra = Object.assign(reduxExtra, data.extra); + data.breadcrumbs.values = []; + return data; + }); + // Raven has an internal check for duplicate errors that we need to disable. + const originalAllowDuplicates = Raven._globalOptions.allowDuplicates; + Raven._globalOptions.allowDuplicates = true; + captureFn(); + Raven._globalOptions.allowDuplicates = originalAllowDuplicates; + }; + + const retryWithoutStateOnRequestTooLarge = originalFn => { + return (...captureArguments) => { + const originalTransport = Raven._globalOptions.transport; + Raven.setTransport(opts => { + Raven.setTransport(originalTransport); + opts.onError = error => { + if (error.request && error.request.status === 413) { + // Retry request without state after "413 request too large" error + retryCaptureWithoutReduxState(() => { + originalFn.apply(Raven, captureArguments); + }); + } + }; + (originalTransport || Raven._makeRequest).call(Raven, opts); + }); + originalFn.apply(Raven, captureArguments); + }; + }; + + Raven.captureException = retryWithoutStateOnRequestTooLarge( + Raven.captureException + ); + Raven.captureMessage = retryWithoutStateOnRequestTooLarge( + Raven.captureMessage + ); + return next => action => { // Log the action taken to Raven so that we have narrative context in our // error report. diff --git a/index.test.js b/index.test.js index a2eb176..896e38c 100644 --- a/index.test.js +++ b/index.test.js @@ -2,9 +2,9 @@ const Raven = require("raven-js"); const createRavenMiddleware = require("./index"); const { createStore, applyMiddleware } = require("redux"); -Raven.config("https://5d5bf17b1bed4afc9103b5a09634775e@sentry.io/146969", { - allowDuplicates: true -}).install(); +Raven.config( + "https://5d5bf17b1bed4afc9103b5a09634775e@sentry.io/146969" +).install(); const reducer = (previousState = { value: 0 }, action) => { switch (action.type) { @@ -32,7 +32,7 @@ describe("raven-for-redux", () => { Raven.setDataCallback(undefined); Raven.setBreadcrumbCallback(undefined); Raven.setUserContext(undefined); - + Raven._globalOptions.allowDuplicates = true; Raven._breadcrumbs = []; Raven._globalContext = {}; }); @@ -186,6 +186,32 @@ describe("raven-for-redux", () => { userData ); }); + + ["captureException", "captureMessage"].forEach(fnName => { + it(`retries ${fnName} without any state if Sentry returns 413 request too large`, () => { + context.mockTransport.mockImplementationOnce(options => { + options.onError({ request: { status: 413 } }); + }); + // allowDuplicates is set to true inside our handler and reset afterwards + Raven._globalOptions.allowDuplicates = null; + Raven[fnName].call(Raven, new Error("Crash!")); + + // Ensure transport and allowDuplicates have been reset + expect(Raven._globalOptions.transport).toEqual(context.mockTransport); + expect(Raven._globalOptions.allowDuplicates).toEqual(null); + expect(context.mockTransport).toHaveBeenCalledTimes(2); + const { extra } = context.mockTransport.mock.calls[0][0].data; + expect(extra).toMatchObject({ + state: { value: 0 }, + lastAction: undefined + }); + const { extra: extra2 } = context.mockTransport.mock.calls[1][0].data; + expect(extra2).toMatchObject({ + state: "Failed to submit state to Sentry: 413 request too large.", + lastAction: undefined + }); + }); + }); }); describe("with all the options enabled", () => { beforeEach(() => { From e1011183f390509ced0029f04ffac4b8297b392b Mon Sep 17 00:00:00 2001 From: Nathan Broadbent Date: Sun, 14 Oct 2018 04:00:43 +0700 Subject: [PATCH 2/2] Skip sending initial request with state if we know the request body is over the limit of 200KB --- .eslintignore | 1 + index.js | 29 +++++++--- index.test.js | 46 ++++++++++++++- package.json | 4 +- vendor/json-stringify-safe/stringify.js | 76 +++++++++++++++++++++++++ 5 files changed, 145 insertions(+), 11 deletions(-) create mode 100644 vendor/json-stringify-safe/stringify.js diff --git a/.eslintignore b/.eslintignore index f91b489..25f7fc7 100644 --- a/.eslintignore +++ b/.eslintignore @@ -1,2 +1,3 @@ built/ coverage/ +vendor/ diff --git a/index.js b/index.js index 8345acb..38e6780 100644 --- a/index.js +++ b/index.js @@ -1,3 +1,5 @@ +const stringify = require("./vendor/json-stringify-safe/stringify"); + const identity = x => x; const getUndefined = () => {}; const filter = () => true; @@ -32,12 +34,12 @@ function createRavenMiddleware(Raven, options = {}) { return original ? original(data) : data; }); - const retryCaptureWithoutReduxState = captureFn => { + const retryCaptureWithoutReduxState = (errorMessage, captureFn) => { Raven.setDataCallback((data, originalCallback) => { Raven.setDataCallback(originalCallback); const reduxExtra = { lastAction: actionTransformer(lastAction), - state: "Failed to submit state to Sentry: 413 request too large." + state: errorMessage }; data.extra = Object.assign(reduxExtra, data.extra); data.breadcrumbs.values = []; @@ -50,15 +52,28 @@ function createRavenMiddleware(Raven, options = {}) { Raven._globalOptions.allowDuplicates = originalAllowDuplicates; }; - const retryWithoutStateOnRequestTooLarge = originalFn => { + const retryWithoutStateIfRequestTooLarge = originalFn => { return (...captureArguments) => { const originalTransport = Raven._globalOptions.transport; Raven.setTransport(opts => { Raven.setTransport(originalTransport); + const requestBody = stringify(opts.data); + if (requestBody.length > 200000) { + // We know the request is too large, so don't try sending it to Sentry. + // Retry the capture function, and don't include the state this time. + const errorMessage = + "Could not send state because request would be larger than 200KB. " + + `(Was: ${requestBody.length}B)`; + retryCaptureWithoutReduxState(errorMessage, () => { + originalFn.apply(Raven, captureArguments); + }); + return; + } opts.onError = error => { if (error.request && error.request.status === 413) { - // Retry request without state after "413 request too large" error - retryCaptureWithoutReduxState(() => { + const errorMessage = + "Failed to submit state to Sentry: 413 request too large."; + retryCaptureWithoutReduxState(errorMessage, () => { originalFn.apply(Raven, captureArguments); }); } @@ -69,10 +84,10 @@ function createRavenMiddleware(Raven, options = {}) { }; }; - Raven.captureException = retryWithoutStateOnRequestTooLarge( + Raven.captureException = retryWithoutStateIfRequestTooLarge( Raven.captureException ); - Raven.captureMessage = retryWithoutStateOnRequestTooLarge( + Raven.captureMessage = retryWithoutStateIfRequestTooLarge( Raven.captureMessage ); diff --git a/index.test.js b/index.test.js index 896e38c..6589986 100644 --- a/index.test.js +++ b/index.test.js @@ -2,6 +2,12 @@ const Raven = require("raven-js"); const createRavenMiddleware = require("./index"); const { createStore, applyMiddleware } = require("redux"); +const stringify = require.requireActual( + "./vendor/json-stringify-safe/stringify" +); +const stringifyMocked = require("./vendor/json-stringify-safe/stringify"); +jest.mock("./vendor/json-stringify-safe/stringify"); + Raven.config( "https://5d5bf17b1bed4afc9103b5a09634775e@sentry.io/146969" ).install(); @@ -27,6 +33,7 @@ const context = {}; describe("raven-for-redux", () => { beforeEach(() => { + stringifyMocked.mockImplementation(obj => stringify(obj)); context.mockTransport = jest.fn(); Raven.setTransport(context.mockTransport); Raven.setDataCallback(undefined); @@ -188,13 +195,48 @@ describe("raven-for-redux", () => { }); ["captureException", "captureMessage"].forEach(fnName => { + it(`skips state for ${fnName} if request would be larger than 200000B`, () => { + expect(Raven._globalOptions.transport).toEqual(context.mockTransport); + stringifyMocked + .mockClear() + .mockImplementationOnce(() => ({ length: 200001 })) + .mockImplementationOnce(() => ({ length: 500 })); + // Test that allowDuplicates is set to true inside our handler and reset afterwards + // (Error message needs to be unique for each test, because we set allowDuplicates to null) + Raven._globalOptions.allowDuplicates = null; + Raven[fnName].call( + Raven, + fnName === "captureException" + ? new Error("Test skip state") + : "Test skip state" + ); + + // Ensure transport and allowDuplicates have been reset + expect(Raven._globalOptions.transport).toEqual(context.mockTransport); + expect(Raven._globalOptions.allowDuplicates).toEqual(null); + expect(context.mockTransport).toHaveBeenCalledTimes(1); + const { extra } = context.mockTransport.mock.calls[0][0].data; + expect(extra).toMatchObject({ + state: + "Could not send state because request would be larger than 200KB. (Was: 200001B)", + lastAction: undefined + }); + }); + it(`retries ${fnName} without any state if Sentry returns 413 request too large`, () => { + expect(Raven._globalOptions.transport).toEqual(context.mockTransport); context.mockTransport.mockImplementationOnce(options => { options.onError({ request: { status: 413 } }); }); - // allowDuplicates is set to true inside our handler and reset afterwards + // Test that allowDuplicates is set to true inside our handler and reset afterwards + // (Error message needs to be unique for each test, because we set allowDuplicates to null) Raven._globalOptions.allowDuplicates = null; - Raven[fnName].call(Raven, new Error("Crash!")); + Raven[fnName].call( + Raven, + fnName === "captureException" + ? new Error("Test retry on 413 error") + : "Test retry on 413 error" + ); // Ensure transport and allowDuplicates have been reset expect(Raven._globalOptions.transport).toEqual(context.mockTransport); diff --git a/package.json b/package.json index 223ae78..086e679 100644 --- a/package.json +++ b/package.json @@ -6,9 +6,9 @@ "scripts": { "fix": "eslint --fix .", "tdd": "jest --watch", - "test": "jest --coverage && npm run lint", + "test": "jest --coverage --collectCoverageFrom=index.js && npm run lint", "lint": "eslint .", - "build": "babel index.js --out-dir built/", + "build": "babel index.js vendor/json-stringify-safe/stringify.js --out-dir built/", "prepublishOnly": "npm run test && npm run build", "serve-example": "webpack-dev-server --watch --config=example/webpack.config.js" }, diff --git a/vendor/json-stringify-safe/stringify.js b/vendor/json-stringify-safe/stringify.js new file mode 100644 index 0000000..f058032 --- /dev/null +++ b/vendor/json-stringify-safe/stringify.js @@ -0,0 +1,76 @@ +/* + json-stringify-safe + Like JSON.stringify, but doesn't throw on circular references. + + Copied from sentry-javascript/packages/raven-js/vendor/json-stringify-safe/stringify.js + + Originally forked from https://github.com/isaacs/json-stringify-safe + version 5.0.1 on 3/8/2017 and modified to handle Errors serialization + and IE8 compatibility. Tests for this are in test/vendor. + + ISC license: https://github.com/isaacs/json-stringify-safe/blob/master/LICENSE +*/ + +exports = module.exports = stringify; +exports.getSerialize = serializer; + +function indexOf(haystack, needle) { + for (var i = 0; i < haystack.length; ++i) { + if (haystack[i] === needle) return i; + } + return -1; +} + +function stringify(obj, replacer, spaces, cycleReplacer) { + return JSON.stringify(obj, serializer(replacer, cycleReplacer), spaces); +} + +// https://github.com/ftlabs/js-abbreviate/blob/fa709e5f139e7770a71827b1893f22418097fbda/index.js#L95-L106 +function stringifyError(value) { + var err = { + // These properties are implemented as magical getters and don't show up in for in + stack: value.stack, + message: value.message, + name: value.name + }; + + for (var i in value) { + if (Object.prototype.hasOwnProperty.call(value, i)) { + err[i] = value[i]; + } + } + + return err; +} + +function serializer(replacer, cycleReplacer) { + var stack = []; + var keys = []; + + if (cycleReplacer == null) { + cycleReplacer = function(key, value) { + if (stack[0] === value) { + return '[Circular ~]'; + } + return '[Circular ~.' + keys.slice(0, indexOf(stack, value)).join('.') + ']'; + }; + } + + return function(key, value) { + if (stack.length > 0) { + var thisPos = indexOf(stack, this); + ~thisPos ? stack.splice(thisPos + 1) : stack.push(this); + ~thisPos ? keys.splice(thisPos, Infinity, key) : keys.push(key); + + if (~indexOf(stack, value)) { + value = cycleReplacer.call(this, key, value); + } + } else { + stack.push(value); + } + + return replacer == null + ? value instanceof Error ? stringifyError(value) : value + : replacer.call(this, key, value); + }; +}