ref(backend): Port functionality from Backend to Client by Lms24 · Pull Request #4911 · getsentry/sentry-javascript

Lms24 · 2022-04-11T10:14:51Z

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:

github-actions · 2022-04-11T10:31:48Z

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% 🔽)

AbhiPrasad

Looks great!

packages/core/src/baseclient.ts

packages/ember/package.json

Lms24 · 2022-04-11T13:40:24Z

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

sentry-javascript/packages/browser/test/integration/suites/api.js

Lines 23 to 62 in 4e722eb

    
           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.

lobsterkatie · 2022-04-11T17:47:00Z

packages/core/src/baseclient.ts

        }

-        this._sendEvent(processedEvent);
+        this.sendEvent(processedEvent);


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.

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.

port assertions checking TestBackend to inspect TestClient instead. Adjust spies to spy on ported functionality of TestBackend in TestClient

mark more removal TODOs

…ent`

calling `sendEvent()` instead of `_sendEvent()` in `BaseClient` does not leads to the super classe's `_sendEvent()` not being called.

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 requested review from AbhiPrasad and lforst April 11, 2022 10:14

Lms24 changed the base branch from master to 7.x April 11, 2022 10:15

AbhiPrasad approved these changes Apr 11, 2022

View reviewed changes

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

AbhiPrasad added this to the 7.0.0 milestone Apr 11, 2022

Lms24 force-pushed the lms-backend-to-client branch from 62ec8da to 99bde12 Compare April 11, 2022 16:13

lobsterkatie reviewed Apr 11, 2022

View reviewed changes

Lms24 force-pushed the lms-backend-to-client branch from 6fbf745 to 67563bb Compare April 12, 2022 07:28

Lms24 added 13 commits April 12, 2022 09:40

port BaseBackend to BaseClient

7425bdb

port Backend interface to Client

c7e18d3

port BrowserBackend to BrowserClient

894293b

port NodeBackend to NodeClient

79e05ef

port TestBackend to TestClient

339a2b9

fix core unit tests

b182d20

port assertions checking TestBackend to inspect TestClient instead. Adjust spies to spy on ported functionality of TestBackend in TestClient

move NoopTransport test to Core Client tests

98dea38

fix linter errors

d2d60f9

fix backend accesses in tests

ad7832b

mark more removal TODOs

remove more backend references

13943b1

fix ember package.json

4f99134

make eventFromException and eventFromMessage abstract in `BaseCli…

6710740

…ent`

fix incorrect sendEvent() call

3ce8093

calling `sendEvent()` instead of `_sendEvent()` in `BaseClient` does not leads to the super classe's `_sendEvent()` not being called.

Lms24 force-pushed the lms-backend-to-client branch from 67563bb to 3ce8093 Compare April 12, 2022 07:41

Lms24 merged commit 7079520 into 7.x Apr 12, 2022

Lms24 deleted the lms-backend-to-client branch April 12, 2022 08:12

Lms24 mentioned this pull request Apr 12, 2022

ref(backend): Delete Backend classes #4919

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

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

ref(backend): Port functionality from Backend to Client#4911
Lms24 merged 13 commits into7.xfrom
lms-backend-to-client

Lms24 commented Apr 11, 2022 •

edited by lforst

Loading

Uh oh!

github-actions bot commented Apr 11, 2022 •

edited

Loading

Uh oh!

AbhiPrasad left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Lms24 commented Apr 11, 2022

Uh oh!

lobsterkatie Apr 11, 2022

Uh oh!

AbhiPrasad Apr 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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

Uh oh!

Comments

Conversation

Lms24 commented Apr 11, 2022 • edited by lforst Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Apr 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

AbhiPrasad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Lms24 commented Apr 11, 2022

Uh oh!

lobsterkatie Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

AbhiPrasad Apr 12, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Lms24 commented Apr 11, 2022 •

edited by lforst

Loading

github-actions bot commented Apr 11, 2022 •

edited

Loading