-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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: session summary config #19887
feat: session summary config #19887
Conversation
Size Change: 0 B Total Size: 2.21 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some general comments but looks really good. Main one is around the DB change
frontend/src/lib/api.mock.ts
Outdated
@@ -73,6 +73,7 @@ export const MOCK_DEFAULT_TEAM: TeamType = { | |||
session_recording_minimum_duration_milliseconds: null, | |||
session_recording_linked_flag: null, | |||
session_recording_network_payload_capture_config: null, | |||
session_recording_summary_config: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a session_recording_config
in https://github.com/PostHog/posthog/pull/19583/files#diff-18b11537782452794a7ca49f8e734fc5c42477e6da0733ee052f2333412cfd6fR76 because we seemed to be adding a ton of different columns to the teams table. I wonder if we should use that here or possibly create a separate table for all these JSON columns? Probably good to align before we merge either of these PRs
</h3> | ||
<p> | ||
These events are treated as more interesting when generating a summary. We recommend you include | ||
events that represent value for your user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
events that represent value for your user | |
events that represent value for your user e.g. `checkout complete` or `signed up` |
<div> | ||
<h3 className="flex items-center gap-2"> | ||
<IconSelectEvents className="text-lg" /> | ||
Excluded events |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My worry with the "Excluded events" approach is that the data will be sent until it is explicitly excluded. It might not be the worst impact given the event name is all that is sent by default (as properties have to be allow listed) but it's not the most intuitive for someone adding a new event to think "I wonder if this should be excluded from the AI summary feature"
Included event properties | ||
</h3> | ||
<p> | ||
We always send the event name and timestamp. The only other data sent are values of the properties |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly fine for v1 but we might get customers looking for per event property selection
* Add `LemonSlider` * Replace Ant `Slider` with `LemonSlider` * Use `LemonSlider` in feature flags * Fix slider sizing * Update UI snapshots for `chromium` (1) * Fix touch and add ring * Reduce transition duration slightly * Restore utilities.scss * Update UI snapshots for `chromium` (1) * Remove leftover comment --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
…validation (#19970) * fix(web-analytics): Add more debugging when test filters do not pass validation * Update test
* fix(web-analytics): validate web analytics filters from url * Hide warning * Import order
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
This PR was closed due to lack of activity. Feel free to reopen if it's still relevant. |
Explicit opt-in for session summaries
And event/property config