Conversation
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,
});
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
Awesome ❤️ This was a quick first pass - I'll come back and do another review tomorrow when I get some time :) |
|
We don't just need |
timfish
force-pushed
the
feat/attachments
branch
5 times, most recently
from
35fd7ea to
2788c6c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
|
@AbhiPrasad I think this is ready for a 2nd pass. It definitely still needs some tests to cover the changes around mutable hints + |
AbhiPrasad
left a comment
AbhiPrasad
left a comment
There was a problem hiding this comment.
Were we able to test if babel will polyfill TextEncoder if they set the according browserslist?
| * @param body report payload | ||
| */ | ||
| export function sendReport(url: string, body: string): void { | ||
| export function sendReport(url: string, body: string | Uint8Array): void { |
There was a problem hiding this comment.
Why are we changing the func signature here?
There was a problem hiding this comment.
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.
It looks like babel now just expects you to import |
|
Awesome, I think after the transport changes get extracted, we should be good to go to merge this in! |
That is done already! |
Woooooops
|
AbhiPrasad
left a comment
AbhiPrasad
left a comment
There was a problem hiding this comment.
I think this is a good starting point for us - @getsentry/team-web-sdk-frontend I would appreciate another set of eyes to confirm!
|
How quickly can we get this into a release so I can test it all in this PR 😆 |
|
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
|
After upgrading sentry to v7 getting below error. I am using sentry with node express project - @timfish @AbhiPrasad |
5 participants
Closes #4996
This PR:
Attachmentinterface and extendsAttachmentItemHeaderswith required fieldsaddAttachment,getAttachmentsandclearAttachmentstoScopeattachments: Attachment[]toEventHintBaseClient, theEventHintargument has moved as it's no longer optional_prepareEventtextEncodertoInternalBaseTransportOptionsand overrides this in the node client to support node 8-10new TextEncoder()in many of the tests so they pass on node.js 8-10serializeEnvelopereturnsstring | Uint8Arraywhich all transports supported without modificationparseEnvelopeintestutils.tsso that it can parse binary envelopesBonus