fix(utils): Fix envelope parsing edge cases #13191
Conversation
This PR backports the new envelope parser in getsentry/spotlight#447 as it fixes some edge cases that the current parser does not handle. This covers binary payloads and implicit ending with EOF and line endings, and empty envelopes. The PR also adds tests cases for all these and more.
| // data sanitization | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access | ||
| if (itemHeader.type && typeof itemPayload === 'object') { | ||
| /* eslint-disable @typescript-eslint/no-unsafe-member-access */ | ||
| // @ts-expect-error -- Does not like assigning to `type` on random object | ||
| itemPayload.type = itemHeader.type; | ||
| } |
There was a problem hiding this comment.
The root of all the test changes with type: 'event' or type: 'span' etc is this. I assumed Spotlight was the correct one here and all events should have a proper type that said in many places, we assume error events to not have any type.
I feel like an explicit type is important so even if this implementation is not correct, we should have a default event type like 'event' or 'error'.
Anyway, I'll leave this to the reviewers and will address change requests accordingly.
There was a problem hiding this comment.
h: I wouldn't do this because we use parseEnvelope in all our tests to assert on what exactly our SDK sends to Sentry. Adding fields that were not included in the envelope when it was serialized by the SDK would be really confusing.
There was a problem hiding this comment.
Would you mind elaborating what concrete edge case this fix fixes?
I might be missing some context but generally, Spotlight's envelope parsing logic needs to handle envelopes from all SDKs while the JS SDK's parseEnvelope function only handles JS SDK-sent envelopes.
That being said, if there's a bug in the logic, I'm happy to merge a fix.
For some context, we use this function
- in the SDK's optional
offlineTransport(code), meaning it is used in production code - in various E2E and integration tests for us to check what exactly we send to Sentry.
- That's also why I want to avoid adding fields during parsing that are not present beforehand.
| @@ -79,6 +79,7 @@ sentryTest( | |||
| start_timestamp: expect.any(Number), | |||
| timestamp: expect.any(Number), | |||
| trace_id: traceId, | |||
| type: 'span', | |||
There was a problem hiding this comment.
h: We should not add this field to standalone span envelopes.
| @@ -119,7 +119,7 @@ sentryTest('error after navigation has navigation traceId', async ({ getLocalTes | |||
| await page.locator('#errorBtn').click(); | |||
| const [errorEvent, errorTraceHeader] = await errorEventPromise; | |||
|
|
|||
| expect(errorEvent.type).toEqual(undefined); | |||
| expect(errorEvent.type).toEqual('event'); | |||
There was a problem hiding this comment.
h: let's not change this please unless there's a good reason to (is there?). So far, sending error events without a type worked well.
| @@ -3,7 +3,7 @@ import { waitForError, waitForTransaction } from '@sentry-internal/test-utils'; | |||
|
|
|||
| test('sends an error', async ({ page }) => { | |||
| const errorPromise = waitForError('ember-classic', async errorEvent => { | |||
| return !errorEvent.type; | |||
| return errorEvent.type !== 'transaction'; | |||
There was a problem hiding this comment.
m: I don't think this change is correct because event.type could be a lot more than 'transaction' or undefined. However, it probably doesn't matter because waitForError already checks for type === undefined before even invoking the callback :)
| // data sanitization | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access | ||
| if (itemHeader.type && typeof itemPayload === 'object') { | ||
| /* eslint-disable @typescript-eslint/no-unsafe-member-access */ | ||
| // @ts-expect-error -- Does not like assigning to `type` on random object | ||
| itemPayload.type = itemHeader.type; | ||
| } |
There was a problem hiding this comment.
h: I wouldn't do this because we use parseEnvelope in all our tests to assert on what exactly our SDK sends to Sentry. Adding fields that were not included in the envelope when it was serialized by the SDK would be really confusing.
There was a problem hiding this comment.
m: I don't think we should test against payloads from other language SDKs. parseEnvelope is used in the JS SDK and it only needs to work with JS SDK-sent events.
|
Scrapping this PR as the decision is to keep the |
This PR backports the new envelope parser in getsentry/spotlight#447 as it fixes some edge cases that the current parser does not handle. This covers binary payloads and implicit ending with EOF and line endings, and empty envelopes. The PR also adds test cases for all these and more.