Skip to content
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

ref(backend): Port functionality from Backend to Client #4911

Merged
merged 13 commits into from
Apr 12, 2022

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Apr 11, 2022

This PR ports all the functionality from the Backend classes to the Client classes. This includes:

  • Backend (interface)
  • BaseBackend
  • BrowserBackend
  • NodeBackend
  • TestBackend

Additionally, the PR fixes the unit tests in Core to spy on TestClient instead of TestBackend. Furthermore, all places where the Backend classes should be removed are marked with a TODO(v7) comment. The removal of the Backend classes will happen in a separate PR.

Ref:

@Lms24 Lms24 requested review from lforst and AbhiPrasad April 11, 2022 10:14
@Lms24 Lms24 changed the base branch from master to 7.x April 11, 2022 10:15
@github-actions
Copy link
Contributor

github-actions bot commented Apr 11, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 20.11 KB (-0.17% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 65.46 KB (+1.32% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.81 KB (-0.27% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 58.69 KB (+1.26% 🔺)
@sentry/browser - Webpack (gzipped + minified) 23.13 KB (-0.45% 🔽)
@sentry/browser - Webpack (minified) 82.66 KB (+1.16% 🔺)
@sentry/react - Webpack (gzipped + minified) 23.17 KB (-0.45% 🔽)
@sentry/nextjs Client - Webpack (gzipped + minified) 47.95 KB (-0.21% 🔽)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.07 KB (-0.03% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 24.46 KB (-0.1% 🔽)

Copy link
Member

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

Looks great!

packages/core/src/baseclient.ts Outdated Show resolved Hide resolved
packages/core/src/baseclient.ts Show resolved Hide resolved
packages/ember/package.json Outdated Show resolved Hide resolved
@Lms24
Copy link
Member Author

Lms24 commented Apr 11, 2022

For some reason, these two old browser integration tests keep failing:

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);
});
});

Seems like summary.breadcrumbs has a length of 0. Still working on finding the cause.

@AbhiPrasad AbhiPrasad added this to the 7.0.0 milestone Apr 11, 2022
@Lms24 Lms24 force-pushed the lms-backend-to-client branch from 62ec8da to 99bde12 Compare April 11, 2022 16:13
@@ -582,7 +654,7 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
this._updateSessionFromEvent(session, processedEvent);
}

this._sendEvent(processedEvent);
this.sendEvent(processedEvent);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.sendEvent(processedEvent);
this._sendEvent(processedEvent);

This should have stayed _sendEvent, which then itself will call sendEvent. By bypassing _sendEvent, you're also bypassing the super._sendEvent call, so event breadcrumbs are never getting attached. That's why your tests are failing.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch! Let's make sure we address this when we get to reviewing the client inheritance chain + looking at all the indirection in the SDK (part 6). We can maybe make the event processing chain smaller and easier to understand.

I made some tickets that went over this.

@Lms24 Lms24 force-pushed the lms-backend-to-client branch from 6fbf745 to 67563bb Compare April 12, 2022 07:28
@Lms24 Lms24 force-pushed the lms-backend-to-client branch from 67563bb to 3ce8093 Compare April 12, 2022 07:41
@Lms24 Lms24 merged commit 7079520 into 7.x Apr 12, 2022
@Lms24 Lms24 deleted the lms-backend-to-client branch April 12, 2022 08:12
lobsterkatie pushed a commit that referenced this pull request Apr 13, 2022
port all the functionality from the Backend classes to the Client classes. This includes:

* Backend (interface)
* BaseBackend
* BrowserBackend
* NodeBackend
* TestBackend

Additionally, fix the unit and integration tests in Core to spy on TestClient instead of TestBackend.
Lms24 added a commit that referenced this pull request Apr 26, 2022
port all the functionality from the Backend classes to the Client classes. This includes:

* Backend (interface)
* BaseBackend
* BrowserBackend
* NodeBackend
* TestBackend

Additionally, fix the unit and integration tests in Core to spy on TestClient instead of TestBackend.
lobsterkatie pushed a commit that referenced this pull request Apr 26, 2022
port all the functionality from the Backend classes to the Client classes. This includes:

* Backend (interface)
* BaseBackend
* BrowserBackend
* NodeBackend
* TestBackend

Additionally, fix the unit and integration tests in Core to spy on TestClient instead of TestBackend.
lobsterkatie pushed a commit that referenced this pull request Apr 26, 2022
port all the functionality from the Backend classes to the Client classes. This includes:

* Backend (interface)
* BaseBackend
* BrowserBackend
* NodeBackend
* TestBackend

Additionally, fix the unit and integration tests in Core to spy on TestClient instead of TestBackend.
AbhiPrasad pushed a commit that referenced this pull request May 30, 2022
port all the functionality from the Backend classes to the Client classes. This includes:

* Backend (interface)
* BaseBackend
* BrowserBackend
* NodeBackend
* TestBackend

Additionally, fix the unit and integration tests in Core to spy on TestClient instead of TestBackend.
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.

3 participants