chore: Add missing fields into Replay NetworkRequestData type by ryan953 · Pull Request #8284 · getsentry/sentry-javascript

ryan953 · 2023-06-02T20:05:47Z

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

If you've added code that should be tested, please add tests.
Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

mydea · 2023-06-05T07:48:45Z

I think this is not what you think it is :D You are probably looking for ReplayNetworkRequestData? NetworkRequestData is basically the old breadcrumb type we used - I wonder if we can/should streamline this (probably...).

ryan953 · 2023-06-05T16:46:30Z

I think this is not what you think it is :D You are probably looking for ReplayNetworkRequestData? NetworkRequestData is basically the old breadcrumb type we used - I wonder if we can/should streamline this (probably...).

@mydea What i'm trying to do is import types into sentry so i can have well-typed breadcrumbs/attachments/segments/frames (whatever they're called now) within the ui. this the the data that comes back from the /recording-segments/ api endpoint.
The ReplayNetworkRequestData doesn't look like it's exported, and this whole file has been freshly added with the correct frame wrappers and stuff. So I was thinking this is the correct place, but I still need to make sure the type matches what shape real objects have.

billyvg · 2023-06-05T17:54:48Z

@ryan953 I can fix this up, what branch on sentry can I test on?

ryan953 · 2023-06-05T18:23:52Z

@billyvg I copy+pasted all the types into ryan953/ref-replay-test-mocks and ryan953/ref-replay-test-mocks uses them even more. So either of those branches would work.

ryan953 · 2023-06-05T18:32:00Z

@billyvg I'm also waiting to merge those two linked PRs after the next SDK bump, so I don't need to copy+paste all the types in. So no rush.

billyvg · 2023-06-05T23:14:35Z

@ryan953 OK I may not actually have too much time to work on this until after the overhead project. ~~I think there's a disconnect here -- FetchBreadcrumbData is wrong here and it is probably ReplayNetworkRequestData that we need there~~ Actually I don't think FetchFrame belongs here at all...

billyvg · 2023-06-05T23:22:21Z

@ryan953 Ok here are the types for the network request/resp, I think we need to add ReplayNetworkRequestData, but NetworkRequestData still needs to be kept there for backwards compatibility.

FetchFrame/XhrFrame does not exist as a BreadcrumbFrame

