feat: sampling that can be shared #1700
Merged
+29
−32
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR introduces a unified sampling mechanism across PostHog JS, making session ID sampling consistent between before_send and session recording functionalities.
- Added new
/src/extensions/sampling.tswith deterministic hashing algorithm for consistent sampling decisions - Modified session recording sampling in
/src/extensions/replay/sessionrecording.tsto usesampleOnPropertyinstead ofMath.random() - Refactored sampling utilities from
before-send.tsinto shared implementation for better code reuse - Added tests in
/src/__tests__/utils/before-send-utils.test.tsto verify consistent sampling behavior across different methods - Ensures events are only captured for sessions that have recordings when both sampling methods are used
5 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
Comment on lines
+24
to
+25
| export function sampleOnProperty(prop: string, percent: number): boolean { | ||
| return simpleHash(prop) % 100 < clampToRange(percent * 100, 0, 100) |
There was a problem hiding this comment.
logic: No input validation for percent parameter. Should verify percent is between 0 and 1 before using clampToRange.
Suggested change
| export function sampleOnProperty(prop: string, percent: number): boolean { | |
| return simpleHash(prop) % 100 < clampToRange(percent * 100, 0, 100) | |
| export function sampleOnProperty(prop: string, percent: number): boolean { | |
| if (percent < 0 || percent > 1) { | |
| throw new Error('percent must be between 0 and 1'); | |
| } | |
| return simpleHash(prop) % 100 < clampToRange(percent * 100, 0, 100) |
There was a problem hiding this comment.
that's what clamp to range is doing, silly robot
Comment on lines
+8
to
+10
| export function updateThreshold(currentValue: number | undefined, percent: number): number { | ||
| return (isUndefined(currentValue) ? 1 : currentValue) * percent | ||
| } |
There was a problem hiding this comment.
style: updateThreshold could return Infinity if percent is very large and multiplied multiple times. Consider adding upper bound check.
pauldambra
force-pushed
the
feat/sampling-that-can-be-shared
branch
from
e9b15bd to
3a17320
Compare
|
Size Change: +1.22 kB (+0.04%) Total Size: 3.28 MB
ℹ️ View Unchanged
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
the suggested implementation of session id sampling for
before_sendis different to that used in session recordingthe one in
before_sendis nicer as its deterministic for each session idby switching to the
before_sendimplementation someone can set a sample rate for session recording, and sample events by session id inbefore_sendand only get events for sessions that have recordings 🦾(suggested by @camerondeleone in slack somewhere)