Skip to content

Differentiate between internal Event and envelope Event types  #10222

Closed

Description

@Lms24

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:

  • Accessing e.g. span.op makes our linter (and VSCode) complain because according to Event['spans'], op is deprecated
    • In reality, span.op will continue to be sent as long as we don't fully drop it in favor of the span.attributes['sentry.op'] property.
  • Using spanToJSON(span) will return an empty object instead of the JSON object because span has no getSpanJSON method.
  • In v8, span.op will be removed according to the type, but in fact still be present in the envelope.

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.

Activity

  1. added this to the 8.0.0 milestone on Jan 18, 2024
  2. Lms24 commented on Jan 18, 2024

    @Lms24
    MemberAuthor

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

  3. Lms24 commented on Jan 18, 2024

    @Lms24
    MemberAuthor

    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);
  4. modified the milestones: 8.0.0, on Jun 4, 2024
  5. lforst commented on Nov 11, 2024

    @lforst
    Contributor

    @Lms24 I would err on closing this because the spans type is already SpanJSON. Do we still have a concrete need for this?

  6. Lms24 commented on Nov 11, 2024

    @Lms24
    MemberAuthor

    yup, I think we fixed this in v8 implicitly but forgot to close this. Closing now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions