getsentry / sentry-javascript

Official Sentry SDKs for JavaScript
https://sentry.io
MIT License
7.88k stars 1.55k forks source link

Differentiate between internal `Event` and envelope `Event` types #10222

Open Lms24 opened 8 months ago

Lms24 commented 8 months ago

Problem Statement

With our Span/Transcation API changes, the Event types are diverging between what properties of an event we can use and access while the event is processed in the SDK vs. what data is sent to Sentry as an EventItem in an envelope.

This was always "problematic" in the sense that e.g. serialized spans would never have methods although the EventItem type permits that. However, it has only become noticeable with us deprecating (and in v8 removing) properties from event payloads like for example a Span.

A concrete place where this is now causing difficulties for us are the Playwright browser integration tests. Here we assume that when we intercept an event envelope, the payload will be of type Event. However, in reality, it's not and functions like spanToJSON can't work correctly because they require methods on the span and not just a serialized and re-parsed JSON object. This leads to the weird situation that:

Note: span.op serves as a good example but is not the only affected field.

Solution Brainstorm

Change the envelope EventItem type

// envelope.ts

// from
export type EventItem = BaseEnvelopeItem<EventItemHeaders, Event>;

// to
export type EventItem = BaseEnvelopeItem<EventItemHeaders, SerializedEvent>;

It's gonna be quite a challenge to figure out the subtle differences when creating SerializedEvent but this would be more correct overall.

Lms24 commented 8 months ago

10240 adds the type, switching to it from Event can only happen in v8

Lms24 commented 8 months ago

More evidence that we need the SerializedEvent type:

We have integration tests wrongfully using the Event type. Whenever we check for event.spans' properties, TS will suggest and check with the wrong types. So for instance, we had a test that tried to check for a spanId, parentSpanId match and both times we used the camelCase syntax vs. the snake_case one. Ironically, the tests passed because undefined === undefined 😭

// wrong but passes
const rootSpanId = transaction?.contexts?.trace?.spanId;
expect(transaction.spans?.[0]).toEqual(rootSpanId);

// correct but TS complains
const rootSpanId = transaction?.contexts?.trace?.span_id;
expect(transaction.spans?.[0]).toEqual(root_span_id);