Skip to content

ref: Add tests for breadcrumbs #2620

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

Merged
merged 2 commits into from
May 27, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/browser/examples/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@ class HappyTransport extends Sentry.Transports.BaseTransport {
event,
);

return {
return Promise.resolve({
status: 'success',
};
});
}
}

Expand Down
30 changes: 6 additions & 24 deletions packages/browser/src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,30 +73,12 @@ export class BrowserClient extends BaseClient<BrowserBackend, BrowserOptions> {
/**
* @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
// 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);
}
}
this._processing = false;
})
.then(null, reason => {
logger.error(reason);
this._processing = false;
});

return eventId;
protected _sendEvent(event: Event): void {
const integration = this.getIntegration(Breadcrumbs);
if (integration) {
integration.addSentryBreadcrumb(event);
}
super._sendEvent(event);
}

/**
Expand Down
6 changes: 5 additions & 1 deletion packages/browser/test/integration/common/init.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,11 @@ function initSDK() {
}

// One of the tests use manually created breadcrumb without eventId and we want to let it through
if (breadcrumb.category.indexOf("sentry" === 0) && breadcrumb.event_id) {
if (
breadcrumb.category.indexOf("sentry" === 0) &&
breadcrumb.event_id &&
!window.allowSentryBreadcrumbs
) {
return null;
}

Expand Down
1 change: 1 addition & 0 deletions packages/browser/test/integration/karma.conf.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ module.exports = config => {
"/base/variants/123": "/base/subjects/123",
// Supresses warnings
"/api/1/store/": "/",
"/api/1/envelope/": "/",
},
frameworks: ["mocha", "chai", "sinon"],
files,
Expand Down
50 changes: 50 additions & 0 deletions packages/browser/test/integration/suites/api.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,56 @@ describe("API", function() {
});
});

it("should capture Sentry internal event as breadcrumbs for the following event sent", function() {
return runInSandbox(sandbox, { manual: true }, function() {
window.allowSentryBreadcrumbs = true;
Sentry.captureMessage("a");
Sentry.captureMessage("b");
// For the loader
Sentry.flush && Sentry.flush(2000);
window.finalizeManualTest();
}).then(function(summary) {
assert.equal(summary.events.length, 2);
assert.equal(summary.breadcrumbs.length, 2);
assert.equal(summary.events[1].breadcrumbs[0].category, "sentry.event");
assert.equal(
summary.events[1].breadcrumbs[0].event_id,
summary.events[0].event_id
);
assert.equal(
summary.events[1].breadcrumbs[0].level,
summary.events[0].level
);
});
});

it("should capture Sentry internal transaction as breadcrumbs for the following event sent", function() {
return runInSandbox(sandbox, { manual: true }, function() {
window.allowSentryBreadcrumbs = true;
Sentry.captureEvent({
event_id: "aa3ff046696b4bc6b609ce6d28fde9e2",
message: "someMessage",
transaction: "wat",
type: "transaction",
});
Sentry.captureMessage("c");
// For the loader
Sentry.flush && Sentry.flush(2000);
window.finalizeManualTest();
}).then(function(summary) {
// We have a length of one here since transactions don't go through beforeSend
// and we add events to summary in beforeSend
assert.equal(summary.events.length, 1);
assert.equal(summary.breadcrumbs.length, 2);
assert.equal(
summary.events[0].breadcrumbs[0].category,
"sentry.transaction"
);
assert.isNotEmpty(summary.events[0].breadcrumbs[0].event_id);
assert.isUndefined(summary.events[0].breadcrumbs[0].level);
});
});

it("should generate a synthetic trace for captureException w/ non-errors", function() {
return runInSandbox(sandbox, function() {
throwNonError();
Expand Down
40 changes: 16 additions & 24 deletions packages/core/src/baseclient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,15 +91,8 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement

this._getBackend()
.eventFromException(exception, hint)
.then(event => this._processEvent(event, hint, scope))
.then(finalEvent => {
// We need to check for finalEvent in case beforeSend returned null
eventId = finalEvent && finalEvent.event_id;
this._processing = false;
})
.then(null, reason => {
logger.error(reason);
this._processing = false;
.then(event => {
eventId = this.captureEvent(event, hint, scope);
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like eventId is always overwritten? Even if hint.event_id is provided? If so, we can:

return this._getBackend()
      .eventFromException(exception, hint)
      .then(event => this.captureEvent(event, hint, scope));

Also this_processing = true; is already called inside captureEvent so we can most likely remove it from here? Same goes for eventId extraction, as it's done in captureEvent too.

Copy link
Member Author

@HazAT HazAT May 27, 2020

Choose a reason for hiding this comment

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

This is tricky so I would leave it like this, because depending on if there is really an async call in between this is true or not.
_isProcessing should also be called before first thing since stuff in captureX can be async

});

return eventId;
Expand All @@ -110,24 +103,15 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
*/
public captureMessage(message: string, level?: Severity, hint?: EventHint, scope?: Scope): string | undefined {
let eventId: string | undefined = hint && hint.event_id;

this._processing = true;

const promisedEvent = isPrimitive(message)
? this._getBackend().eventFromMessage(`${message}`, level, hint)
: this._getBackend().eventFromException(message, hint);

promisedEvent
.then(event => this._processEvent(event, hint, scope))
.then(finalEvent => {
// We need to check for finalEvent in case beforeSend returned null
eventId = finalEvent && finalEvent.event_id;
this._processing = false;
})
.then(null, reason => {
logger.error(reason);
this._processing = false;
});
promisedEvent.then(event => {
eventId = this.captureEvent(event, hint, scope);
});
Comment on lines +112 to +114
Copy link
Member Author

@HazAT HazAT May 27, 2020

Choose a reason for hiding this comment

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

@kamilogorek No idea why we didn't do it like this in the first place.
Now everything goes through captureEvent as it's supposed to be.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch

Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Whole method can be written as:

const promisedEvent = // ...;

return promisedEvent.then(event => this.captureEvent(event, hint, scope);


return eventId;
}
Expand Down Expand Up @@ -372,6 +356,14 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
}
}

/**
* Tells the backend to send this event
* @param event The Sentry event to send
*/
protected _sendEvent(event: Event): void {
this._getBackend().sendEvent(event);
}

Comment on lines +359 to +366
Copy link
Member Author

Choose a reason for hiding this comment

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

New internal function, helper for that.

/**
* Processes an event (either error or message) and sends it to Sentry.
*
Expand Down Expand Up @@ -413,7 +405,7 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
const isInternalException = hint && hint.data && (hint.data as { [key: string]: any }).__sentry__ === true;
// We skip beforeSend in case of transactions
if (isInternalException || !beforeSend || isTransaction) {
this._getBackend().sendEvent(finalEvent);
this._sendEvent(finalEvent);
resolve(finalEvent);
return;
}
Expand All @@ -434,7 +426,7 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
}

// From here on we are really async
this._getBackend().sendEvent(finalEvent);
this._sendEvent(finalEvent);
resolve(finalEvent);
}
})
Expand Down Expand Up @@ -467,7 +459,7 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
return;
}
// From here on we are really async
this._getBackend().sendEvent(processedEvent);
this._sendEvent(processedEvent);
resolve(processedEvent);
})
.then(null, e => {
Expand Down