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: never send when not sampled #1706

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pauldambra
Copy link
Member

see: https://posthoghelp.zendesk.com/agent/tickets/24373

If a sampling decision has been made and a session should not be recorded

but event or URL triggers are defined

the trigger pending status takes precedence and we build up a buffer

when the trigger is seen we send that buffer

and then the sampling status can assert itself and the recording immediately stops

this lifts the check for sampled is False higher in the status check to avoid this happening

@pauldambra pauldambra requested a review from veryayskiy January 30, 2025 15:16
@pauldambra pauldambra added the bump patch Bump patch version when this PR gets merged label Jan 30, 2025
Copy link

vercel bot commented Jan 30, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
posthog-js ✅ Ready (Inspect) Visit Preview Jan 30, 2025 4:11pm

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR modifies session recording behavior to prevent unnecessary data collection when sampling indicates a session should not be recorded, even if event/URL triggers are defined.

  • Modified /src/extensions/replay/sessionrecording.ts to prioritize sampling status over trigger states in recording decisions
  • Added test case in /src/__tests__/extensions/replay/sessionrecording.test.ts to verify recording is disabled when sampling rate is 0
  • Fixes issue where trigger pending status could override sampling decisions, leading to unwanted data collection
  • Ensures recording buffer is not built up when session is explicitly not sampled

2 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

src/__tests__/extensions/replay/sessionrecording.test.ts Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Jan 30, 2025

Size Change: +310 B (+0.01%)

Total Size: 3.28 MB

Filename Size Change
dist/array.full.es5.js 266 kB +31 B (+0.01%)
dist/array.full.js 369 kB +31 B (+0.01%)
dist/array.full.no-external.js 368 kB +31 B (+0.01%)
dist/array.js 181 kB +31 B (+0.02%)
dist/array.no-external.js 180 kB +31 B (+0.02%)
dist/main.js 182 kB +31 B (+0.02%)
dist/module.full.js 369 kB +31 B (+0.01%)
dist/module.full.no-external.js 368 kB +31 B (+0.01%)
dist/module.js 181 kB +31 B (+0.02%)
dist/module.no-external.js 180 kB +31 B (+0.02%)
ℹ️ View Unchanged
Filename Size
dist/all-external-dependencies.js 215 kB
dist/customizations.full.js 13.8 kB
dist/dead-clicks-autocapture.js 14.5 kB
dist/exception-autocapture.js 9.48 kB
dist/external-scripts-loader.js 2.64 kB
dist/recorder-v2.js 115 kB
dist/recorder.js 115 kB
dist/surveys-preview.js 68.8 kB
dist/surveys.js 71.8 kB
dist/tracing-headers.js 1.76 kB
dist/web-vitals.js 10.4 kB

compressed-size-action

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump patch Bump patch version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant