From 8387ed3232aa170d3d8eb78f9c2fa292acf73748 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 26 May 2020 15:00:27 +0200 Subject: [PATCH 1/7] ref: Don't wrap xhr/fetch for sentry breadcrumbs --- packages/browser/src/client.ts | 30 ++++++++ .../browser/src/integrations/breadcrumbs.ts | 73 ++++++------------- 2 files changed, 52 insertions(+), 51 deletions(-) diff --git a/packages/browser/src/client.ts b/packages/browser/src/client.ts index 7a170c49c68b..43e085475acc 100644 --- a/packages/browser/src/client.ts +++ b/packages/browser/src/client.ts @@ -3,6 +3,7 @@ import { DsnLike, Event, EventHint } from '@sentry/types'; import { getGlobalObject, logger } from '@sentry/utils'; import { BrowserBackend, BrowserOptions } from './backend'; +import { Breadcrumbs } from './integrations/index'; import { SDK_NAME, SDK_VERSION } from './version'; /** @@ -69,6 +70,35 @@ export class BrowserClient extends BaseClient { return super._prepareEvent(event, scope, hint); } + /** + * @inheritDoc + */ + public captureEvent(event: Event, hint?: EventHint, scope?: Scope): string | undefined { + let eventId: string | undefined = hint && hint.event_id; + this._processing = true; + + this._processEvent(event, hint, scope) + .then(finalEvent => { + // We need to check for finalEvent in case beforeSend returned null + eventId = finalEvent && finalEvent.event_id; + // We do this here to not parse any requests if they are outgoing to Sentry + // We log breadcrumbs if the integration has them enabled + if (finalEvent) { + const integration = this.getIntegration(Breadcrumbs); + if (integration) { + integration.addSentryBreadcrumb(finalEvent); + } + } + this._processing = false; + }) + .then(null, reason => { + logger.error(reason); + this._processing = false; + }); + + return eventId; + } + /** * Show a report dialog to the user to send feedback to a specific event. * diff --git a/packages/browser/src/integrations/breadcrumbs.ts b/packages/browser/src/integrations/breadcrumbs.ts index 528baa92df41..099bb4e86def 100644 --- a/packages/browser/src/integrations/breadcrumbs.ts +++ b/packages/browser/src/integrations/breadcrumbs.ts @@ -1,17 +1,14 @@ -import { API, getCurrentHub } from '@sentry/core'; -import { Integration, Severity } from '@sentry/types'; +import { getCurrentHub } from '@sentry/core'; +import { Event, Integration, Severity } from '@sentry/types'; import { addInstrumentationHandler, getEventDescription, getGlobalObject, htmlTreeAsString, - logger, parseUrl, safeJoin, } from '@sentry/utils'; -import { BrowserClient } from '../client'; - /** * @hidden */ @@ -67,6 +64,26 @@ export class Breadcrumbs implements Integration { }; } + /** + * Create a breadcrumb of `sentry` from the events themselves + */ + public addSentryBreadcrumb(event: Event): void { + if (!this._options.sentry) { + return; + } + getCurrentHub().addBreadcrumb( + { + category: `sentry.${event.type === 'transaction' ? 'transaction' : 'event'}`, + event_id: event.event_id, + level: event.level, + message: getEventDescription(event), + }, + { + event, + }, + ); + } + /** * Creates breadcrumbs from console API calls */ @@ -151,11 +168,6 @@ export class Breadcrumbs implements Integration { return; } - - // We only capture issued sentry requests - if (this._options.sentry && handlerData.xhr.__sentry_own_request__) { - addSentryBreadcrumb(handlerData.args[0]); - } } /** @@ -167,24 +179,6 @@ export class Breadcrumbs implements Integration { return; } - const client = getCurrentHub().getClient(); - const dsn = client && client.getDsn(); - if (this._options.sentry && dsn) { - const filterUrl = new API(dsn).getBaseApiEndpoint(); - // if Sentry key appears in URL, don't capture it as a request - // but rather as our own 'sentry' type breadcrumb - if ( - filterUrl && - handlerData.fetchData.url.indexOf(filterUrl) !== -1 && - handlerData.fetchData.method === 'POST' && - handlerData.args[1] && - handlerData.args[1].body - ) { - addSentryBreadcrumb(handlerData.args[1].body); - return; - } - } - if (handlerData.error) { getCurrentHub().addBreadcrumb( { @@ -306,26 +300,3 @@ export class Breadcrumbs implements Integration { } } } - -/** - * Create a breadcrumb of `sentry` from the events themselves - */ -function addSentryBreadcrumb(serializedData: string): void { - // There's always something that can go wrong with deserialization... - try { - const event = JSON.parse(serializedData); - getCurrentHub().addBreadcrumb( - { - category: `sentry.${event.type === 'transaction' ? 'transaction' : 'event'}`, - event_id: event.event_id, - level: event.level || Severity.fromString('error'), - message: getEventDescription(event), - }, - { - event, - }, - ); - } catch (_oO) { - logger.error('Error while adding sentry type breadcrumb'); - } -} From 6afb60579cc8349dc0b211fd0c256b13b9b77970 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 26 May 2020 16:06:49 +0200 Subject: [PATCH 2/7] fix: Breadcrumb tests --- .../browser/src/integrations/breadcrumbs.ts | 5 + .../test/integration/suites/breadcrumbs.js | 109 ------------------ 2 files changed, 5 insertions(+), 109 deletions(-) diff --git a/packages/browser/src/integrations/breadcrumbs.ts b/packages/browser/src/integrations/breadcrumbs.ts index 099bb4e86def..639d084d6b6e 100644 --- a/packages/browser/src/integrations/breadcrumbs.ts +++ b/packages/browser/src/integrations/breadcrumbs.ts @@ -179,6 +179,11 @@ export class Breadcrumbs implements Integration { return; } + if (handlerData.fetchData.url.match(/sentry_key/) && handlerData.fetchData.method === 'POST') { + // We will not create breadcrumbs for fetch requests that contain `sentry_key` (internal sentry requests) + return; + } + if (handlerData.error) { getCurrentHub().addBreadcrumb( { diff --git a/packages/browser/test/integration/suites/breadcrumbs.js b/packages/browser/test/integration/suites/breadcrumbs.js index 2997ca626685..77fe03ff98ab 100644 --- a/packages/browser/test/integration/suites/breadcrumbs.js +++ b/packages/browser/test/integration/suites/breadcrumbs.js @@ -84,115 +84,6 @@ describe("breadcrumbs", function() { } ); - it( - optional( - "should transform XMLHttpRequests with events to the Sentry store endpoint as sentry.event type breadcrumb", - IS_LOADER - ), - function() { - return runInSandbox(sandbox, { manual: true }, function() { - var store = - document.location.protocol + - "//" + - document.location.hostname + - (document.location.port ? ":" + document.location.port : "") + - "/api/1/store/" + - "?sentry_key=1337"; - - var xhr = new XMLHttpRequest(); - xhr.open("POST", store); - xhr.send('{"message":"someMessage","level":"warning"}'); - waitForXHR(xhr, function() { - Sentry.captureMessage("test"); - window.finalizeManualTest(); - }); - }).then(function(summary) { - // The async loader doesn't wrap XHR - if (IS_LOADER) { - return; - } - assert.equal(summary.breadcrumbs.length, 1); - assert.equal(summary.breadcrumbs[0].category, "sentry.event"); - assert.equal(summary.breadcrumbs[0].level, "warning"); - assert.equal(summary.breadcrumbs[0].message, "someMessage"); - }); - } - ); - - it( - optional( - "should transform XMLHttpRequests with transactions type to the Sentry store endpoint as sentry.transaction type breadcrumb", - IS_LOADER - ), - function() { - return runInSandbox(sandbox, { manual: true }, function() { - var store = - document.location.protocol + - "//" + - document.location.hostname + - (document.location.port ? ":" + document.location.port : "") + - "/api/1/store/" + - "?sentry_key=1337"; - - var xhr = new XMLHttpRequest(); - xhr.open("POST", store); - xhr.send( - '{"message":"someMessage","transaction":"wat","level":"warning", "type": "transaction"}' - ); - waitForXHR(xhr, function() { - Sentry.captureMessage("test"); - window.finalizeManualTest(); - }); - }).then(function(summary) { - // The async loader doesn't wrap XHR - if (IS_LOADER) { - return; - } - assert.equal(summary.breadcrumbs.length, 1); - assert.equal(summary.breadcrumbs[0].category, "sentry.transaction"); - assert.equal(summary.breadcrumbs[0].level, "warning"); - assert.equal(summary.breadcrumbs[0].message, "someMessage"); - }); - } - ); - - it( - optional( - "should not transform XMLHttpRequests with transactions attribute to the Sentry store endpoint as sentry.transaction type breadcrumb", - IS_LOADER - ), - function() { - return runInSandbox(sandbox, { manual: true }, function() { - var store = - document.location.protocol + - "//" + - document.location.hostname + - (document.location.port ? ":" + document.location.port : "") + - "/api/1/store/" + - "?sentry_key=1337"; - - var xhr = new XMLHttpRequest(); - xhr.open("POST", store); - xhr.send( - '{"message":"someMessage","transaction":"wat","level":"warning"}' - ); - waitForXHR(xhr, function() { - Sentry.captureMessage("test"); - window.finalizeManualTest(); - }); - }).then(function(summary) { - // The async loader doesn't wrap XHR - if (IS_LOADER) { - return; - } - assert.equal(summary.breadcrumbs.length, 1); - assert.equal(summary.breadcrumbs[0].category, "sentry.event"); - assert.equal(summary.breadcrumbs[0].level, "warning"); - assert.equal(summary.breadcrumbs[0].message, "someMessage"); - }); - } - ); - it("should record a fetch request", function() { return runInSandbox(sandbox, { manual: true }, function() { fetch("/base/subjects/example.json", { From 0b267a24c5f98dce44313812a314a4df9a5b7454 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 26 May 2020 16:08:20 +0200 Subject: [PATCH 3/7] chore: Changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 17a5f8edae40..ff3fae005444 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ - [core] fix: sent_at for envelope headers to use same clock #2597 - [apm] fix: Improve bundle size by moving span status to @sentry/apm #2589 - [apm] feat: No longer discard transactions instead mark them deadline exceeded #2588 +- [browser] fix: Emit Sentry Request breadcrumbs from inside the client (#2615) ## 5.15.5 From e60fe9969c3d2fdc8171418d974b9677ab00c4df Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 26 May 2020 16:08:36 +0200 Subject: [PATCH 4/7] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Kamil Ogórek --- packages/browser/src/client.ts | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/packages/browser/src/client.ts b/packages/browser/src/client.ts index 43e085475acc..c633341bd94a 100644 --- a/packages/browser/src/client.ts +++ b/packages/browser/src/client.ts @@ -3,7 +3,7 @@ import { DsnLike, Event, EventHint } from '@sentry/types'; import { getGlobalObject, logger } from '@sentry/utils'; import { BrowserBackend, BrowserOptions } from './backend'; -import { Breadcrumbs } from './integrations/index'; +import { Breadcrumbs } from './integrations'; import { SDK_NAME, SDK_VERSION } from './version'; /** @@ -79,22 +79,20 @@ export class BrowserClient extends BaseClient { this._processEvent(event, hint, scope) .then(finalEvent => { - // We need to check for finalEvent in case beforeSend returned null - eventId = finalEvent && finalEvent.event_id; + // We need to check for finalEvent in case beforeSend returned null. + if (!finalEvent) { + return; + } + eventId = finalEvent.event_id; // We do this here to not parse any requests if they are outgoing to Sentry // We log breadcrumbs if the integration has them enabled - if (finalEvent) { - const integration = this.getIntegration(Breadcrumbs); - if (integration) { - integration.addSentryBreadcrumb(finalEvent); - } + const integration = this.getIntegration(Breadcrumbs); + if (integration) { + integration.addSentryBreadcrumb(finalEvent); } - this._processing = false; }) - .then(null, reason => { - logger.error(reason); - this._processing = false; - }); + .catch(reason => logger.error(reason)) + .finally(() => (this._processing = false)); return eventId; } From 12b2ebc5ccf12c213eab4ca7b24799124a77046c Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 26 May 2020 16:13:42 +0200 Subject: [PATCH 5/7] Revert "Apply suggestions from code review" This reverts commit e60fe9969c3d2fdc8171418d974b9677ab00c4df. --- packages/browser/src/client.ts | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/packages/browser/src/client.ts b/packages/browser/src/client.ts index c633341bd94a..43e085475acc 100644 --- a/packages/browser/src/client.ts +++ b/packages/browser/src/client.ts @@ -3,7 +3,7 @@ import { DsnLike, Event, EventHint } from '@sentry/types'; import { getGlobalObject, logger } from '@sentry/utils'; import { BrowserBackend, BrowserOptions } from './backend'; -import { Breadcrumbs } from './integrations'; +import { Breadcrumbs } from './integrations/index'; import { SDK_NAME, SDK_VERSION } from './version'; /** @@ -79,20 +79,22 @@ export class BrowserClient extends BaseClient { this._processEvent(event, hint, scope) .then(finalEvent => { - // We need to check for finalEvent in case beforeSend returned null. - if (!finalEvent) { - return; - } - eventId = finalEvent.event_id; + // We need to check for finalEvent in case beforeSend returned null + eventId = finalEvent && finalEvent.event_id; // We do this here to not parse any requests if they are outgoing to Sentry // We log breadcrumbs if the integration has them enabled - const integration = this.getIntegration(Breadcrumbs); - if (integration) { - integration.addSentryBreadcrumb(finalEvent); + if (finalEvent) { + const integration = this.getIntegration(Breadcrumbs); + if (integration) { + integration.addSentryBreadcrumb(finalEvent); + } } + this._processing = false; }) - .catch(reason => logger.error(reason)) - .finally(() => (this._processing = false)); + .then(null, reason => { + logger.error(reason); + this._processing = false; + }); return eventId; } From 4f1b8a39b1f5197a97b8e1c23bb2d9d1fb6b92a2 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 26 May 2020 16:14:38 +0200 Subject: [PATCH 6/7] fix: Client --- packages/browser/src/client.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/browser/src/client.ts b/packages/browser/src/client.ts index 43e085475acc..2276d3b06ec6 100644 --- a/packages/browser/src/client.ts +++ b/packages/browser/src/client.ts @@ -3,7 +3,7 @@ import { DsnLike, Event, EventHint } from '@sentry/types'; import { getGlobalObject, logger } from '@sentry/utils'; import { BrowserBackend, BrowserOptions } from './backend'; -import { Breadcrumbs } from './integrations/index'; +import { Breadcrumbs } from './integrations'; import { SDK_NAME, SDK_VERSION } from './version'; /** From 85b04bcd85e8a483d61728ae4f7b6436d9a31da7 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 26 May 2020 22:28:41 +0200 Subject: [PATCH 7/7] ref: Fix if --- packages/browser/src/client.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/browser/src/client.ts b/packages/browser/src/client.ts index 2276d3b06ec6..ba10d63e63e4 100644 --- a/packages/browser/src/client.ts +++ b/packages/browser/src/client.ts @@ -80,10 +80,10 @@ export class BrowserClient extends BaseClient { this._processEvent(event, hint, scope) .then(finalEvent => { // We need to check for finalEvent in case beforeSend returned null - eventId = finalEvent && finalEvent.event_id; // We do this here to not parse any requests if they are outgoing to Sentry // We log breadcrumbs if the integration has them enabled if (finalEvent) { + eventId = finalEvent.event_id; const integration = this.getIntegration(Breadcrumbs); if (integration) { integration.addSentryBreadcrumb(finalEvent);