Skip to content

feat(core): Expose prepareEvent util function #6517

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

Jump to bottom
Merged
merged 1 commit into from
Jan 3, 2023
Merged

feat(core): Expose prepareEvent util function #6517

merged 1 commit into from
Jan 3, 2023

Conversation

mydea
Copy link
Member

@mydea mydea commented Dec 13, 2022

So we can use this in replay, instead of using a private method from the client.

Also added a test to replay to make sure event processors work properly (which they didn't really before).

@mydea mydea added Package: core Issues related to the Sentry Core SDK Package: replay Issues related to the Sentry Replay SDK labels Dec 13, 2022
@mydea mydea self-assigned this Dec 13, 2022
mydea
mydea commented Dec 13, 2022
View reviewed changes
packages/core/src/utils/prepareEvent.ts
// because `getAttachments` can be undefined if users are using an older version
// of `@sentry/core` that does not have the `getAttachments` method.
// See: https://github.com/getsentry/sentry-javascript/issues/5229
if (finalScope) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Here a small change: Previously, this was:

if( finalScope && finalScope.getAttachments) {
  // ...
}

However, that meant that if a scope existed BUT no getAttachments, it would never run applyToEvent, which I guess is not desired. cc @AbhiPrasad , or was that on purpose this way?

Copy link
Member

Choose a reason for hiding this comment

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

This was to guard against cases where user's had different package versions and getAttachments was not defined on the scope object. This could happen if someone was using captureContext on a captureException call that passed in a old scope. This is on purpose, since then attachments didn't exist as a concept on the scope.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I get that, what I meant is that we actually guard for all the finalScope stuff this way, including this:

result = finalScope.applyToEvent(prepared, hint);

Is it desired that this will not be called when finalScope.getAttachments is not defined?

Copy link
Member

Choose a reason for hiding this comment

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

For future reference: We discussed this internally and IMO we can go with the change and revert this particular part if we get mismatch errors again.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note: We should still be handling mismatches, unless something inside of scope.applyToEvent relies on attachments to exists, which I think it shouldn't, as far as I can tell.

mydea
mydea commented Dec 13, 2022
View reviewed changes
packages/replay/jest.setup.ts Outdated
@@ -15,27 +15,6 @@ jest.mock('./src/util/isBrowser', () => {
};
});

afterEach(() => {
Copy link
Member Author

Choose a reason for hiding this comment

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

This seems to not be necessary anymore (and actually lead to test issues), so I removed it.

mydea
mydea commented Dec 13, 2022
View reviewed changes
packages/replay/src/replay.ts Outdated
@@ -947,6 +947,11 @@ export class ReplayContainer implements ReplayContainerInterface {

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

if (!replayEvent) {
Copy link
Member Author

Choose a reason for hiding this comment

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

We actually did not handle this case before (I guess types can be helpful after all xD), which can be the case when using an event processor.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 13, 2022 •
edited
Loading

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.78 KB (-0.02% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 61.28 KB (-0.05% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.56 KB (-0.04% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 54.86 KB (+0.05% 🔺)
@sentry/browser - Webpack (gzipped + minified) 20.32 KB (-0.1% 🔽)
@sentry/browser - Webpack (minified) 66.39 KB (-0.14% 🔽)
@sentry/react - Webpack (gzipped + minified) 20.34 KB (-0.12% 🔽)
@sentry/nextjs Client - Webpack (gzipped + minified) 47.6 KB (+0.08% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.74 KB (-0.05% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 25.18 KB (-0.03% 🔽)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 42.09 KB (-0.01% 🔽)
@sentry/replay - Webpack (gzipped + minified) 38.17 KB (0%)

@mydea mydea mentioned this pull request Dec 13, 2022
Closed
5 tasks
billyvg
billyvg reviewed Dec 13, 2022
View reviewed changes
Copy link
Member

@billyvg billyvg left a comment

Choose a reason for hiding this comment

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

LGTM from the replay side of things

packages/replay/src/replay.ts Outdated
@@ -947,6 +947,11 @@ export class ReplayContainer implements ReplayContainerInterface {

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

if (!replayEvent) {
__DEBUG_BUILD__ && logger.log('[Replay] Event was dropped.');
Copy link
Member

Choose a reason for hiding this comment

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

How do we handle other events when they get dropped?

Copy link
Member Author

Choose a reason for hiding this comment

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

This happens here:

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

Line 489 in b74d9ab

this.recordDroppedEvent('event_processor', event.type || 'error', event);

I guess we could also use recordDroppedEvent here 🤔 I'll give it a look!

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated this to also record dropped replay events, and left a comment as well referencing where this is taken from!

@mydea mydea mentioned this pull request Dec 14, 2022
Merged
@mydea
Copy link
Member Author

mydea commented Dec 14, 2022

Note: I've actually split this into two PRs, one where we expose this, and #6522 where we start using this in replay.

Lms24
Lms24 approved these changes Dec 14, 2022
View reviewed changes
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

LGTM!

@mydea mydea merged commit b16ea89 into master Jan 3, 2023
@mydea mydea deleted the fn/prepareEvent branch January 3, 2023 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@billyvg billyvg billyvg left review comments

@Lms24 Lms24 Lms24 approved these changes

@lforst lforst Awaiting requested review from lforst

@AbhiPrasad AbhiPrasad Awaiting requested review from AbhiPrasad

Assignees

@mydea mydea

Labels
Package: core Issues related to the Sentry Core SDK Package: replay Issues related to the Sentry Replay SDK
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mydea @billyvg @Lms24 @AbhiPrasad