-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(core): Add replay_event
type for events
#6481
Conversation
@@ -233,7 +233,7 @@ export class Hub implements HubInterface { | |||
*/ | |||
public captureEvent(event: Event, hint?: EventHint): string { | |||
const eventId = hint && hint.event_id ? hint.event_id : uuid4(); | |||
if (event.type !== 'transaction') { | |||
if (!event.type) { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
sentry-javascript/packages/hub/test/hub.test.ts
Lines 360 to 373 in c68d429
test('transactions do not set lastEventId', () => { | |
const event: Event = { | |
extra: { b: 3 }, | |
type: 'transaction', | |
}; | |
const testClient = makeClient(); | |
const hub = new Hub(testClient); | |
hub.captureEvent(event); | |
const args = getPassedArgs(testClient.captureEvent); | |
expect(args[1].event_id).not.toEqual(hub.lastEventId()); | |
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in 37535a0
71839e0
to
2a68ccf
Compare
size-limit report 📦
|
@@ -233,7 +233,7 @@ export class Hub implements HubInterface { | |||
*/ | |||
public captureEvent(event: Event, hint?: EventHint): string { | |||
const eventId = hint && hint.event_id ? hint.event_id : uuid4(); | |||
if (event.type !== 'transaction') { | |||
if (!event.type) { |
There was a problem hiding this comment.
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.
@@ -233,7 +233,7 @@ export class Hub implements HubInterface { | |||
*/ | |||
public captureEvent(event: Event, hint?: EventHint): string { | |||
const eventId = hint && hint.event_id ? hint.event_id : uuid4(); | |||
if (event.type !== 'transaction') { | |||
if (!event.type) { |
There was a problem hiding this comment.
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:
sentry-javascript/packages/hub/test/hub.test.ts
Lines 360 to 373 in c68d429
test('transactions do not set lastEventId', () => { | |
const event: Event = { | |
extra: { b: 3 }, | |
type: 'transaction', | |
}; | |
const testClient = makeClient(); | |
const hub = new Hub(testClient); | |
hub.captureEvent(event); | |
const args = 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'; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me.
37535a0
to
4e35eb8
Compare
@@ -22,7 +19,7 @@ export function handleGlobalEventListener(replay: ReplayContainer): (event: Even | |||
|
|||
// Only tag transactions with replayId if not waiting for an error | |||
// @ts-ignore private | |||
if (event.type !== 'transaction' || !replay._waitForError) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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 "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
4e35eb8
to
0e74f6a
Compare
0e74f6a
to
4e87368
Compare
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
andreplay_recording
. These are sent in one envelope, wherereplay_event
contains the replay metadata etc, andreplay_recording
the actual recording as string.IMHO for the
event
type we have here it is sufficient to havereplay_event
, as thereplay_recording
is not really passed around but only need to be considered when we (eventually) build the envelope.