Skip to content

ref(replay): Log warning if sample rates are all undefined - #6959

Merged
Lms24 merged 5 commits into
masterfrom
lms-replay-sample-rate-warning
Jan 30, 2023
Merged

ref(replay): Log warning if sample rates are all undefined#6959
Lms24 merged 5 commits into
masterfrom
lms-replay-sample-rate-warning

Conversation

@Lms24

@Lms24Lms24 commented Jan 27, 2023

Copy link
Copy Markdown
Member

Since we decided to set both replay sample rates to 0 by default, we should log a warning that replay will be disabled if no sample rates were passed to Sentry.init. Since we're still supporting the deprecated sample rates in the integration options, this warning will only be emitted if these rates aren't set either.

Had to do some refactoring to nail down the logic as previously, default values were applied too early

@Lms24
Lms24 requested review from billyvg and mydeaJanuary 27, 2023 18:55
Comment threadpackages/replay/test/mocks/mockSdk.ts Outdated
@github-actions

github-actionsBot commented Jan 27, 2023

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
@sentry/browser - ES5 CDN Bundle (gzipped + minified)19.85 KB (-0.01% 🔽)
@sentry/browser - ES5 CDN Bundle (minified)61.55 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)18.52 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified)54.85 KB (0%)
@sentry/browser - Webpack (gzipped + minified)20.24 KB (0%)
@sentry/browser - Webpack (minified)66.25 KB (0%)
@sentry/react - Webpack (gzipped + minified)20.27 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified)47.56 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)26.77 KB (0%)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)25.06 KB (-0.01% 🔽)
@sentry/replay ES6 CDN Bundle (gzipped + minified)44.15 KB (+1.01% 🔺)
@sentry/replay - Webpack (gzipped + minified)38.9 KB (+1.16% 🔺)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified)61.47 KB (+0.71% 🔺)

Comment threadpackages/replay/src/integration.ts Outdated
@Lms24
Lms24force-pushed the lms-replay-sample-rate-warning branch from 6b5a8c8 to 095dd92CompareJanuary 30, 2023 11:20
@Lms24Lms24 changed the title ref(replay): Log warning if sample rates are all 0ref(replay): Log warning if sample rates are all undefinedJan 30, 2023
Comment threadpackages/replay/src/integration.ts Outdated
Comment threadpackages/replay/src/integration.ts Outdated
const finalOptions = { sessionSampleRate: 0, errorSampleRate: 0, ...dropUndefinedKeys(initialOptions) };

if (!opt) {
return finalOptions;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

m: Hmm in this case we'll also want to check for the warning, I guess?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I'd argue that in this case, something more fundamental is wrong with the SDK than anything Replay specific. We should always get defined options from the client.

Thinking about it, the typecast above unnecessarily includes | undefined. I think it should be good to be removed.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

argh my jet lag isn't helping here - the client could be undefined which is what we're actually checking for here 😅

So yes, we probably should emit another warning there but I'd still argue that it is more a fundamental SDK problem than just the sample rates.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

jup, right! Maybe we just add a more generic client not found type message there?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Done 7314883

@Lms24
Lms24force-pushed the lms-replay-sample-rate-warning branch from 8eef3df to 7314883CompareJanuary 30, 2023 15:10

@mydeamydea left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! 👍

@Lms24
Lms24 merged commit 15ec85b into masterJan 30, 2023
@Lms24
Lms24 deleted the lms-replay-sample-rate-warning branch January 30, 2023 15:47
Sign up for freeto 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.

3 participants

@Lms24@billyvg@mydea