Skip to content

ref(replay): Send SDK's name in replay event #6514

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Dec 14, 2022
Merged

ref(replay): Send SDK's name in replay event #6514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cab74fa
ref(replay): Send SDK's name in replay event
Lms24 Dec 13, 2022
76be0ce
adjust test
Lms24 Dec 13, 2022
4c09f4a
add new envelope creation test
Lms24 Dec 13, 2022
73d63b3
fix test and lint error
Lms24 Dec 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions packages/replay/src/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,3 @@ export const MAX_SESSION_LIFE = 1_800_000; // 30 minutes
*/
export const DEFAULT_SESSION_SAMPLE_RATE = 0.1;
export const DEFAULT_ERROR_SAMPLE_RATE = 1.0;

export const REPLAY_SDK_INFO = {
name: 'sentry.javascript.integration.replay',
version: __SENTRY_REPLAY_VERSION__,
};
5 changes: 2 additions & 3 deletions packages/replay/src/replay.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -939,7 +939,6 @@ export class ReplayContainer implements ReplayContainerInterface {
};

const replayEvent = await getReplayEvent({ scope, client, replayId, event: baseEvent });

replayEvent.tags = {
...replayEvent.tags,
sessionSampleRate: this._options.sessionSampleRate,
Expand Down Expand Up @@ -971,8 +970,8 @@ export class ReplayContainer implements ReplayContainerInterface {
"BrowserTracing",
"Replay"
],
"name": "sentry.javascript.integration.replay",
"version": "7.24.2"
"name": "sentry.javascript.browser",
"version": "7.25.0"
},
"sdkProcessingMetadata": {},
"tags": {
Expand Down
5 changes: 2 additions & 3 deletions packages/replay/src/util/createReplayEnvelope.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,17 @@
import { Envelope, Event } from '@sentry/types';
import { createEnvelope } from '@sentry/utils';

import { REPLAY_SDK_INFO } from '../constants';

export function createReplayEnvelope(
replayId: string,
replayEvent: Event,
payloadWithSequence: string | Uint8Array,
): Envelope {
const { name, version } = replayEvent.sdk || {};
return createEnvelope(
{
event_id: replayId,
sent_at: new Date().toISOString(),
sdk: REPLAY_SDK_INFO,
sdk: { name, version },
Copy link
Member

Choose a reason for hiding this comment

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

l: Should we maybe just pass sdk: replayEvent.sdk here?

Copy link
Member Author

Choose a reason for hiding this comment

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

For envelopes in the SDK, we only send name and version in the header:

sentry-javascript/packages/core/src/envelope.ts

Lines 16 to 24 in 2591d2a

/** Extract sdk info from from the API metadata */
function getSdkMetadataForEnvelopeHeader(metadata?: SdkMetadata): SdkInfo | undefined {
if (!metadata || !metadata.sdk) {
return;
}
const { name, version } = metadata.sdk;
return { name, version };
}

According to the dev spec, we can send the entire sdk object though, so we could change it... I think for now we should go with SDK consistency, WDYT? Keeps the envelope a little smaller if we don't pass the integrations array twice for instance

Copy link
Member

Choose a reason for hiding this comment

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

ahh, ok, I see. Sorry, this was/is a bit confusing for me, what goes on the event vs. what goes on the envelope 😅

},
[
// @ts-ignore New types
Expand Down
9 changes: 6 additions & 3 deletions packages/replay/src/util/getReplayEvent.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import { Scope } from '@sentry/core';
import { Client, Event } from '@sentry/types';

import { REPLAY_SDK_INFO } from '../constants';

export async function getReplayEvent({
client,
scope,
Expand All @@ -18,9 +16,14 @@ export async function getReplayEvent({
// @ts-ignore private api
const preparedEvent: Event = await client._prepareEvent(event, { event_id }, scope);

// extract the SDK name because `client._prepareEvent` doesn't add it to the event
const metadata = client.getOptions() && client.getOptions()._metadata;
const { name } = (metadata && metadata.sdk) || {};

preparedEvent.sdk = {
Copy link
Member

Choose a reason for hiding this comment

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

l: IMHO we don't need to overwrite this like that, as it will never be set. I'd just do:

preparedEvent.sdk = {
  name, 
  version: 'xxx'
}

Or, maybe export the getSdkMetadataForEnvelopeHeader function or similar from core and use that somehow (but I'd say that's optional).

Copy link
Member Author

Choose a reason for hiding this comment

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

If we don't use the ...preparedEvent.sdk spread, we don't add the entire SDK metadata to the event. For instance, the integrations array won't be added. I think it's fine to omit it in the envelope header but I don't know if we should remove it from the event.

getSdkMetadataForEnvelopeHeader would only make sense for constructing the envelope header. I'll take a look if this reduces bundle size

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, we can use getSdkMetadataForEnvelopeHeader. Will do this in a follow-up PR though, as it involves moving stuff around.

...preparedEvent.sdk,
...REPLAY_SDK_INFO,
version: __SENTRY_REPLAY_VERSION__,
name,
};

return preparedEvent;
Expand Down
62 changes: 62 additions & 0 deletions packages/replay/test/unit/util/createReplayEnvelope.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import { Event } from '@sentry/types';

import { createReplayEnvelope } from '../../../src/util/createReplayEnvelope';

describe('createReplayEnvelope', () => {
it('creates an envelope for a given Replay event', () => {
const replayId = '1234';
const replayEvent = {
type: 'replay_event',
timestamp: 1670837008.634,
error_ids: ['errorId'],
trace_ids: ['traceId'],
urls: ['https://example.com'],
replay_id: 'eventId',
segment_id: 3,
platform: 'javascript',
event_id: 'eventId',
environment: 'production',
sdk: {
integrations: ['BrowserTracing', 'Replay'],
name: 'sentry.javascript.browser',
version: '7.25.0',
},
tags: {
sessionSampleRate: 1,
errorSampleRate: 0,
replayType: 'error',
},
};
const payloadWithSequence = 'payload';

const envelope = createReplayEnvelope(replayId, replayEvent as Event, payloadWithSequence);

expect(envelope).toEqual([
{
event_id: '1234',
sdk: { name: 'sentry.javascript.browser', version: '7.25.0' },
sent_at: expect.any(String),
},
[
[
{ type: 'replay_event' },
{
environment: 'production',
error_ids: ['errorId'],
event_id: 'eventId',
platform: 'javascript',
replay_id: 'eventId',
sdk: { integrations: ['BrowserTracing', 'Replay'], name: 'sentry.javascript.browser', version: '7.25.0' },
segment_id: 3,
tags: { errorSampleRate: 0, replayType: 'error', sessionSampleRate: 1 },
timestamp: 1670837008.634,
trace_ids: ['traceId'],
type: 'replay_event',
urls: ['https://example.com'],
},
],
[{ length: 7, type: 'replay_recording' }, 'payload'],
],
]);
});
});
2 changes: 1 addition & 1 deletion packages/replay/test/unit/util/getReplayEvent.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ describe('getReplayEvent', () => {
event_id: 'replay-ID',
environment: 'production',
sdk: {
name: 'sentry.javascript.integration.replay',
name: 'sentry.javascript.browser',
version: 'version:Test',
},
sdkProcessingMetadata: {},
Expand Down