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

fix: set session sample rate configurable and defaults to 0 #29

Merged
merged 2 commits into from
May 13, 2024

Conversation

abdullahwaheed
Copy link
Contributor

No description provided.

@@ -35,8 +35,8 @@ class DatadogLoggingService extends NewRelicLoggingService {
service: process.env.DATADOG_SERVICE,
env: process.env.DATADOG_ENV,
version: process.env.DATADOG_VERSION,
sessionSampleRate: 50,
sessionReplaySampleRate: 100,
sessionSampleRate: parseInt(process.env.SESSION_SAMPLE_RATE || 0, 10),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the 0 be the default? Should that be 50?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we want sessionSampleRate default to 50 for all MFEs? if yes then i'll update it

sessionSampleRate: 50,
sessionReplaySampleRate: 100,
sessionSampleRate: parseInt(process.env.SESSION_SAMPLE_RATE || 0, 10),
sessionReplaySampleRate: parseInt(process.env.SESSION_REPLAY_SAMPLE_RATE || 0, 10),
Copy link
Contributor

Choose a reason for hiding this comment

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

0 default seems right for this for now. Should we add a comment that we are defaulting to 0 until we work out privacy concerns?

@@ -47,7 +47,7 @@ class DatadogLoggingService extends NewRelicLoggingService {
site: process.env.DATADOG_SITE,
env: process.env.DATADOG_ENV,
forwardErrorsToLogs: true,
sessionSampleRate: 100,
sessionSampleRate: parseInt(process.env.SESSION_SAMPLE_RATE || 0, 10),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want 100 as our default for now?

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 was thinking to default everything to 0 and then configure from envs. but if we want default values set, i'll update it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should sessionSampleRate for both RUM and Browser logging differ?

i am currently using same env for both

Copy link
Contributor

@robrap robrap left a comment

Choose a reason for hiding this comment

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

I changed my mind. 0 will work, with explicit settings for each MFE.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (master@8054e3d). Click here to learn what that means.

Additional details and impacted files
@@            Coverage Diff            @@
##             master      #29   +/-   ##
=========================================
  Coverage          ?   95.83%           
=========================================
  Files             ?        1           
  Lines             ?       48           
  Branches          ?       17           
=========================================
  Hits              ?       46           
  Misses            ?        2           
  Partials          ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@abdullahwaheed abdullahwaheed merged commit 7599e32 into master May 13, 2024
5 checks passed
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.

3 participants