-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
added performance telemetry to profiling pages #208832
Conversation
I'm going to publish this pr to see if I can get the cloud instance to build. |
…apping'
/oblt-deploy |
x-pack/solutions/observability/plugins/profiling/public/views/functions/topn/index.tsx
Outdated
Show resolved
Hide resolved
...solutions/observability/plugins/profiling/public/views/functions/differential_topn/index.tsx
Outdated
Show resolved
Hide resolved
.../observability/plugins/profiling/public/views/flamegraphs/differential_flamegraphs/index.tsx
Outdated
Show resolved
Hide resolved
…functions/topn/index.tsx Co-authored-by: Carlos Crespo <[email protected]>
…functions/differential_topn/index.tsx Co-authored-by: Carlos Crespo <[email protected]>
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.
All Universal Profiling pages are crashing. I'm seeing this error:
![image](https://private-user-images.githubusercontent.com/2767137/408516391-d543d73d-dd7e-42ce-8d09-1e38018cf67b.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk4ODg3MDEsIm5iZiI6MTczOTg4ODQwMSwicGF0aCI6Ii8yNzY3MTM3LzQwODUxNjM5MS1kNTQzZDczZC1kZDdlLTQyY2UtOGQwOS0xZTM4MDE4Y2Y2N2IucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIxOCUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMThUMTQyMDAxWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9MDlkMTM0MDBhODVmMDViZDExNjNlZjVjNTk1YjA0M2IzMWMzNjdkNjM5NmFmNWM0ODI0NGQ2NTlkOGNhN2FkNSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.6X6iTrpjvETsvR-bf1LitN88OUKkWvYPp2DdDEUqFnM)
We need to wrap the routes with the on here https://github.com/elastic/kibana/blob/main/x-pack/solutions/observability/plugins/profiling/public/app.tsx
/oblt-deploy |
…apping'
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.
LGTM. Shouldn't we track the storage explorer page too?
onPageReady({ | ||
meta: { | ||
rangeFrom, | ||
rangeTo, | ||
}, | ||
}); |
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 think the page will be ready when storageExplorerSummaryState
is complete and the default open tab content also finishes loading. In this case, host_breakdown
. Wdyt?
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.
Good catch, I agree. I'll add a status check for the host breakdown.
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've moved the onPageReady call into the host_breakdown component. I'm not super jazzed about it, but pulling the host_breakdown fetch into the storage_explorer looked like a mess due to the tab state changes. One issue with this solution is the onPageReady will be called every time the tab is toggled, I tried wrapping the call in a useMemo, but it looks like the api-call is also refreshed, so it seems like more of an issue with the component itself. Is this due to the fact the time window is technically changed from the last fetch? Is this ok to leave alone?
Do you think this solution is alright, or do you have an alternative suggestion?
If this looks ok, then it might be worthwhile to add telemetry to the data breakdown tab as well.
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.
Hey Bryce, thanks for the changes. I think it's fine to call onPageReady
more than once. It will only register one event per page view: https://github.com/crespocarlos/kibana/blob/main/src/platform/packages/shared/kbn-ebt-tools/src/performance_metrics/context/use_page_ready.ts
You could also test this by following this comment and checking on Discover whether the event is registered on every tab switch.
Should new events be registered when switching tabs, we'll need to see what we can do.
x-pack/solutions/observability/plugins/profiling/public/views/storage_explorer/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/solutions/observability/plugins/profiling/public/views/flamegraphs/flamegraph/index.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.
Thanks for all the changes so far @bryce-b . Left a few comments, but It's almost there.
...solutions/observability/plugins/profiling/public/views/functions/differential_topn/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/solutions/observability/plugins/profiling/public/views/storage_explorer/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/solutions/observability/plugins/profiling/public/views/stack_traces_view/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/solutions/observability/plugins/profiling/public/views/functions/topn/index.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.
LGTM.
Did you manage to create a dashboard on the staging env https://telemetry-v2-staging.elastic.dev/?
@crespocarlos I've added Profiler specific charts to the performance dashboard |
⏳ Build in-progress
History
|
Summary
This adds performance telemetry to the profiling pages listed in #205393
Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.
release_note:*
label is applied per the guidelines