-
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(web-analytics): Add Web Vitals to the toolbar #28173
Conversation
b6f5d8e
to
0e5f934
Compare
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.
PR Summary
This PR adds Web Vitals monitoring functionality to the PostHog toolbar, enabling users to view performance metrics for both current page loads and historical data from the last 7 days.
Key points to address:
- API endpoint
/api/web_vitals.py
usesCALCULATE_BLOCKING_ALWAYS
which could impact performance - consider using non-blocking calculation mode - The web_vitals scope in
scopes.py
needs proper RBAC implementation with access control restrictions and documentation - Missing validation for required pathname parameter in web vitals API endpoint
- The ModelViewSet in
web_vitals.py
exposes unnecessary HTTP methods since only list() is implemented - Dynamic color class generation in
WebVitalsToolbarMenu.tsx
using template literals could pose security risks if not properly sanitized
34 file(s) reviewed, 19 comment(s)
Edit PR Review Bot Settings | Greptile
@@ -99,6 +99,7 @@ export const APIScopes: APIScope[] = [ | |||
}, | |||
}, | |||
{ key: 'webhook', info: 'Webhook configuration is currently only enabled for the Zapier integration.' }, | |||
{ key: 'web_vitals' }, |
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.
logic: The web_vitals scope is added without any access control restrictions or documentation. Consider adding disabledActions, description, and potential warnings about data access.
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.
@zlwaterfield that's where I need you. Do I need to do anything here?
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 haven't worked much with the api scopes, what is your goal? Does the toolbar load data through personal API keys?
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 thought this was RBAC related, so that's why I tagged you, my apologies hahah
It uses the same temporary_token
other toolbar stuff is using, so I don't think I'm using personal API Keys
posthog/api/web_vitals.py
Outdated
from posthog.hogql_queries.query_runner import get_query_runner, ExecutionMode | ||
|
||
|
||
class WebVitalsViewSet(TeamAndOrgViewSetMixin, viewsets.ModelViewSet): |
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.
style: ModelViewSet exposes all CRUD methods but only list() is implemented. Consider using ReadOnlyModelViewSet instead
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.
Question to those with more REST Django knowledge than I have (nil), do I actually need to inherit anything here?
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.
Because you're not actually representing a Django model with this viewset, you should not be inheriting from ModelViewSet
– just GenericViewSet
35b706b
to
662912e
Compare
8bdcbb2
to
aa73914
Compare
Size Change: +5.85 kB (+0.5%) Total Size: 1.17 MB
|
📸 UI snapshots have been updated10 snapshot changes in total. 0 added, 10 modified, 0 deleted:
Triggered by this commit. |
WEB_REVENUE_TRACKING: 'web-revenue-tracking', // owner: @robbie-c #team-web-analytics | ||
LLM_OBSERVABILITY: 'llm-observability', // owner: #team-ai-product-manager | ||
ONBOARDING_SESSION_REPLAY_SEPERATE_STEP: 'onboarding-session-replay-separate-step', // owner: @joshsny #team-growth | ||
ONBOARDING_SESSION_REPLAY_SEPARATE_STEP: 'onboarding-session-replay-separate-step', // owner: @joshsny #team-growth |
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.
@joshsny Updated this because greptile was complaining about it, hope you don't mind
We can customize the spinner speed now, just pass a "${number}s" string, it's 1s by default
We'll be adding a new Web Vitals section to the toolbar, but let's keep it hidden behind a FF
We had some leftovers functions inside `webAnalyticsLogic`, let's keep them inside `nodes/WebVitals/definitions.ts` instead, it makes more sense and simplifies the whole import/export dance
This endpoint can be used to collect information from the most recent 7 days of data for your Web Vitals, this is used in the toolbar
I'm using this locally, it helps with my workflow, let's stop having this in my git stash all the time
We can now look at web vitals from our toolbar. It'll display information from the current page run but also from the last 7 days in that specific page.
After rebasing on master we had to fix this
This will return more reasonable values to the UI
The previous value was better for local testing, but we should use the cached value in production
We never write, so let's remove some endpoints we don't need
This caused our storybook tests to never finish running, see https://github.com/PostHog/posthog/actions/runs/13085431533/job/36515998085?pr=28173
Make some of the metrics yellow/red
Align with the existing `--spinner-color` var
We're not inheriting from a Django model or anything, so let's use the generic viewset instead
8d3b3b3
to
42d0a43
Compare
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
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 don't see blockers anymore, but gonna let @robbie-c give the final ✅, since you two are much closer working on web analytics 😄
threshold: WebVitalsThreshold | ||
): WebVitalsMetricBand | 'none' => { | ||
export const getMetricBand = (value: number | undefined, metric: WebVitalsMetric): WebVitalsMetricBand | 'none' => { | ||
const threshold = WEB_VITALS_THRESHOLDS[metric] |
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.
👍 to moving this lookup here (and below in getThresholdColor
), I think it makes everything that calls it feel cleaner
--toolbar-width-expanded: 334px; | ||
} | ||
|
||
&--with-experiments-and-web-vitals { |
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.
hmm, this approach is probably fine for now but seems unscalable 😅
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.
Yeah, I agree hahah, but I expect Web Vitals and Experiments to go out of beta eventually, and then we can revert this :)
|
||
# This is a simple wrapper around a basic query, so that's why `scope_object = "query"` | ||
# This `Viewset` does need to exist, however, because we need to support the `TemporaryTokenAuthentication` method | ||
class WebVitalsViewSet(TeamAndOrgViewSetMixin, viewsets.GenericViewSet): |
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 don't feel super able to review this part, I've not touched this part of django, but if @Twixes is happy then so am I
@@ -1,9 +1,3 @@ | |||
## API Scopes |
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.
Why get rid of the comment?
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.
It's duplicated below
this is cool... now i want to be able to get this data for prod while i have the toolbar on my local dev env! |
If someone is storing local/prod data in the same PH project then this is going to work :). Not sure it'll interact well with environments tho |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
We can now look at web vitals from our toolbar. It'll display information from the current page run but also from the last 7 days in that specific page.
This is hidden behind a separate FF so this should be pretty safe to merge