Skip to content

feat(core): Add replay_event type for events - #6481

Merged
mydea merged 4 commits into
masterfrom
fn/add-replay-types
Jan 5, 2023
Merged

feat(core): Add replay_event type for events#6481
mydea merged 4 commits into
masterfrom
fn/add-replay-types

Conversation

@mydea

@mydeamydea commented Dec 9, 2022

Copy link
Copy Markdown
Member

As a first step for #6480, this adds the replay event type.

Note that replay is a bit special, in that it has two event types: replay_event and replay_recording. These are sent in one envelope, where replay_event contains the replay metadata etc, and replay_recording the actual recording as string.

IMHO for the event type we have here it is sufficient to have replay_event, as the replay_recording is not really passed around but only need to be considered when we (eventually) build the envelope.

@mydeamydea self-assigned this Dec 9, 2022
public captureEvent(event: Event, hint?: EventHint): string {
const eventId = hint && hint.event_id ? hint.event_id : uuid4();
if (event.type !== 'transaction') {
if (!event.type) {

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: I guess replays should not count here? (Eventually - currently they do not go through this method at all)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading #3966, I think you're right.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might also add a test for the new event type here:

test('transactions do not set lastEventId',()=>{
constevent: Event={
extra: {b: 3},
type: 'transaction',
};
consttestClient=makeClient();
consthub=newHub(testClient);
hub.captureEvent(event);
constargs=getPassedArgs(testClient.captureEvent);
expect(args[1].event_id).not.toEqual(hub.lastEventId());
});

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in 37535a0

@mydea
mydeaforce-pushed the fn/add-replay-types branch from 71839e0 to 2a68ccfCompareDecember 9, 2022 08:47
@github-actions

github-actionsBot commented Dec 9, 2022

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
@sentry/browser - ES5 CDN Bundle (gzipped + minified)19.82 KB (+0.1% 🔺)
@sentry/browser - ES5 CDN Bundle (minified)61.36 KB (+0.08% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)18.6 KB (+0.11% 🔺)
@sentry/browser - ES6 CDN Bundle (minified)54.92 KB (+0.08% 🔺)
@sentry/browser - Webpack (gzipped + minified)20.37 KB (+0.1% 🔺)
@sentry/browser - Webpack (minified)66.54 KB (+0.07% 🔺)
@sentry/react - Webpack (gzipped + minified)20.39 KB (+0.1% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified)47.64 KB (+0.04% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)26.79 KB (+0.09% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)25.22 KB (+0.07% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified)42.99 KB (-0.03% 🔽)
@sentry/replay - Webpack (gzipped + minified)37.68 KB (-0.02% 🔽)

public captureEvent(event: Event, hint?: EventHint): string {
const eventId = hint && hint.event_id ? hint.event_id : uuid4();
if (event.type !== 'transaction') {
if (!event.type) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading #3966, I think you're right.

public captureEvent(event: Event, hint?: EventHint): string {
const eventId = hint && hint.event_id ? hint.event_id : uuid4();
if (event.type !== 'transaction') {
if (!event.type) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might also add a test for the new event type here:

test('transactions do not set lastEventId',()=>{
constevent: Event={
extra: {b: 3},
type: 'transaction',
};
consttestClient=makeClient();
consthub=newHub(testClient);
hub.captureEvent(event);
constargs=getPassedArgs(testClient.captureEvent);
expect(args[1].event_id).not.toEqual(hub.lastEventId());
});


// TODO: Handle replay_event here
// Currently, this is done by replay, but we want to upstream this here
const eventType = event.type && event.type !== 'replay_event' ? event.type : 'event';

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To note, this is technically correct, but may increase the bundle size slightly for "nothing", as this will not really be hit right now. I'd still rather leave this in and remove it once we (hopefully soon!) actually handle replay events "normally". WDYT?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me.

@mydea
mydeaforce-pushed the fn/add-replay-types branch 2 times, most recently from 37535a0 to 4e35eb8CompareDecember 9, 2022 12:31

// Only tag transactions with replayId if not waiting for an error
// @ts-ignore private
if (event.type !== 'transaction' || !replay._waitForError) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for this change?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be explicit that we only want this when the event is an error. This is IMHO more future proof, as when/if we add further event types, we'll need to explicitly handle them.

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@mydea@billyvg@Lms24