[APM] Load list of services from ML/terms enum by dgieselaar · Pull Request #126581 · elastic/kibana

dgieselaar · 2022-03-01T12:54:34Z

Creates a new endpoint (GET /internal/apm/sorted_and_filtered_services) that returns a list of services based on anomaly data and the terms enum API. When environment is set, terms enum data is not returned (as it cannot be filtered). When kuery is set, ML data is not returned.

Note that GET /internal/apm/services still returns ML data - in a further iteration this might be improved, but for now this seems a good compromise in terms of scope/risk vs being fully optimised.

Closes elastic#126463.

elasticmachine · 2022-03-01T12:54:37Z

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

…ices

sorenlouv · 2022-03-02T13:55:52Z

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

+    sortedAndFilteredServicesData,
+    sortedAndFilteredServicesStatus,
    mainStatisticsData,
    mainStatisticsStatus,
    comparisonData,


Instead of separating status from data I think we should make it a convention to pass it around as a single object:

Suggested change

sortedAndFilteredServicesData,

sortedAndFilteredServicesStatus,

mainStatisticsData,

mainStatisticsStatus,

comparisonData,

sortedAndFilteredServices,

mainStatistics,

comparison,

I think that make sense. Should we append something like 'fetch', e.g. sortedAndFilteredServicesFetch?

Yes, a Fetch suffix sgtm 👍

sorenlouv · 2022-03-02T20:08:18Z

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

+  const isLoading =
+    sortedAndFilteredServicesStatus === FETCH_STATUS.LOADING ||
+    (sortedAndFilteredServicesStatus === FETCH_STATUS.SUCCESS &&
+      sortedAndFilteredServicesData?.services.length === 0 &&
+      mainStatisticsStatus === FETCH_STATUS.LOADING);


A little complex for a single loading state 😅

I can break it up in variables, I don't think the logic itself would get easier, or do you have any suggestions?

I don't have specific suggestions - just wondering if this is a symptom of having too complex requirements for loading states. Or if this is something that can be handled more elegantly in useFetcher.

…ices

kibana-ci · 2022-03-09T11:10:18Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`apm`	1134	1135	+1

Async chunks

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

id	before	after	diff
`apm`	2.7MB	2.7MB	+616.0B

History

💚 Build #28438 succeeded 618303c
💔 Build #28350 failed 3d930e7
💔 Build #27295 failed dfb1314
💔 Build #26806 failed 829e039
💔 Build #26786 failed 00a6bb3

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

sorenlouv · 2022-03-09T13:00:49Z

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

+  hasHistoricalData: true,
+  hasLegacyData: false,


I think hasLegacyData is leftover and should have been removed in #125962.
hasHistoricalData I'm not sure about - I don't see any references to it anywhere. Was that removed accidentally? Or perhaps we replaced that with something else when we introduced the new Observability "no data" screen?

tldr: I think both of these should be removed but please double check

hasHistoricalData was removed in #111630. It is no longer needed.

However, it looks like the message is now simply saying "No services found" when there is no data.

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

Lines 127 to 138 in 8b82657

const noItemsMessage = (

<EuiEmptyPrompt

title={

<div>

{i18n.translate('xpack.apm.servicesTable.notFoundLabel', {

defaultMessage: 'No services found',

})}

</div>

}

titleSize="s"

/>

);

It should instead say something like "No data found for selected period. Please adjust your time range and other possible filters" (because we know there is historical data available)

(fyi @MiriamAparicio)

sorenlouv · 2022-03-09T13:11:07Z

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

@@ -62,17 +77,17 @@ function useServicesFetcher() {
        }).then((mainStatisticsData) => {
          return {
            requestId: uuid(),


I know you didn't add this but wouldn't it be better if useFetcher added a unique id to each response.

const mainStatisticsFetch = useFetcher(); // ... const { requestId, data, status, error } = mainStatisticsFetch;

sorenlouv · 2022-03-09T13:14:05Z

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

  );

-  const { mainStatisticsData, requestId } = data;
+  const { data: mainStatisticsData = initialData } = mainStatisticsFetch;


nit: Probably just me being old school but I prefer this

Suggested change

const { data: mainStatisticsData = initialData } = mainStatisticsFetch;

const mainStatisticsData = mainStatisticsFetch.data ?? initialData;

sorenlouv · 2022-03-09T13:18:19Z

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

+  const initialSortField = displayHealthStatus ? 'healthStatus' : 'serviceName';
+  const initialSortDirection = displayHealthStatus ? 'desc' : 'asc';


nit

Suggested change

const initialSortField = displayHealthStatus ? 'healthStatus' : 'serviceName';

const initialSortDirection = displayHealthStatus ? 'desc' : 'asc';

const initialSort = displayHealthStatus

? {field: 'healthStatus', direction: 'desc' }

: {field: 'serviceName', direction: 'asc' }

sorenlouv · 2022-03-09T13:21:42Z

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

+  const services = joinByKey(
+    [
+      ...servicesWithHealthStatuses,
+      ...serviceNamesFromTermsEnum.map((serviceName) => ({ serviceName })),


Can we do this within getServicesFromTermsEnum? So it just returns a list of service names?

it does return just a list of service names (ie, strings). this converts it to an object to match what should be finally returned.

sorenlouv

just nits. Feel free to merge when green

kibanamachine · 2022-03-11T13:46:19Z