Skip to content

Conversation

@JonasBa
Copy link
Member

@JonasBa JonasBa commented Dec 7, 2022

Adds profile envelope type so we can skip the ts-expect error in the profiling sdk.

@JonasBa JonasBa requested review from AbhiPrasad and lforst December 7, 2022 22:19
@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.68 KB (+0.04% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 60.96 KB (+0.03% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.47 KB (+0.07% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 54.51 KB (+0.04% 🔺)
@sentry/browser - Webpack (gzipped + minified) 20.27 KB (+0.04% 🔺)
@sentry/browser - Webpack (minified) 66.25 KB (+0.03% 🔺)
@sentry/react - Webpack (gzipped + minified) 20.29 KB (+0.04% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified) 47.29 KB (+0.02% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.67 KB (+0.03% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 25.12 KB (+0.05% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 41.8 KB (+0.1% 🔺)
@sentry/replay - Webpack (gzipped + minified) 37.88 KB (+0.01% 🔺)

| 'internal';
| 'internal'
// Profile event type
| 'profile';
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this change is breaking in a very unfortunate way. We use the DataCategory type as a callback argument in a hook that can be configured by the users:

recordDroppedEvent(reason: EventDropReason, dataCategory: DataCategory, event?: Event): void;

We essentially have two options:

  • Disregard this fact under the assumption that it won't matter for many people.
  • Add logic that prevents 'profile' from being passed to this hook.

I am fine with both.

Copy link
Member

Choose a reason for hiding this comment

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

Disregard this fact under the assumption that it won't matter for many people

I'm in favor of this, recordDroppedEvent is not going to be used by the 99.9% of folks, we can add a note to the changelog about it.

Copy link
Member Author

@JonasBa JonasBa Dec 9, 2022

Choose a reason for hiding this comment

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

@lforst yeah, I figured. As it stands today with the profiling SDK implementation, I dont think recordDroppedEvent will even be called for profiles (we just early return if sampling rate is lower than random). I think for now this is fine, but as we integrate more tightly with the SDK, we can fix this and actually pass a profile to the recordDroppedEvent if necessary

@AbhiPrasad AbhiPrasad changed the title feat(profiling): add profile envelope item type feat(types): add profile envelope item type Dec 9, 2022
@AbhiPrasad
Copy link
Member

@JonasBa gonna make the scope types instead of profiling - we typically use the main package affected as the scope name.

@AbhiPrasad
Copy link
Member

@JonasBa
Copy link
Member Author

JonasBa commented Dec 9, 2022

Oh yes @AbhiPrasad, that's my bad

@JonasBa
Copy link
Member Author

JonasBa commented Dec 9, 2022

@JonasBa do we have to update the event.ts types like we did here: https://github.com/getsentry/sentry-javascript/pull/6481/files#diff-f23e180558fc344ec82d146663395c076559f45fd59418e4ba94bcb0b1dc102f?

Probably. Let me take a look

@JonasBa JonasBa merged commit 8342523 into master Dec 9, 2022
@JonasBa JonasBa deleted the jb/envelope/profile-type branch December 9, 2022 20:43
Sign up for free to 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.

4 participants