-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(insights): Update Web Vital Meter in the landing page with link to performance issues #93520
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
Conversation
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.
Makes sense, just a few nits and questions here and there! The suggestion to use GroupType
is just for your consideration, not a blocker 👍🏻
export const WEB_VITAL_PERFORMANCE_ISSUES: Record< | ||
WebVitals, | ||
Array<keyof typeof ISSUE_TYPE_TO_ISSUE_TITLE> | ||
> = { | ||
lcp: [ | ||
'performance_render_blocking_asset_span', | ||
'performance_uncompressed_assets', | ||
'performance_http_overhead', | ||
'performance_consecutive_http', | ||
'performance_n_plus_one_api_calls', | ||
'performance_large_http_payload', | ||
'performance_p95_endpoint_regression', | ||
], | ||
fcp: [ | ||
'performance_render_blocking_asset_span', | ||
'performance_uncompressed_assets', | ||
'performance_http_overhead', | ||
'performance_consecutive_http', | ||
'performance_n_plus_one_api_calls', | ||
'performance_large_http_payload', | ||
'performance_p95_endpoint_regression', | ||
], | ||
inp: [ | ||
'performance_http_overhead', | ||
'performance_consecutive_http', | ||
'performance_n_plus_one_api_calls', | ||
'performance_large_http_payload', | ||
'performance_p95_endpoint_regression', | ||
], | ||
cls: [], | ||
ttfb: ['performance_http_overhead'], | ||
} as const; |
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.
This makes sense, but I bet people won't remember to update this. I think a more flexible solution here would be to:
- Add a field to the
GroupType
class in Python that specifies whether a given group type affects page load performance or not - Add a filter to issue search that allows to search for issues that affect page load
I think that approach is better because when we add more issue detectors (apparently this is planned) and when users start creating their own issue detectors based on metrics we can make sure that those issues also show up in these dropdowns somehow
That's a lot more work, obviously, but worth considering, IMO!
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, interesting solution. I can look into this! Would you mind if I explored this separately and handle this in separate prs? Should be simple to pivot the frontend once I've dug into this a bit more
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 didn't mean for that comment to be a blocker, just thinking ahead since the Issue Platform folks are working in this area right now
static/app/views/insights/browser/webVitals/queries/useWebVitalsIssuesQuery.tsx
Outdated
Show resolved
Hide resolved
static/app/views/insights/browser/webVitals/components/webVitalMetersWithIssues.spec.tsx
Outdated
Show resolved
Hide resolved
static/app/views/insights/browser/webVitals/components/webVitalMetersWithIssues.spec.tsx
Outdated
Show resolved
Hide resolved
static/app/views/insights/browser/webVitals/components/webVitalMetersWithIssues.tsx
Outdated
Show resolved
Hide resolved
static/app/views/insights/browser/webVitals/components/webVitalMetersWithIssues.tsx
Outdated
Show resolved
Hide resolved
static/app/views/insights/browser/webVitals/components/webVitalMetersWithIssues.tsx
Outdated
Show resolved
Hide resolved
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.
Makes sense! The only thing left is the hover behaviour, which I think is worth clarifying, if possible. Worth asking the FE folks in Slack, even, if stopping the event propagation doesn't work. Otherwise LGTM 👍🏻
cb55aa8
to
ce080e0
Compare
…eb-vitals-perf-issues
Update to design with link to related perf issues.
