[APM] Add waterfall to dependency operations #143257
Conversation
…kibana into dependencies-trace-waterfall
|
Pinging @elastic/apm-ui (Team:APM) |
...c/components/app/dependency_operation_detail_view/dependency_operation_detail_trace_list.tsx
Show resolved
Hide resolved
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
| const queryRef = useRef(query); | ||
|
|
||
| queryRef.current = query; |
There was a problem hiding this comment.
Isn't the ref initialised on every render with query?
There was a problem hiding this comment.
It is to get around the dependencies lint rule for the useEffect call. I'd rather do this (which is not great either) than disable the lint rule for the entire call. I don't want to redirect when the page or pageSize changes.
| history, | ||
| page: queryRef.current.page ?? 0, | ||
| pageSize: queryRef.current.pageSize ?? 10, | ||
| replace, |
There was a problem hiding this comment.
Do we need to pass replace as an argument? It is a singleton that you can import from ../../shared/links/url_helpers.
There was a problem hiding this comment.
ah.. does it make testing easier?
| spanId: '11', | ||
| }); | ||
| }); | ||
| }); |
| const samplePageIndex = isControlled | ||
| ? selectedSample | ||
| ? traceSamples?.indexOf(selectedSample) | ||
| : 0 | ||
| : sampleActivePage; |
There was a problem hiding this comment.
It looks like isControlled is not needed and makes the code slightly harder to read
| const samplePageIndex = isControlled | |
| ? selectedSample | |
| ? traceSamples?.indexOf(selectedSample) | |
| : 0 | |
| : sampleActivePage; | |
| const samplePageIndex = selectedSample !== undefined | |
| ? traceSamples?.indexOf(selectedSample) | |
| : sampleActivePage; |
There was a problem hiding this comment.
The purpose of isControlled is to make it explicit that there are two modes: controlled and uncontrolled. Additionally, if I don't use an intermediate variable, I end up having to pass selectedSample == undefined as a dependency to the useEffect call which is weird to me.
| if (!isControlled) { | ||
| setSampleActivePage(index); | ||
| } |
There was a problem hiding this comment.
I think this would be easier to read than having an intermediary variable:
| if (!isControlled) { | |
| setSampleActivePage(index); | |
| } | |
| if (selectedSample !== undefined) { | |
| setSampleActivePage(index); | |
| } |
|
|
||
| import React, { useRef } from 'react'; | ||
|
|
||
| export function ResettingHeightRetainer( |
There was a problem hiding this comment.
Is this meant to disable the existing behaviour of HeightRetainer? Is it possible to extend HeightRetainer and disable/reset it that way?
There was a problem hiding this comment.
HeightRetainer never resets (ie, it keeps the max height of all dimensions it has seen), but because the implementation is quite different I opted for a new component. Hopefully we can eventually replace HeightRetainer with this one, but I was worried I'd break something in other components, so I left it out of scope for this PR.
* main: (24 commits) [Files] Add file upload to file picker (elastic#143969) [Security solution] Guided onboarding, alerts & cases (elastic#143598) [APM] Critical path for a single trace (elastic#143735) skip failing test suite (elastic#143933) [Fleet] Update GH Projects automation (elastic#144123) [APM] Performance fix for 'cardinality' telemetry task (elastic#144061) [Enterprise Search] Attach ML Inference Pipeline - Pipeline re-use (elastic#143979) [8.5][DOCS] Add support for differential logs (elastic#143242) Bump nwsapi from v2.2.0 to v2.2.2 (elastic#144001) [APM] Add waterfall to dependency operations (elastic#143257) [Shared UX] Add deprecation message to kibana react Markdown (elastic#143766) [Security Solution][Endpoint] Adds RBAC API checks for Blocklist (elastic#144047) Improve `needs-team` auto labeling regex (elastic#143787) [Reporting/CSV Export] _id field can not be formatted (elastic#143807) Adds SavedObjectsWarning to analytics results pages. (elastic#144109) Bump chromedriver to 107 (elastic#144073) Update cypress (elastic#143755) [Maps] nest security layers in layer group (elastic#144055) [Lens][Agg based Heatmap] Navigate to Lens Agg based Heatmap. (elastic#143820) Added support of saved search (elastic#144095) ...
Closes #142368.
Adds a trace waterfall in the dependency operation detail view.