[APM] Add waterfall to dependency operations by dgieselaar · Pull Request #143257 · elastic/kibana

dgieselaar · 2022-10-13T08:15:43Z

Closes #142368.

Adds a trace waterfall in the dependency operation detail view.

Closes elastic#142368.

…-ref HEAD~1..HEAD --fix'

…ace-waterfall

…kibana into dependencies-trace-waterfall

x-pack/test/apm_api_integration/tests/dependencies/top_spans.spec.ts

elasticmachine · 2022-10-13T12:59:05Z

Pinging @elastic/apm-ui (Team:APM)

…ace-waterfall

...c/components/app/dependency_operation_detail_view/dependency_operation_detail_trace_list.tsx

…ace-waterfall

kibana-ci · 2022-10-27T08:01:58Z

💚 Build Succeeded

Buildkite Build
Commit: f340ba7

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`apm`	1334	1336	+2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`apm`	3.1MB	3.1MB	+2.6KB

History

💚 Build #82887 succeeded 5b25407
💚 Build #82060 succeeded bc21036
💚 Build #80381 succeeded 6d1e5f7
💔 Build #80101 failed 961927a
💔 Build #80023 failed e2ad688

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

sorenlouv · 2022-10-27T14:39:43Z

x-pack/plugins/apm/public/components/app/dependency_operation_detail_view/index.tsx

+  const queryRef = useRef(query);
+
+  queryRef.current = query;


Isn't the ref initialised on every render with query?

... and why do we need it ?

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.

sorenlouv · 2022-10-27T14:41:56Z

x-pack/plugins/apm/public/components/app/dependency_operation_detail_view/index.tsx

+      history,
+      page: queryRef.current.page ?? 0,
+      pageSize: queryRef.current.pageSize ?? 10,
+      replace,


Do we need to pass replace as an argument? It is a singleton that you can import from ../../shared/links/url_helpers.

ah.. does it make testing easier?

sorenlouv · 2022-10-27T14:43:37Z

...ponents/app/dependency_operation_detail_view/maybe_redirect_to_available_span_sample.test.ts

+      spanId: '11',
+    });
+  });
+});


Thanks for the test

sorenlouv · 2022-10-27T14:52:07Z

x-pack/plugins/apm/public/components/app/transaction_details/waterfall_with_summary/index.tsx

+  const samplePageIndex = isControlled
+    ? selectedSample
+      ? traceSamples?.indexOf(selectedSample)
+      : 0
+    : sampleActivePage;


It looks like isControlled is not needed and makes the code slightly harder to read

Suggested change

const samplePageIndex = isControlled

? selectedSample

? traceSamples?.indexOf(selectedSample)

: 0

: sampleActivePage;

const samplePageIndex = selectedSample !== undefined

? traceSamples?.indexOf(selectedSample)

: sampleActivePage;

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.

sorenlouv · 2022-10-27T14:52:56Z

x-pack/plugins/apm/public/components/app/transaction_details/waterfall_with_summary/index.tsx

+    if (!isControlled) {
+      setSampleActivePage(index);
+    }


I think this would be easier to read than having an intermediary variable:

Suggested change

if (!isControlled) {

setSampleActivePage(index);

}

if (selectedSample !== undefined) {

setSampleActivePage(index);

}

sorenlouv · 2022-10-27T15:03:40Z

x-pack/plugins/apm/public/components/shared/height_retainer/resetting_height_container.tsx

+
+import React, { useRef } from 'react';
+
+export function ResettingHeightRetainer(


Is this meant to disable the existing behaviour of HeightRetainer? Is it possible to extend HeightRetainer and disable/reset it that way?

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) ...

dgieselaar and others added 4 commits October 13, 2022 10:15

[APM] Add waterfall to dependency operations

566d43d

Closes elastic#142368.

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

e2ad688

…-ref HEAD~1..HEAD --fix'

Add unit tests

2596388

Merge branch 'main' of github.com:elastic/kibana into dependencies-tr…

6660226

…ace-waterfall

dgieselaar added release_note:enhancement v8.6.0 labels Oct 13, 2022

dgieselaar added 3 commits October 13, 2022 13:54

Remove unused translations

267eab0

Remove API test for spans without transaction

854fc23

Merge branch 'dependencies-trace-waterfall' of github.com:dgieselaar/…

ce60e90

…kibana into dependencies-trace-waterfall

dgieselaar commented Oct 13, 2022

View reviewed changes

x-pack/test/apm_api_integration/tests/dependencies/top_spans.spec.ts Show resolved Hide resolved

Undo changes in app_root.tsx

961927a

dgieselaar marked this pull request as ready for review October 13, 2022 12:53

dgieselaar requested a review from a team October 13, 2022 12:53

botelastic bot added the Team:APM - DEPRECATED Use Team:obs-ux-infra_services. label Oct 13, 2022

dgieselaar added 3 commits October 14, 2022 11:02

Merge branch 'main' of github.com:elastic/kibana into dependencies-tr…

eba0702

…ace-waterfall

Address issue with tests

ce0ac3a

Merge branch 'main' of github.com:elastic/kibana into dependencies-tr…

6d1e5f7

…ace-waterfall

kpatticha reviewed Oct 17, 2022

View reviewed changes

...c/components/app/dependency_operation_detail_view/dependency_operation_detail_trace_list.tsx Show resolved Hide resolved

dgieselaar added 4 commits October 24, 2022 11:51

Merge branch 'main' of github.com:elastic/kibana into dependencies-tr…

bc21036

…ace-waterfall

Merge branch 'main' of github.com:elastic/kibana into dependencies-tr…

5b25407

…ace-waterfall

Make sure service name is displayed

bebc66a

Merge branch 'main' of github.com:elastic/kibana into dependencies-tr…

f340ba7

…ace-waterfall

dgieselaar mentioned this pull request Oct 27, 2022

[APM] Remove table in dependency operations view #144081

Closed

dgieselaar requested review from kpatticha and sorenlouv October 27, 2022 06:59

sorenlouv reviewed Oct 27, 2022

View reviewed changes

sorenlouv approved these changes Oct 27, 2022

View reviewed changes

dgieselaar merged commit f648ef3 into elastic:main Oct 27, 2022

dgieselaar deleted the dependencies-trace-waterfall branch October 27, 2022 17:09

kibanamachine added the backport:skip This PR does not require backporting label Oct 27, 2022


		import React, { useRef } from 'react';

		export function ResettingHeightRetainer(

Conversation

dgieselaar commented Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

elasticmachine commented Oct 13, 2022

Uh oh!

Uh oh!

kibana-ci commented Oct 27, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sorenlouv Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dgieselaar commented Oct 13, 2022 •

edited

Loading

sorenlouv Oct 27, 2022 •

edited

Loading