feat: Add attachments API #5004

timfish · 2022-04-28T02:18:25Z

Closes #4996

This PR:

Adds Attachment interface and extends AttachmentItemHeaders with required fields
Adds addAttachment, getAttachments and clearAttachments to Scope
Adds attachments: Attachment[] to EventHint
In the private capture/process/prepare/send methods in the BaseClient, the EventHint argument has moved as it's no longer optional
- We can't mutate the hint if it's undefined!
Copies attachments from the scope to the hint in _prepareEvent
Adds optional textEncoder to InternalBaseTransportOptions and overrides this in the node client to support node 8-10
- Manually pass new TextEncoder() in many of the tests so they pass on node.js 8-10
Adds binary serialisation support for envelopes
- serializeEnvelope returns string | Uint8Array which all transports supported without modification
- Defaults to concatenating strings when no attachments are found. String concatenation is about 10x faster than the binary serialisation
Rewrites parseEnvelope in testutils.ts so that it can parse binary envelopes
- Is there a common test utils so this can be used elsewhere?

Bonus

This will significantly simplify sending minidumps from the Electron SDK

packages/core/src/baseclient.ts

AbhiPrasad · 2022-04-28T18:05:44Z

We could move the envelope serialisation to the client so we can have multiple implementations?

I like this idea. I think that it should be client defined, and we pass it into the transport as an option. We did something similar with @lforst's work on client reports. See: #5008

// core/src/baseclient.ts
this._transport = options.transport({
  serializeEnvelope: mySpecificSerializeEnvelopeImpl,
  ...options.transportOptions,
  url,
});

What to do about attachment rate limits?

The changes in #5008 should mean that we respect rate limits per envelope items. See:

sentry-javascript/packages/core/src/transports/base.ts

Lines 46 to 54 in 1c5bff0

    
           // Drop rate limited items from envelope 
        
           forEachEnvelopeItem(envelope, (item, type) => { 
        
             const envelopeItemDataCategory = envelopeItemTypeToDataCategory(type); 
        
             if (isRateLimited(rateLimits, envelopeItemDataCategory)) { 
        
               options.recordDroppedEvent('ratelimit_backoff', envelopeItemDataCategory); 
        
             } else { 
        
               filteredEnvelopeItems.push(item); 
        
             } 
        
           });

This will significantly simplify sending minidumps from the Electron SDK

Awesome ❤️

This was a quick first pass - I'll come back and do another review tomorrow when I get some time :)

timfish · 2022-05-01T12:58:30Z

We don't just need TextEncoder when serialising the envelope, we also need it outside the transport when creating the attachment envelope items. Attachments must include a length (in case that attachment contains newlines) and the easiest way to know the byte length of a utf8 string is to just convert it to bytes.

timfish · 2022-05-16T11:30:25Z

@AbhiPrasad I think this is ready for a 2nd pass.

It definitely still needs some tests to cover the changes around mutable hints + beforeSend + event processors.

AbhiPrasad

Were we able to test if babel will polyfill TextEncoder if they set the according browserslist?

AbhiPrasad · 2022-05-16T14:54:33Z

packages/browser/src/transports/utils.ts

