Skip to content
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

Jump to bottom

feat(browser): Client Report Support #3955

Merged
merged 3 commits into from Sep 20, 2021
Merged

feat(browser): Client Report Support #3955

merged 3 commits into from Sep 20, 2021

Conversation

kamilogorek
Copy link
Contributor

@kamilogorek kamilogorek commented Sep 3, 2021 •
edited

This adds support for client reports to the Browser SDK. This will cause the SDK to send a report once the tab goes hidden or has been closed and will send it as a separate envelope.

The feature can be turned off via the sendClientReports option which is on by default.

Refs:
getsentry/relay#1074
getsentry/sentry-python#1181

@github-actions
Copy link
Contributor

github-actions bot commented Sep 3, 2021 •
edited

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 22.23 KB (0%)
@sentry/browser - Webpack 23.22 KB (0%)
@sentry/react - Webpack 23.26 KB (0%)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 29.67 KB (0%)

AbhiPrasad
AbhiPrasad reviewed Sep 7, 2021
View reviewed changes
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Quick review, will do in more detail after DACI is ✅

packages/browser/src/transports/base.ts Show resolved Hide resolved
packages/browser/src/transports/base.ts Show resolved Hide resolved
packages/browser/src/transports/base.ts Show resolved Hide resolved
packages/core/src/baseclient.ts Outdated Show resolved Hide resolved
iker-barriocanal
iker-barriocanal reviewed Sep 8, 2021
View reviewed changes
Copy link
Member

@iker-barriocanal iker-barriocanal left a comment

Choose a reason for hiding this comment

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

Haven't looked at tests yet.

packages/types/src/transport.ts Outdated Show resolved Hide resolved
packages/types/src/options.ts Outdated Show resolved Hide resolved
packages/browser/src/transports/base.ts Outdated Show resolved Hide resolved
packages/browser/src/transports/base.ts Outdated Show resolved Hide resolved
packages/browser/src/transports/base.ts Outdated Show resolved Hide resolved
@kamilogorek kamilogorek force-pushed the client-report branch 2 times, most recently from e107443 to 7d924e7 Compare September 8, 2021 12:39
@kamilogorek kamilogorek changed the title feat(browser): SDK Outcomes feat(browser): Client Report Support Sep 9, 2021
@kamilogorek kamilogorek marked this pull request as ready for review September 9, 2021 15:32
@kamilogorek kamilogorek force-pushed the client-report branch 6 times, most recently from cd5162c to 0e585c2 Compare September 10, 2021 11:23
AbhiPrasad
AbhiPrasad approved these changes Sep 10, 2021
View reviewed changes
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Ok nice, I think we are good to go

packages/browser/src/transports/base.ts Show resolved Hide resolved
packages/browser/src/transports/fetch.ts Show resolved Hide resolved
@kamilogorek
Copy link
Contributor Author

We'll cut the beta release from this branch, so I will hold with merging it to master.

iker-barriocanal
iker-barriocanal approved these changes Sep 13, 2021
View reviewed changes
Copy link
Member

@iker-barriocanal iker-barriocanal left a comment

Choose a reason for hiding this comment

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

LGTM, just minor feedback.

packages/types/src/transport.ts Outdated Show resolved Hide resolved
packages/browser/src/transports/fetch.ts Show resolved Hide resolved
packages/browser/test/unit/transports/fetch.test.ts Show resolved Hide resolved
packages/browser/test/unit/transports/fetch.test.ts Show resolved Hide resolved
packages/browser/src/transports/xhr.ts Show resolved Hide resolved
@iker-barriocanal
Copy link
Member

🚀

@kamilogorek kamilogorek force-pushed the client-report branch 2 times, most recently from 73480d5 to a7eaf2f Compare September 15, 2021 10:18
@kamilogorek kamilogorek merged commit 44033d1 into master Sep 20, 2021
@kamilogorek kamilogorek deleted the client-report branch September 20, 2021 11:24
@smeubank smeubank added this to the Client Reports milestone Sep 30, 2021
@smeubank smeubank mentioned this pull request Dec 7, 2021
Closed
This was referenced Feb 1, 2022
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhcarvalho rhcarvalho rhcarvalho left review comments

@AbhiPrasad AbhiPrasad AbhiPrasad approved these changes

@iker-barriocanal iker-barriocanal iker-barriocanal approved these changes

@mitsuhiko mitsuhiko Awaiting requested review from mitsuhiko

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Client Reports
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kamilogorek @iker-barriocanal @rhcarvalho @AbhiPrasad @smeubank