github-actions · 2023-06-06T18:35:33Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	21.12 KB (+0.46% 🔺)
@sentry/browser - ES5 CDN Bundle (minified)	65.86 KB (+0.37% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	19.65 KB (+0.46% 🔺)
@sentry/browser - ES6 CDN Bundle (minified)	58.34 KB (+0.44% 🔺)
@sentry/browser - Webpack (gzipped + minified)	21.28 KB (+0.53% 🔺)
@sentry/browser - Webpack (minified)	69.26 KB (+0.33% 🔺)
@sentry/react - Webpack (gzipped + minified)	21.31 KB (+0.54% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified)	49.23 KB (+0.25% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	28.74 KB (+0.35% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	26.98 KB (+0.37% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified)	48.63 KB (+2.14% 🔺)
@sentry/replay - Webpack (gzipped + minified)	42.22 KB (+1.82% 🔺)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified)	67.59 KB (+1.63% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified)	60.5 KB (+1.89% 🔺)

billyvg · 2023-06-06T20:20:37Z

packages/replay/src/types/index.ts

 export * from './replay';
 export * from './replayFrame';
 export * from './rrweb';
+export type { EventType } from '@sentry-internal/rrweb';


Hmm we have this duplicated here but I forgot why... might be because our fork of rrweb, it wasn't exported? cc @mydea

I took that duplicate definition out, as you saw below.

Now getsentry/sentry-javascript is importing from '@sentry-internal/rrweb' and getsentry/sentry will import from getsentry/sentry-javascript
hopefully i can skip @sentry-internal/rrweb altogether if i've cross referenced everything correctly.

packages/replay/src/types/replayFrame.ts

billyvg · 2023-06-06T20:23:35Z

packages/replay/src/types/replayFrame.ts

+
+interface NetworkRequestFrame extends BaseSpanFrame {
+  data: NetworkRequestData | ReplayNetworkRequestData;
+  op: 'resource.fetch'


What about xhr?

I think xhr payloads are shaped like ResourceData instead.

packages/replay/src/types/rrweb.ts

…ReplayNetworkRequestOrResponse added into NetworkRequestData

packages/replay/src/types/performance.ts

ryan953 · 2023-06-06T21:08:43Z

packages/replay/src/types/index.ts

 export * from './replay';
 export * from './replayFrame';
 export * from './rrweb';
+export type { EventType } from '@sentry-internal/rrweb';


I took that duplicate definition out, as you saw below.

Now getsentry/sentry-javascript is importing from '@sentry-internal/rrweb' and getsentry/sentry will import from getsentry/sentry-javascript
hopefully i can skip @sentry-internal/rrweb altogether if i've cross referenced everything correctly.

ryan953 · 2023-06-06T21:09:02Z

packages/replay/src/types/replayFrame.ts

+  sessionSampleRate: number;
+  useCompression: boolean;
+  useCompressionOption: boolean;
 }


sorted these fields, no changes.

this object is duplicated in sentry, so i'm happy for the re-use here.

ryan953 · 2023-06-06T21:10:33Z

packages/replay/src/types/replayFrame.ts

+
+interface NetworkRequestFrame extends BaseSpanFrame {
+  data: NetworkRequestData | ReplayNetworkRequestData;
+  op: 'resource.fetch'


I think xhr payloads are shaped like ResourceData instead.

ryan953 · 2023-06-07T04:47:22Z

packages/replay/src/types/index.ts

 export * from './performance';
 export * from './replay';
 export * from './replayFrame';
+export * from './request';


new file to prevent circular refs.

mydea · 2023-06-09T09:06:31Z

Huh, tests did not actually pass here - it seems it cancelled on build step (which we sadly take as "this is fine", apparently). This broke tests 😬 I'll fix it here: https://github.com/getsentry/sentry-javascript/actions/runs/5220067685/jobs/9422724589?pr=8310

This breaks, as we do not have these packages as dependencies but devDependencies, but the types are not inlined... This was introduced here: #8284 Fixes #8326

ryan953 requested review from Lms24 and billyvg and removed request for billyvg June 2, 2023 20:05

ryan953 marked this pull request as draft June 3, 2023 00:12

ryan953 requested a review from mydea June 5, 2023 16:40

ryan953 changed the title ~~chrore: Add missing fields into Replay NetworkRequestData type~~ chore: Add missing fields into Replay NetworkRequestData type Jun 5, 2023

ryan953 closed this Jun 5, 2023

ryan953 reopened this Jun 5, 2023

ryan953 force-pushed the ryan953/chore-NetworkRequestData-fields branch from 5b7d63d to 732589e Compare June 6, 2023 18:24

Reset

13092c3

ryan953 force-pushed the ryan953/chore-NetworkRequestData-fields branch from 732589e to 13092c3 Compare June 6, 2023 18:27

refactor SpanFrame to use a base class and map op to data

ce4889d

ryan953 marked this pull request as ready for review June 6, 2023 19:41

billyvg reviewed Jun 6, 2023

View reviewed changes

ryan953 added 2 commits June 6, 2023 13:27

cleanup extra exports that are not needed

4db6b20

Replace ReplayNetworkRequestData, which has start/endTimestamp, with …

889cb13

…ReplayNetworkRequestOrResponse added into NetworkRequestData

billyvg reviewed Jun 6, 2023

View reviewed changes

packages/replay/src/types/performance.ts Show resolved Hide resolved

billyvg approved these changes Jun 6, 2023

View reviewed changes

ryan953 commented Jun 7, 2023

View reviewed changes

ryan953 added 2 commits June 6, 2023 18:07

break circular dependency

de36842

prettier

0ec4343

billyvg approved these changes Jun 7, 2023

View reviewed changes

ryan953 added 2 commits June 6, 2023 19:59

combine xhr and fetch request types

dbcc440

re-export rrweb event/node types and enums

69f73ee

ryan953 force-pushed the ryan953/chore-NetworkRequestData-fields branch from 308fc44 to 69f73ee Compare June 7, 2023 03:49

ryan953 mentioned this pull request Jun 7, 2023

test(replay): Create test stubs for all replay frames and frame-data getsentry/sentry#50246

Merged

ryan953 force-pushed the ryan953/chore-NetworkRequestData-fields branch from 0d0fefb to 69f73ee Compare June 7, 2023 03:54

ryan953 commented Jun 7, 2023

View reviewed changes

ryan953 merged commit b877c10 into develop Jun 7, 2023

ryan953 deleted the ryan953/chore-NetworkRequestData-fields branch June 7, 2023 16:36

mydea mentioned this pull request Jun 14, 2023

fix(replay): Do not export types from @sentry-internal/rrweb #8329

Merged

Uh oh!

Conversation

ryan953 commented Jun 2, 2023

Uh oh!

mydea commented Jun 5, 2023

Uh oh!

ryan953 commented Jun 5, 2023

Uh oh!

billyvg commented Jun 5, 2023

Uh oh!

ryan953 commented Jun 5, 2023

Uh oh!

ryan953 commented Jun 5, 2023

Uh oh!

billyvg commented Jun 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

billyvg commented Jun 5, 2023

Uh oh!

github-actions bot commented Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mydea commented Jun 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

billyvg commented Jun 5, 2023 •

edited

Loading

github-actions bot commented Jun 6, 2023 •

edited

Loading