@@ -86,7 +86,7 @@ export function getNativeFetchImplementation(): FetchImpl {
 * @param url report endpoint
 * @param body report payload
 */
-export function sendReport(url: string, body: string): void {
+export function sendReport(url: string, body: string | Uint8Array): void {


Why are we changing the func signature here?

serializeEnvelope now returns string | Uint8Array. In this case it will never actually be a Uint8Array because there's no attachment but both sendBeacon and fetch take body: string | Uint8Array so it seemed the best thing to do.

packages/core/src/baseclient.ts

packages/node/src/transports/http.ts

timfish · 2022-05-16T17:26:56Z

Were we able to test if babel will polyfill TextEncoder if they set the according browserslist?

It looks like babel now just expects you to import core-js if you want things polyfilled.

AbhiPrasad · 2022-05-17T17:33:50Z

Awesome, I think after the transport changes get extracted, we should be good to go to merge this in!

timfish · 2022-05-17T17:36:08Z

I think after the transport changes get extracted, we should be good to go to merge this in!

That is done already!

AbhiPrasad · 2022-05-17T17:58:54Z

I think after the transport changes get extracted, we should be good to go to merge this in!

That is done already!

Woooooops

AbhiPrasad

I think this is a good starting point for us - @getsentry/team-web-sdk-frontend I would appreciate another set of eyes to confirm!

lforst

Looks great. Thank you Tim!

timfish · 2022-05-17T19:58:12Z

How quickly can we get this into a release so I can test it all in this PR 😆

AbhiPrasad · 2022-05-17T21:09:18Z

I think we can merge tommorow, so get it out in a release by end of this week!

- Adds `Attachment` interface and extends `AttachmentItemHeaders` with required fields - Adds `addAttachment`, `getAttachments` and `clearAttachments` to `Scope` - Adds `attachments: Attachment[]` to `EventHint` - In the private capture/process/prepare/send methods in the `BaseClient`, the `EventHint` argument has moved as it's no longer optional - We can't mutate the hint if it's undefined! - Copies attachments from the scope to the hint in `_prepareEvent` - Adds optional `textEncoder` to `InternalBaseTransportOptions` and overrides this in the node client to support node 8-10 - Manually pass `new TextEncoder()` in many of the tests so they pass on node.js 8-10 - Adds binary serialisation support for envelopes - `serializeEnvelope` returns `string | Uint8Array` which all transports supported without modification - Defaults to concatenating strings when no attachments are found. String concatenation is about 10x faster than the binary serialisation - Rewrites `parseEnvelope` in `testutils.ts` so that it can parse binary envelopes

pnutmath · 2022-06-08T06:13:03Z

After upgrading sentry to v7 getting below error. I am using sentry with node express project - @timfish @AbhiPrasad

  message: 'finalScope.getAttachments is not a function or its return value is not iterable',
  code: undefined,
  sentry: '888a4d2d434b4e1c9b5dcea80eb99791',
  stack: 'TypeError: finalScope.getAttachments is not a function or its return value is not iterable\n' +
    '    at NodeClient._prepareEvent (/srv/server/node_modules/@sentry/core/cjs/baseclient.js:403:69)\n' +
    '    at NodeClient._prepareEvent (/srv/server/node_modules/@sentry/node/cjs/client.js:151:18)\n' +
    '    at NodeClient._processEvent (/srv/server/node_modules/@sentry/core/cjs/baseclient.js:598:17)\n' +
    '    at NodeClient._captureEvent (/srv/server/node_modules/@sentry/core/cjs/baseclient.js:554:17)\n' +
    '    at NodeClient.captureEvent (/srv/server/node_modules/@sentry/core/cjs/baseclient.js:147:12)\n' +
    '    at NodeClient.captureEvent (/srv/server/node_modules/@sentry/node/cjs/client.js:93:18)\n' +
    '    at Hub._invokeClient (/srv/server/node_modules/winston-sentry-log/node_modules/@sentry/hub/dist/hub.js:384:35)\n' +
    '    at Hub.captureEvent (/srv/server/node_modules/winston-sentry-log/node_modules/@sentry/hub/dist/hub.js:171:14)\n' +
    '    at Transaction.finish (/srv/server/node_modules/@sentry/tracing/cjs/transaction.js:142:22)\n' +
    '    at Immediate.<anonymous> (/srv/server/node_modules/@sentry/node/cjs/handlers.js:51:21)\n' +
    '    at processImmediate (internal/timers.js:464:21)\n' +
    '    at process.topLevelDomainCallback (domain.js:152:15)\n' +
    '    at process.callbackTrampoline (internal/async_hooks.js:128:24)'

lforst · 2022-06-08T07:47:56Z

Hi @pnutmath I moved your comment into a separate issue #5229

timfish added 7 commits April 28, 2022 02:55

Mostly there

ed65564

Fix tests

8db6659

Merge remote-tracking branch 'upstream/7.x' into feat/attachments

0dd456f

Fix test spy and add implementation for creating attachment item

7388581

Merge remote-tracking branch 'upstream/7.x' into feat/attachments

8d70b13

Load attachments from paths in node

dcef2ab

Few more minor fixes and additions

dbd70d1

AbhiPrasad added this to the 7.0.0 milestone Apr 28, 2022

AbhiPrasad reviewed Apr 28, 2022

View reviewed changes

packages/core/src/baseclient.ts Outdated Show resolved Hide resolved

timfish added 4 commits April 30, 2022 20:43

Merge remote-tracking branch 'upstream/7.x' into feat/attachments

9325e24

Parse binary envelopes in tests

1398bfc

Merge remote-tracking branch 'upstream/7.x' into feat/attachments

23e1f90

Change _attachmentsFromScope return type

93960c6

timfish force-pushed the feat/attachments branch from c16c1fa to 8d717d5 Compare May 1, 2022 04:50

Much improved

23f4b29

timfish force-pushed the feat/attachments branch from 8d717d5 to 23f4b29 Compare May 1, 2022 04:58

More simplify

fc3303c

timfish added 2 commits May 1, 2022 14:04

Final newline is optional

68e077a

Fix gatsby test

735812e

timfish force-pushed the feat/attachments branch 5 times, most recently from 35fd7ea to 2788c6c Compare May 1, 2022 22:11

Fix node v8/v10

fe7f49f

timfish force-pushed the feat/attachments branch from 2788c6c to fe7f49f Compare May 1, 2022 22:47

Fix node session tests

22a8f07

This comment was marked as resolved.

Sign in to view

timfish added 2 commits May 5, 2022 23:54

Remove the need for custom jest env

2d25f86

Fix gatsby tests

1731945

timfish force-pushed the feat/attachments branch from 62ebd9d to 1731945 Compare May 6, 2022 09:53

smeubank mentioned this pull request May 12, 2022

Attachment API isn't working as expected #5080

Closed

3 tasks

timfish added 5 commits May 12, 2022 10:21

Pass through hint

250260b

Merge remote-tracking branch 'upstream/7.x' into feat/attachments

723bfdb

Lint

21a04d8

Merge remote-tracking branch 'upstream/7.x' into feat/attachments

870ae81

Remove erroneous changes

be927c8

AbhiPrasad reviewed May 16, 2022

View reviewed changes

Review changes

c8d43eb

AbhiPrasad approved these changes May 17, 2022

View reviewed changes

lforst approved these changes May 17, 2022

View reviewed changes

AbhiPrasad merged commit 457b806 into getsentry:7.x May 18, 2022

timfish deleted the feat/attachments branch May 19, 2022 12:35

timfish mentioned this pull request May 19, 2022

ref(core): Test mutation of attachments in hint #5140

Merged

lforst mentioned this pull request May 24, 2022

NodeJS and Attachment #4371

Closed

AbhiPrasad mentioned this pull request May 30, 2022

Attachments API #4996

Closed

smirowstanitzok mentioned this pull request Jun 3, 2022

Usage of TextEncoderCommon causes compiler error in a setup without DOM Library #5199

Closed

3 tasks

lforst mentioned this pull request Jun 8, 2022

Problem with attachments API (getAttachments) #5229

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add attachments API #5004

feat: Add attachments API #5004

timfish commented Apr 28, 2022 •

edited

Loading

AbhiPrasad commented Apr 28, 2022 •

edited

Loading

timfish commented May 1, 2022 •

edited

Loading

This comment was marked as resolved.

timfish commented May 16, 2022

AbhiPrasad left a comment

AbhiPrasad May 16, 2022

timfish May 16, 2022

timfish commented May 16, 2022

AbhiPrasad commented May 17, 2022

timfish commented May 17, 2022

AbhiPrasad commented May 17, 2022

AbhiPrasad left a comment

lforst left a comment •

edited

Loading

timfish commented May 17, 2022 •

edited

Loading

AbhiPrasad commented May 17, 2022

pnutmath commented Jun 8, 2022

lforst commented Jun 8, 2022

feat: Add attachments API #5004

feat: Add attachments API #5004

Conversation

timfish commented Apr 28, 2022 • edited Loading

Bonus

AbhiPrasad commented Apr 28, 2022 • edited Loading

timfish commented May 1, 2022 • edited Loading

This comment was marked as resolved.

timfish commented May 16, 2022

AbhiPrasad left a comment

Choose a reason for hiding this comment

AbhiPrasad May 16, 2022

Choose a reason for hiding this comment

timfish May 16, 2022

Choose a reason for hiding this comment

timfish commented May 16, 2022

AbhiPrasad commented May 17, 2022

timfish commented May 17, 2022

AbhiPrasad commented May 17, 2022

AbhiPrasad left a comment

Choose a reason for hiding this comment

lforst left a comment • edited Loading

Choose a reason for hiding this comment

timfish commented May 17, 2022 • edited Loading

AbhiPrasad commented May 17, 2022

pnutmath commented Jun 8, 2022

lforst commented Jun 8, 2022

timfish commented Apr 28, 2022 •

edited

Loading

AbhiPrasad commented Apr 28, 2022 •

edited

Loading

timfish commented May 1, 2022 •

edited

Loading

lforst left a comment •

edited

Loading

timfish commented May 17, 2022 •

edited

Loading