[APM] Service overview: Dependencies table by dgieselaar · Pull Request #83416 · elastic/kibana

dgieselaar · 2020-11-16T12:18:59Z

Closes #83152.

~~(Draft PR without tests until #83152 (comment) is resolved.~~

Closes elastic#83152.

…iew-dependencies-table

elasticmachine · 2020-12-03T09:06:56Z

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

dgieselaar · 2020-12-03T09:07:38Z

No tests yet - I need an updated ES archive and running into some issues with getting the ML data the work (seemingly something with spaces).

…iew-dependencies-table

x-pack/plugins/apm/public/components/app/service_overview/service_overview.test.tsx

x-pack/plugins/apm/common/utils/join_by_key/index.ts

cauemarcondes · 2020-12-07T09:05:38Z

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

-                      </ServiceMapLink>
-                    </TableLinkFlexItem>
-                  </EuiFlexGroup>
+                  <ServiceOverviewDependenciesTable serviceName={serviceName} />


I think we could drop the Overview in the name, ServiceDependenciesTable is clear enough and can be reused elsewhere.

Suggested change

<ServiceOverviewDependenciesTable serviceName={serviceName} />

<ServiceDependenciesTable serviceName={serviceName} />

cauemarcondes · 2020-12-07T09:22:10Z

x-pack/plugins/apm/public/components/shared/truncate_with_tooltip/index.tsx

+
+const TooltipWrapper = styled.div`
+  width: 100%;
+  .euiToolTipAnchor {


EUI team advise against it, if the CSS name changes this wouldn't work anymore. You could use styled_component and set the anchorClassName to the EuiToolTip. Here is an example: https://github.com/elastic/kibana/blob/master/x-pack/plugins/apm/public/components/app/service_inventory/ServiceList/index.tsx#L47-L56

cauemarcondes · 2020-12-07T09:26:24Z

x-pack/plugins/apm/server/lib/services/get_service_dependencies/index.ts

+
+      if ('service' in destination) {
+        return {
+          name: destination.service!.name,


Shouldn't TS recognize that service is available in this scope since you checked it?

It doesn't, for some reason. (Technically, a key can be in an object while it's value is undefined, you just don't see TS complaining about it often).

cauemarcondes · 2020-12-07T09:28:07Z

x-pack/plugins/apm/server/lib/services/get_service_dependencies/index.ts

+        spanType:
+          'span.type' in destination ? destination[SPAN_TYPE] : undefined,


If SPAN_TYPE doesn't exist in the destination, it'll return undefined, right? so no reason for the ternary here?!

Suggested change

spanType:

'span.type' in destination ? destination[SPAN_TYPE] : undefined,

spanType: destination[SPAN_TYPE]

I need the refinement so TS allows access to the property.

cauemarcondes · 2020-12-07T09:30:32Z

x-pack/test/apm_api_integration/basic/tests/index.ts

    // TODO: we should not have a service overview.
    describe('Service overview', function () {
      loadTestFile(require.resolve('./service_overview/error_groups'));
+      loadTestFile(require.resolve('./service_overview/dependencies'));


I think this test should be placed inside Services not Service overview, since we don't have service_overview API.

What do you mean with "we don't have a service_overview API"?

Service Overview is the Page, it consumes the transactions, services, errors APIs.

The errors APIs are in separate files as well (server/routes/errors.ts). Metrics are service-specific as well, but both their tests and API definitions are in separate files. Moreover, the error APIs on the service overview page and the service error group page have different data needs. I agree some clean up is needed - but I think we should consider being more specific instead of less. Thoughts @sqren?

I agree that the current API definition and where it's placed is a bit of a mess right now, but I think from an API point of view there is no Service overview. What I understood is Service Overview is a page which shows information of different APIs.

Alright let's chat to @sqren about it later.

cauemarcondes · 2020-12-07T09:31:11Z

x-pack/test/apm_api_integration/basic/tests/service_overview/dependencies.ts

@@ -0,0 +1,90 @@
+/*


I think this test should be placed inside /Services not /service_overview, since we don't have service_overview API.

…iew-dependencies-table

sorenlouv · 2020-12-05T00:20:59Z

x-pack/plugins/apm/common/utils/formatters/duration.ts

-
 /**
 * Convert a microsecond value to decimal milliseconds. Normally we use
 * `asDuration`, but this is used in places like tables where we always want


Great improvements in this file! 😉

sorenlouv · 2020-12-07T09:28:39Z

x-pack/plugins/apm/common/utils/join_by_key/index.ts

      prev.push(item);
    } else {
-      Object.assign(item, current);
+      item = mergeFn(item, current);


Probably just Monday me who can't read code but what is this line doing? It's assigning the results to item but item is never used.

bug! thanks for catching this. It should have replaced the item. I'll add a test for it.

sorenlouv · 2020-12-07T09:33:49Z

...ins/apm/public/components/app/service_overview/service_overview_dependencies_table/index.tsx

+        return (
+          <SparkPlotWithValueLabel
+            color="euiColorVis1"
+            series={latency.timeseries ?? undefined}


Wouldn't this have the same effect?

Suggested change

series={latency.timeseries ?? undefined}

series={latency.timeseries}

If it's because latency.timeseries can be null can we change that?

Or alternatively handle this in SparkPlotWithValueLabel

Yeah, I'll make sure to handle this in a more elegant manner.

sorenlouv · 2020-12-07T09:52:43Z

...ins/apm/public/components/app/service_overview/service_overview_dependencies_table/index.tsx

+    error_rate_value: item.error_rate.value,
+    latency_value: item.latency.value,
+    throughput_value: item.throughput.value,
+    impact_value: item.impact,


why snake case?

Habit 😁 One that I'm trying to get rid of. Will change it.

sorenlouv · 2020-12-08T00:36:10Z

...ins/apm/public/components/app/service_overview/service_overview_dependencies_table/index.tsx

+      <EuiFlexItem>
+        <TableFetchWrapper status={status}>
+          <ServiceOverviewTableContainer
+            isEmptyAndLoading={


Instead of using the wrapper <ServiceOverviewTableContainer isEmptyAndLoading={...} /> what about using EuiBasicTable directly:

<EuiBasicTable noItemsMessage={status === FETCH_STATUS.LOADING ? "Loading" : "No dependencies"} />

There's a bunch of styling in ServiceOverviewTableContainer - don't we want that here as well?

Good point. But I'd prefer to keep the custom styling to a minimum. So if we can avoid isEmptyAndLoading and the resulting visibility styling that would be better.

Also: have we talked to EUI about better ways at getting the styles we want, instead of the custom route?

@smith, any thoughts here?

ServiceOverviewTableContainer makes a fixed-size table and the styling pins the pagination to the bottom. elastic/eui#4281 is open to make it so we don't have to override styles if EUI were to implement this natively.

elastic/eui#4282 was opened by me and closed by the team to handle the empty loading state. The default behavior makes it so there's a cell with text in it while it's loading, which looks much worse than what we have now, where it's empty and shows the loading bar, which looks pretty nice.

...but, the ServiceOverviewTable was meant to be used for each of the tables so I don't understand why we need to export the container.

I'm using a EuiInMemoryTable rather than a EuiBasicTable. What I can do is export only the container, would that be better?

What I can do is export only the container, would that be better?

Yes since we're just passing the props along to the basic or in-memory table that should work.

I'll make sure to do that in the service instances PR.

sorenlouv · 2020-12-08T00:47:25Z

x-pack/plugins/apm/server/lib/services/get_service_dependencies/index.ts

+    getDestinationMap({
+      apmEventClient,
+      serviceName,
+      start,
+      end,
+      environment,
+    }),
+  ]);


What about just forwarding setup thereby making the other arguments more noticeable:

Suggested change

getDestinationMap({

apmEventClient,

serviceName,

start,

end,

environment,

}),

]);

getDestinationMap({ setup, serviceName, environment }),

]);

I wanted to be explicit but I agree that your suggestion emphasises that which is more relevant 👍

sorenlouv · 2020-12-08T01:01:53Z

x-pack/plugins/apm/server/lib/services/get_service_dependencies/index.ts

+      if ('service' in destination) {
+        return {
+          name: destination.service!.name,
+          serviceName: destination.service!.name,


Would be better if we could avoid forcing the compiler (using !.)
I'm actually surprised this is necessary - doesn't typescript narrow the type inside the if block?

I was surprised as well, it might be because of the way joinByKey returns its type. Will have a look, and I can possibly add && destination.service here (though I think I would prefer an error here).

sorenlouv · 2020-12-08T01:10:23Z

x-pack/plugins/apm/server/lib/services/get_service_dependencies/index.ts

+        spanType:
+          'span.type' in destination ? destination[SPAN_TYPE] : undefined,


Nit: this feels a bit heavyhanded. Does tsc complain if you do this?

Suggested change

spanType:

'span.type' in destination ? destination[SPAN_TYPE] : undefined,

spanType: destination[SPAN_TYPE]

Yes. But let me see if I can change the data structure. I wanted something flat but it requires a lot of refinements.

sorenlouv · 2020-12-08T01:16:39Z

x-pack/plugins/apm/server/lib/services/get_service_dependencies/index.ts

+
+  const latencySums = metricsByResolvedAddress
+    .map((metrics) => metrics.latency.value)
+    .filter((n) => n !== null) as number[];


nit: To avoid the explicit type you can use our existing helper that has a type guard:

Suggested change

.filter((n) => n !== null) as number[];

.filter(isValidCoordinateValue)

You should probably rename it though

Suggested change

.filter((n) => n !== null) as number[];

.filter(isNumber)

Does that work with filter though?

Yes, type guards work in filter and will return a nice clean array without nullables :)

sorenlouv · 2020-12-08T01:19:26Z

x-pack/test/apm_api_integration/basic/tests/service_overview/dependencies.ts

+        expectSnapshot(names.sort()).toMatchInline();
+
+        expectSnapshot(serviceNames.sort()).toMatchInline();
+
+        expectSnapshot(latencyValues).toMatchInline();
+
+        expectSnapshot(throughputValues).toMatchInline();


nit: Can you split these into separate its? That makes it easier when debugging failing test cases.

…iew-dependencies-table

sorenlouv · 2020-12-08T16:09:36Z

x-pack/plugins/apm/server/lib/services/get_service_dependencies/index.ts

+
+  const latencySums = metricsByResolvedAddress
+    .map((metrics) => metrics.latency.value)
+    .filter(isFiniteNumber);


Nice, forgot we already had this!

sorenlouv · 2020-12-09T00:09:36Z

x-pack/plugins/apm/public/components/shared/span_icon/get_span_icon.ts

+
+export function getSpanIcon(type?: string, subtype?: string) {
+  if (!type) {
+    return;


We don't want to return a default icon in this case?
Btw. I though span.type was always specified. Is that not the case?

I'm aggregating on span.destination.service.resource only, so I can't guarantee that (due to needing to match with span events). But I will add a top_metrics aggregation on span.type and span.subtype to make sure span.type is always there as well.

span.type is not available on span metrics, so I can't guarantee that it will actually be there (it should be in most cases, if we find a span event with the same span.destination.service.resource).

smith

Should we have these circles enabled on the sparklines? I'm seeing them all the tables, not just dependencies.

How does the deps table work with pagination? I see that we're using EuiInMemoryTable instead of EuiBasicTable and I don't see any pagination controls.

Can we also make the Impact column the same width as it is on the Transactions table, because it's cut off for me:

…iew-dependencies-table

smith · 2020-12-09T23:40:25Z

Thanks for the fixes. It's looking great.

One other thing I noticed that's not a blocker is we probably should exclude the service if it has a link to itself. In this screenshot clicking the link does nothing because I'm already here!

Or maybe we leave it but don't make it a link so people know it's a self-reference.

dgieselaar · 2020-12-10T07:42:47Z

@elasticmachine merge upstream

dgieselaar · 2020-12-10T07:45:12Z

@smith That shouldn't happen in normal situations (this is because of the way the observability test environment routes traffic). I'm wary of introducing additional code here to deal with such an edge case, if the consequences are rather small.

kibanamachine · 2020-12-10T09:28:57Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 981635a

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`apm`	1720	1724	+4

Async chunks

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

id	before	after	diff
`apm`	5.4MB	5.4MB	+7.1KB

Distributable file count

id	before	after	diff
`default`	46991	47754	+763

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`observability`	74.9KB	75.1KB	+107.0B

History

💔 Build #93265 failed e24e3f3
💔 Build #93218 failed 70b6b72
💔 Build #93028 failed a6687a0
💔 Build #92694 failed 8464326
💔 Build #92270 failed 3de214b

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

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* master: (53 commits) Fixing recovered instance reference bug (elastic#85412) Switch to new elasticsearch client for Visualizations (elastic#85245) Switch to new elasticsearch client for TSVB (elastic#85275) Switch to new elasticsearch client for Vega (elastic#85280) [ILM] Add shrink field to hot phase (elastic#84087) Add rolling-file appender to core logging (elastic#84735) [APM] Service overview: Dependencies table (elastic#83416) [Uptime ]Update empty message for certs list (elastic#78575) [Graph] Fix graph saved object references (elastic#85295) [APM] Create new API's to return Latency and Throughput charts (elastic#85242) [Advanced settings] Reset to default for empty strings (elastic#85137) [SECURITY SOLUTION] Bundles _source -> Fields + able to sort on multiple fields in Timeline (elastic#83761) [Fleet] Update agent listing for better status reporting (elastic#84798) [APM] enable 'sanitize_field_names' for Go (elastic#85373) Update dependency @elastic/charts to v24.4.0 (elastic#85452) Introduce external url service (elastic#81234) Deprecate disabling the security plugin (elastic#85159) [FLEET] New Integration Policy Details page for use in Integrations section (elastic#85355) [Security Solutions][Detection Engine] Fixes one liner access control with find_rules REST API chore: 🤖 remove extraPublicDirs (elastic#85454) ...

[APM] Service overview dependencies table

44a2203

Closes elastic#83152.

dgieselaar added release_note:enhancement Team:APM - DEPRECATED Use Team:obs-ux-infra_services. v7.11.0 labels Nov 16, 2020

dgieselaar added 4 commits November 19, 2020 16:36

Merge branch 'master' of github.com:elastic/kibana into service-overv…

1ec8bd7

…iew-dependencies-table

Merge branch 'master' of github.com:elastic/kibana into service-overv…

788f1fc

…iew-dependencies-table

Merge branch 'master' of github.com:elastic/kibana into service-overv…

54ddee2

…iew-dependencies-table

Merge branch 'master' of github.com:elastic/kibana into service-overv…

a4917e8

…iew-dependencies-table

sorenlouv changed the title ~~[APM] Service overview dependencies table~~ [APM] Service overview: Dependencies table Nov 24, 2020

dgieselaar added 6 commits November 30, 2020 13:49

Merge branch 'master' of github.com:elastic/kibana into service-overv…

9ca351d

…iew-dependencies-table

Merge branch 'master' of github.com:elastic/kibana into service-overv…

74d5e19

…iew-dependencies-table

Merge branch 'master' of github.com:elastic/kibana into service-overv…

4e0f1eb

…iew-dependencies-table

Use top_hits instead of collapse

e35bb80

Merge branch 'master' of github.com:elastic/kibana into service-overv…

6ccb4cf

…iew-dependencies-table

Merge branch 'master' of github.com:elastic/kibana into service-overv…

d304420

…iew-dependencies-table

dgieselaar marked this pull request as ready for review December 3, 2020 09:06

dgieselaar requested a review from a team December 3, 2020 09:06

dgieselaar added 2 commits December 4, 2020 09:06

Merge branch 'master' of github.com:elastic/kibana into service-overv…

12d9c7d

…iew-dependencies-table

Merge branch 'master' of github.com:elastic/kibana into service-overv…

3de214b

…iew-dependencies-table

cauemarcondes reviewed Dec 7, 2020

View reviewed changes

x-pack/plugins/apm/public/components/app/service_overview/service_overview.test.tsx Show resolved Hide resolved

cauemarcondes reviewed Dec 7, 2020

View reviewed changes

x-pack/plugins/apm/common/utils/join_by_key/index.ts Show resolved Hide resolved

cauemarcondes reviewed Dec 7, 2020

View reviewed changes

Merge branch 'master' of github.com:elastic/kibana into service-overv…

c412c91

…iew-dependencies-table

sorenlouv approved these changes Dec 8, 2020

View reviewed changes

dgieselaar added 3 commits December 8, 2020 09:27

Merge branch 'master' of github.com:elastic/kibana into service-overv…

c5eed84

…iew-dependencies-table

Make sure result of mergeFn overrides existing item

dab72cd

Review feedback

8464326

sorenlouv reviewed Dec 8, 2020

View reviewed changes

sorenlouv reviewed Dec 9, 2020

View reviewed changes

smith requested changes Dec 9, 2020

View reviewed changes

dgieselaar added 3 commits December 9, 2020 09:32

Merge branch 'master' of github.com:elastic/kibana into service-overv…

981c3ff

…iew-dependencies-table

Review feedback

a6687a0

API tests

70b6b72

dgieselaar requested a review from smith December 9, 2020 20:32

Merge branch 'master' of github.com:elastic/kibana into service-overv…

e24e3f3

…iew-dependencies-table

smith approved these changes Dec 9, 2020

View reviewed changes

Merge branch 'master' into service-overview-dependencies-table

981635a

dgieselaar merged commit 04428ff into elastic:master Dec 10, 2020

dgieselaar deleted the service-overview-dependencies-table branch December 10, 2020 09:32

dgieselaar mentioned this pull request Dec 10, 2020

[7.x] [APM] Service overview: Dependencies table (#83416) #85522

Merged

dgieselaar added a commit to dgieselaar/kibana that referenced this pull request Dec 10, 2020

[APM] Service overview: Dependencies table (elastic#83416)

079204f

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

sorenlouv mentioned this pull request Dec 18, 2020

[APM] 7.11 Test plan #86511

Closed

	<ServiceOverviewDependenciesTable serviceName={serviceName} />
	<ServiceDependenciesTable serviceName={serviceName} />

		spanType:
		'span.type' in destination ? destination[SPAN_TYPE] : undefined,

	spanType:
	'span.type' in destination ? destination[SPAN_TYPE] : undefined,
	spanType: destination[SPAN_TYPE]

	series={latency.timeseries ?? undefined}
	series={latency.timeseries}

	.filter((n) => n !== null) as number[];
	.filter(isValidCoordinateValue)

Conversation

dgieselaar commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Dec 3, 2020

Uh oh!

dgieselaar commented Dec 3, 2020

Uh oh!

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

sorenlouv Dec 8, 2020 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

dgieselaar commented Nov 16, 2020 •

edited

Loading

sorenlouv Dec 8, 2020 •

edited

Loading

sorenlouv Dec 8, 2020 •

edited

Loading