[APM] Service inventory redesign by dgieselaar · Pull Request #76744 · elastic/kibana

dgieselaar · 2020-09-04T08:29:48Z

Summary

Before:

After:

dgieselaar · 2020-09-04T09:30:14Z

x-pack/plugins/apm/server/lib/helpers/get_bucket_size/index.ts

+export function getBucketSize(
+  start: number,
+  end: number,
+  numBuckets: number = 100


This is needed for the spark plots, where we should use less buckets because of how small the plot is.

dgieselaar · 2020-09-04T09:30:45Z

x-pack/plugins/apm/server/lib/helpers/get_bucket_size/index.ts

-      bucketSize: minBucketSize,
-      intervalString: interval,
+      bucketSize: 0,
+      intervalString: 'auto',


I could not find any instances where we were using anything else than 'auto', so I hardcoded the value into the function.

x-pack/plugins/apm/server/lib/service_map/get_service_anomalies.ts

dgieselaar · 2020-09-04T09:32:54Z

x-pack/plugins/apm/server/routes/services.ts

+
+    const services = await getServices({
+      setup,
+      mlAnomaliesEnvironment: environment,


Using mlAnomaliesEnvironment to make it more explicit that this should only be used for fetching anomalies, as it's already part of uiFilters in other cases.

elasticmachine · 2020-09-04T09:39:56Z

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

x-pack/plugins/apm/server/lib/helpers/get_bucket_size/index.ts

x-pack/plugins/apm/public/hooks/useAnomalyDetectionSettings.ts

x-pack/plugins/apm/public/hooks/useLocalStorage.ts

formgeist

First of all - this looks so good 😍

Made a small suggestion to the tooltip on the service name in the list.

Besides this, I found that the sparkline charts for TPM and Error rate both dip at the end. I think I recall we shifting these in our regular charts?

Additionally, I wonder how it's possible to chart the error rate for opbeans-rum when its error rate chart is empty. Looks like it's a replicate of the TPM chart.

formgeist · 2020-09-04T11:01:41Z

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

    sortable: true,
-    render: (serviceName: string) => (
+    render: (_, { serviceName, agentName }) => (
      <EuiToolTip content={formatString(serviceName)} id="service-name-tooltip">


Suggested change

<EuiToolTip content={formatString(serviceName)} id="service-name-tooltip">

<EuiToolTip delay="long" content={formatString(serviceName)} id="service-name-tooltip">

I would like to delay the tooltip on the service name (since we're showing it always for truncation). It's always been a pet peeve of mine and the way we're truncating these values in our tables.

x-pack/plugins/apm/public/components/app/ServiceOverview/ServiceList/HealthBadge.tsx

x-pack/plugins/apm/public/hooks/useAnomalyDetectionSettings.ts

sorenlouv · 2020-09-04T11:57:32Z

x-pack/plugins/apm/public/hooks/useAnomalyDetectionSettings.ts

+  return useFetcher(
+    (callApmApi) =>
+      callApmApi({
+        pathname: `/api/apm/settings/anomaly-detection`,


In a previous PR review I asked Oliver to change this to

Suggested change

pathname: `/api/apm/settings/anomaly-detection`,

pathname: `/api/apm/settings/anomaly-detection/jobs`,

to make it obvious that it returned the jobs and to align it with POST /api/apm/settings/anomaly-detection/jobs.
However, he ran in to some typescript issues (it was not possible to have the same route twice even for different http methods).
Would you mind looking into it?

I was thinking about fixing this when we can use template string types. We could index route definitions as POST /api/apm/settings/anomaly-detection/jobs and GET /api/apm/settings/anomaly-detection/jobs. I expect this to be easier to implement than resolving this specific issue with the options we have today. Thoughts?

Sure, that's fine with me 👍

x-pack/plugins/apm/server/lib/services/get_services/get_services_items_stats.ts

sorenlouv · 2020-09-04T12:12:04Z

x-pack/plugins/apm/server/lib/services/get_services/get_services_items_stats.ts

+  return Object.keys(anomalies.serviceAnomalies).reduce<
+    Array<{ serviceName: string; severity?: Severity }>
+  >((prev, serviceName) => {
+    const stats = anomalies.serviceAnomalies[serviceName];
+
+    const severity = getSeverity(stats.anomalyScore);
+
+    return prev.concat({
+      serviceName,
+      severity,
+    });
+  }, []);


I think I commented on something similar recently: can you rewrite with [].map?

Edit: ~~map/filter should always be preferred over reduce~~ To explain why I find map/filter preferable over reduce: With map, without reading your implementation, I know that an array with the same length will be returned. With filter I know the length might change but the items will be identical.
reduce comes with barely any guarantees, since mostly anything can be returned. This forces the reader to spent more time understanding the implementation because it's not possible to assume much.

map/filter should always be preferred over reduce

Not sure I agree with this. Maybe when transforming and array to another array.

reduce comes with barely any guarantees, since mostly anything can be returned

What does that mean? In this case I know an Array<{ serviceName: string; severity?: Severity }> is being returned.

Not sure I agree with this. Maybe when transforming and array to another array.

But that's exactly it. When transforming an array to another array and keeping the same length, that's what map is for.

To be clear, [].reduce has its place, like when you want to sum the items in an array (number[] => number). But transformations like T[] => T[] and T[] => P[] almost always become simpler with [].filter and/or [].map.

Fwiw, this is an oversight on my part. It should have been a .map. I think I just default to reduce out of habit when using Object.keys().

sorenlouv · 2020-09-04T12:14:11Z

x-pack/plugins/apm/server/lib/transactions/charts/get_anomaly_data/index.ts

+  const mlJobIds = await getMLJobIds(
+    setup.ml.anomalyDetectors,
+    uiFilters.environment
+  );


Better 👍

smith

Agree with most of the comments from Soren and Casper, just here to say this is gorgeous and I love it.

smith · 2020-09-04T18:55:03Z

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

-                  addWarning,
+      <EuiThemeProvider>
+        <MockApmPluginContextWrapper
+          value={


This crap is way easier if you use immer.

I think something like defaults would help here as well. Will check.

…tory

dgieselaar · 2020-09-07T14:08:40Z

@formgeist I was still using errors per minute. Now that Cauê's work for event.outcome is merged, I've replaced it with the transaction error rate. I've also updated the screenshots. I'm not sure if we handle the "dip" differently in other charts, @sqren are you aware of any custom handling there?

x-pack/plugins/apm/server/lib/alerts/register_transaction_duration_anomaly_alert_type.ts

x-pack/plugins/apm/server/lib/services/get_services/get_services_items.ts

x-pack/plugins/apm/server/lib/services/get_services/get_services_items_stats.ts

x-pack/test/apm_api_integration/basic/tests/services/top_services.ts

x-pack/test/apm_api_integration/trial/tests/services/top_services.ts

dgieselaar · 2020-09-12T13:43:25Z

@elasticmachine merge upstream

dgieselaar · 2020-09-13T13:21:58Z

@sqren I've optimistically removed io-ts and added value tests, under the assumption that #77229 lands in some form in the near future.

sorenlouv · 2020-09-14T10:04:11Z

x-pack/plugins/apm/common/anomaly_detection.ts

+          defaultMessage: 'Critical',
+        }
+      );
+      break;


nit: break no longer necessary

sorenlouv

I added a single nit but you can ignore that since you've got a green build and fixing that doesn't justify holding this up any longer.

…tory

sorenlouv · 2020-09-14T12:51:57Z

x-pack/test/apm_api_integration/trial/tests/services/top_services.ts

+          expectSnapshot(severityScores).toMatchInline(`
+            Array [
+              undefined,
+              undefined,
+              undefined,
+              undefined,
+              undefined,
+              "warning",
+              undefined,
+            ]
+          `);


Would it make sense to filter out undefined severity scores up front:

Suggested change

expectSnapshot(severityScores).toMatchInline(`

Array [

undefined,

undefined,

undefined,

undefined,

undefined,

"warning",

undefined,

]

`);

const severityScores = response.body.items.map((item: any) => item.severity).filter(Boolean);

expect(severityScores.length).to.be.greaterThan(0);

expectSnapshot(severityScores).toMatchInline(`

Array [

"warning",

]

`);

I think that would make sense, but it might no longer be necessary when we fix #77083. I've added it as a note there, to be able to merge this in a bit.

kibanamachine · 2020-09-14T14:05:48Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 625ada6

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
apm	1245	+6	1239
observability	90	+1	89
total		+7

async chunks size

id	value	diff	baseline
apm	4.9MB	+36.3KB	4.9MB
observability	296.6KB	-291.0B	296.9KB
total		+36.0KB

miscellaneous assets size

id	value	diff	baseline
apm	49.0KB	-39.1KB	88.1KB

page load bundle size

id	value	diff	baseline
apm	41.7KB	-51.0B	41.7KB
observability	51.7KB	+1.9KB	49.9KB
total		+1.8KB

distributable file count

id	value	diff	baseline
default	45514	-4	45518

History

💚 Build #73978 succeeded 13f5ec5
💔 Build #73956 failed a31722e
💚 Build #73704 succeeded 48ae3dd
💚 Build #73681 succeeded 1f46dda
💚 Build #73665 succeeded 137d0b1

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

* master: (26 commits) updating datatable type (elastic#77320) [ML] Fix custom URLs processing for security app (elastic#76957) [telemetry] add schema guideline + schema_check new check for --path config (elastic#75747) [ML] Transforms: API schemas and integration tests (elastic#75164) [Mappings editor] Add support for wildcard field type (elastic#76574) [Ingest Manager] Fix flyout instruction selection (elastic#77071) [Telemetry Tools] update lodash to 4.17 (elastic#77317) [APM] Service inventory redesign (elastic#76744) Hide management sections based on cluster/index privileges (elastic#67791) [Snapshot Restore] Disable steps when form is invalid (elastic#76540) [Mappings editor] Add support for positive_score_impact to rank_feature (elastic#76824) Update apm.ts (elastic#77310) [OBS] Remove beta badge, change news feed size and add external icon to news feed link (elastic#77164) [Discover] Convert legacy sort to be compatible with multi sort (elastic#76986) [APM] API Snapshot Testing (elastic#77229) [ML] Functional tests - increase wait time for DFA start (elastic#77307) [UiActions][Drilldowns] Fix actions sorting in context menu (elastic#77162) [Drilldowns] Wire up new links to new docs (elastic#77154) Fix APM issue template [Ingest Pipelines] Drop into an empty tree (elastic#76885) ...

* master: (65 commits) [Security Solution][Resolver] Analyzed event styling (elastic#77115) filter invalid SOs from the searc hresults in Task Manager (elastic#76891) [RUM Dashboard] Visitors by region map (elastic#77135) [Security Solution][Endpoint][Admin] Task/endpoint list actions (elastic#76555) [Ingest pipelines] Forms for processors T-U (elastic#76710) updating datatable type (elastic#77320) [ML] Fix custom URLs processing for security app (elastic#76957) [telemetry] add schema guideline + schema_check new check for --path config (elastic#75747) [ML] Transforms: API schemas and integration tests (elastic#75164) [Mappings editor] Add support for wildcard field type (elastic#76574) [Ingest Manager] Fix flyout instruction selection (elastic#77071) [Telemetry Tools] update lodash to 4.17 (elastic#77317) [APM] Service inventory redesign (elastic#76744) Hide management sections based on cluster/index privileges (elastic#67791) [Snapshot Restore] Disable steps when form is invalid (elastic#76540) [Mappings editor] Add support for positive_score_impact to rank_feature (elastic#76824) Update apm.ts (elastic#77310) [OBS] Remove beta badge, change news feed size and add external icon to news feed link (elastic#77164) [Discover] Convert legacy sort to be compatible with multi sort (elastic#76986) [APM] API Snapshot Testing (elastic#77229) ...

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

cauemarcondes · 2020-10-14T11:25:37Z

Tested on all browsers.

Opened some bugs:
#80472
#80473
#80475
#80482

[APM] Service inventory redesign

c7d8b6a

Closes elastic#75252.

dgieselaar added release_note:enhancement Team:APM - DEPRECATED Use Team:obs-ux-infra_services. v7.10.0 labels Sep 4, 2020

dgieselaar added 2 commits September 4, 2020 11:05

Split out HealthBadge/ServiceListMetric into separate files

d721c17

Return empty object in anomaly alert

edae816

dgieselaar commented Sep 4, 2020

View reviewed changes

x-pack/plugins/apm/server/lib/service_map/get_service_anomalies.ts Show resolved Hide resolved

dgieselaar commented Sep 4, 2020

View reviewed changes

Remove unused translations

eed20ef

dgieselaar marked this pull request as ready for review September 4, 2020 09:39

dgieselaar requested a review from a team September 4, 2020 09:39