Skip to content

ref: Record dropped events in BaseClient #5017

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

Merged
merged 4 commits into from
Apr 29, 2022

Conversation

lforst
Copy link
Contributor

@lforst lforst commented Apr 29, 2022

Builds on #5008 to now track any dropped events on the BaseClient. Next we can implement the sending of the client reports on the SDK specific clients.

Ref: https://getsentry.atlassian.net/browse/WEB-775

@lforst lforst added this to the 7.0.0 milestone Apr 29, 2022
@lforst lforst requested review from Lms24 and AbhiPrasad April 29, 2022 08:12
Comment on lines 297 to 302
// We want to track each category (error, transaction, session) separately
// but still keep the distinction between different type of outcomes.
// We could use nested maps, but it's much easier to read and type this way.
// A correct type for map-based implementation if we want to go that route
// would be `Partial<Record<SentryRequestType, Partial<Record<Outcome, number>>>>`
// With typescript 4.1 we could even use template literal types
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably should mention here that I decided not to change this from the previous implementation: https://cs.github.com/getsentry/sentry-javascript/blob/4e722eb8778e27d7910e96ccb1aac108bcbea146/packages/browser/src/transports/base.ts#L101

@github-actions
Copy link
Contributor

github-actions bot commented Apr 29, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 18.45 KB (-8.41% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 57.68 KB (-10.73% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 17.21 KB (-8.78% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 51.88 KB (-10.51% 🔽)
@sentry/browser - Webpack (gzipped + minified) 18.89 KB (-18.7% 🔽)
@sentry/browser - Webpack (minified) 61.15 KB (-25.17% 🔽)
@sentry/react - Webpack (gzipped + minified) 18.92 KB (-18.73% 🔽)
@sentry/nextjs Client - Webpack (gzipped + minified) 42.61 KB (-11.33% 🔽)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 24.25 KB (-7.01% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 22.78 KB (-6.96% 🔽)

} else {
IS_DEBUG_BUILD && logger.error('Transport disabled');
}
public recordDroppedEvent(reason: EventDropReason, category: DataCategory): void {
Copy link
Member

Choose a reason for hiding this comment

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

This should only work if the sendClientReports option is true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can get away with recording the events, but only actually sending them when sendClientReports is true. At least that's how I envisioned it with #5018.

I would also find it a tad cleaner from a single-responsibility perspective. Wdyt? I'll let you decide 😄

Copy link
Member

Choose a reason for hiding this comment

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

I think we can get away with recording the events, but only actually sending them when sendClientReports is true

I was thinking problems with integer overflow or outcomes for long running applications (like a node server), but maybe not that big of a deal with what you showed before.

@lforst lforst merged commit dd1fb29 into 7.x Apr 29, 2022
@lforst lforst deleted the lforst-add-track-client-reports-in-baseclient branch April 29, 2022 15:52
AbhiPrasad pushed a commit that referenced this pull request May 30, 2022
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.

2 participants