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

feat: allow config of before_send function to edit or reject events #1515

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

Conversation

pauldambra
Copy link
Member

@pauldambra pauldambra commented Nov 8, 2024

see RFC: https://github.com/PostHog/product-internal/pull/668/
see docs PR: PostHog/posthog.com#9844

The number of config options to redact urls or edit properties is growing
It is simpler to have a single hook where you can edit an event before it is captured

This also allows the enterprising customer configure a sampling function


tested locally and event capture still works without a beforeSend set
and e.g. the pre-canned event sampling function works as expected

Copy link

vercel bot commented Nov 8, 2024

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

Name Status Preview Updated (UTC)
posthog-js ✅ Ready (Inspect) Visit Preview Nov 15, 2024 0:45am

Copy link

github-actions bot commented Nov 8, 2024

Size Change: +10.4 kB (+0.33%)

Total Size: 3.12 MB

Filename Size Change
dist/all-external-dependencies.js 204 kB -6 B (0%)
dist/array.full.es5.js 255 kB +1.06 kB (+0.42%)
dist/array.full.js 357 kB +940 B (+0.26%)
dist/array.full.no-external.js 356 kB +940 B (+0.27%)
dist/array.js 172 kB +943 B (+0.55%)
dist/array.no-external.js 171 kB +941 B (+0.55%)
dist/dead-clicks-autocapture.js 13.1 kB +74 B (+0.57%)
dist/external-scripts-loader.js 2.27 kB +74 B (+3.37%)
dist/main.js 173 kB +1.01 kB (+0.59%)
dist/module.full.js 357 kB +1 kB (+0.28%)
dist/module.full.no-external.js 356 kB +1 kB (+0.28%)
dist/module.js 172 kB +1 kB (+0.59%)
dist/module.no-external.js 171 kB +1 kB (+0.59%)
dist/recorder-v2.js 113 kB +71 B (+0.06%)
dist/recorder.js 114 kB +71 B (+0.06%)
dist/surveys-preview.js 56.7 kB +71 B (+0.13%)
dist/surveys.js 62.2 kB +83 B (+0.13%)
dist/tracing-headers.js 1.4 kB +74 B (+5.57%) 🔍
ℹ️ View Unchanged
Filename Size
dist/exception-autocapture.js 8.8 kB
dist/web-vitals.js 10.3 kB

compressed-size-action

@pauldambra pauldambra marked this pull request as ready for review November 10, 2024 12:59
src/types.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/posthog-core.ts Outdated Show resolved Hide resolved
@pauldambra pauldambra added the bump minor Bump minor version when this PR gets merged label Nov 13, 2024
@pauldambra
Copy link
Member Author

have updated and tested the examples in the posthog.com docs PR

and tested this locally

i think this is good to go

src/types.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
Copy link
Member

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

left a few comments but I don't have strong opinion about them so LGTM

Copy link
Contributor

@robbie-c robbie-c left a comment

Choose a reason for hiding this comment

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

Non-blocking and has been covered by others but:

  • I would probably allow the footgun and let people do any event, and make a customization to only touch safe events. That way the list of safe events can be outside of the default bundle and keep the size down
  • I agree with @marandaneto that sampling should be 0-1 rather than 0-100, just because I'd expect this from using other dev tools e.g. Sentry.
  • It'd be cool if we provided a way to chain these functions easily :)

src/posthog-core.ts Outdated Show resolved Hide resolved
src/posthog-core.ts Outdated Show resolved Hide resolved
@pauldambra
Copy link
Member Author

@robbie-c how about allowing an array of functions...

then we can do

before_send: [
	sampleByDistinctId(0.5), // only half of people
	sampleByEvent(['$web_vitals], 0.1), // and they capture all events except 10% of web vitals
]

@robbie-c
Copy link
Contributor

Makes sense, would you multiply the sample thresholds together?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump minor Bump minor version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants