[APM] Remove initial time range for service maps (#57308)

* [APM] Remove initial time range for service maps The initial time range for the service maps request might no longer be necessary, from what I can tell. Not having it improves the overall loading time and the user experience. * Remove loading indicator for service maps * Don't unnecessarily paginate * Unset loading indicator if component unmounts
elastic · Feb 12, 2020 · 6581426 · 6581426
1 parent cb97670
commit 6581426
Show file tree

Hide file tree

Showing 6 changed files with 58 additions and 42 deletions.
diff --git a/x-pack/legacy/plugins/apm/index.ts b/x-pack/legacy/plugins/apm/index.ts
@@ -72,8 +72,7 @@ export const apm: LegacyPluginInitializer = kibana => {
         autocreateApmIndexPattern: Joi.boolean().default(true),
 
         // service map
-        serviceMapEnabled: Joi.boolean().default(false),
-        serviceMapInitialTimeRange: Joi.number().default(60 * 1000 * 60) // last 1 hour
+        serviceMapEnabled: Joi.boolean().default(false)
       }).default();
     },
 

diff --git a/x-pack/legacy/plugins/apm/public/components/app/ServiceMap/index.tsx b/x-pack/legacy/plugins/apm/public/components/app/ServiceMap/index.tsx
@@ -27,10 +27,10 @@ import { useUrlParams } from '../../../hooks/useUrlParams';
 import { Controls } from './Controls';
 import { Cytoscape } from './Cytoscape';
 import { getCytoscapeElements } from './get_cytoscape_elements';
-import { LoadingOverlay } from './LoadingOverlay';
 import { PlatinumLicensePrompt } from './PlatinumLicensePrompt';
 import { Popover } from './Popover';
 import { useRefHeight } from './useRefHeight';
+import { useLoadingIndicator } from '../../../hooks/useLoadingIndicator';
 
 interface ServiceMapProps {
   serviceName?: string;
@@ -79,8 +79,9 @@ export function ServiceMap({ serviceName }: ServiceMapProps) {
   const openToast = useRef<string | null>(null);
 
   const [responses, setResponses] = useState<ServiceMapAPIResponse[]>([]);
-  const [isLoading, setIsLoading] = useState(true);
-  const [percentageLoaded, setPercentageLoaded] = useState(0);
+
+  const { setIsLoading } = useLoadingIndicator();
+
   const [, _setUnusedState] = useState(false);
 
   const elements = useMemo(() => getCytoscapeElements(responses, search), [
@@ -115,14 +116,14 @@ export function ServiceMap({ serviceName }: ServiceMapProps) {
             }
           });
           setResponses(resp => resp.concat(data));
-          setIsLoading(false);
 
           const shouldGetNext =
             responses.length + 1 < MAX_REQUESTS && data.after;
 
           if (shouldGetNext) {
-            setPercentageLoaded(value => value + 30); // increase loading bar 30%
             await getNext({ after: data.after });
+          } else {
+            setIsLoading(false);
           }
         } catch (error) {
           setIsLoading(false);
@@ -134,14 +135,12 @@ export function ServiceMap({ serviceName }: ServiceMapProps) {
         }
       }
     },
-    [callApmApi, params, responses.length, notifications.toasts]
+    [params, setIsLoading, callApmApi, responses.length, notifications.toasts]
   );
 
   useEffect(() => {
     const loadServiceMaps = async () => {
-      setPercentageLoaded(5);
       await getNext({ reset: true });
-      setPercentageLoaded(100);
     };
 
     loadServiceMaps();
@@ -167,7 +166,7 @@ export function ServiceMap({ serviceName }: ServiceMapProps) {
       forceUpdate();
     };
 
-    if (newElements.length > 0 && percentageLoaded === 100) {
+    if (newElements.length > 0 && renderedElements.current.length > 0) {
       openToast.current = notifications.toasts.add({
         title: i18n.translate('xpack.apm.newServiceMapData', {
           defaultMessage: `Newly discovered connections are available.`
@@ -193,7 +192,7 @@ export function ServiceMap({ serviceName }: ServiceMapProps) {
     };
 
     // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [elements, percentageLoaded]);
+  }, [elements]);
 
   const isValidPlatinumLicense =
     license?.isActive &&
@@ -212,10 +211,6 @@ export function ServiceMap({ serviceName }: ServiceMapProps) {
         height={height}
         style={cytoscapeDivStyle}
       >
-        <LoadingOverlay
-          isLoading={isLoading}
-          percentageLoaded={percentageLoaded}
-        />
         <Controls />
         <Popover focusedServiceName={serviceName} />
       </Cytoscape>

diff --git a/x-pack/legacy/plugins/apm/public/hooks/useFetcher.tsx b/x-pack/legacy/plugins/apm/public/hooks/useFetcher.tsx
@@ -9,10 +9,10 @@ import { i18n } from '@kbn/i18n';
 import { IHttpFetchError } from 'src/core/public';
 import { toMountPoint } from '../../../../../../src/plugins/kibana_react/public';
 import { LoadingIndicatorContext } from '../context/LoadingIndicatorContext';
-import { useComponentId } from './useComponentId';
 import { APMClient } from '../services/rest/createCallApmApi';
 import { useCallApmApi } from './useCallApmApi';
 import { useApmPluginContext } from './useApmPluginContext';
+import { useLoadingIndicator } from './useLoadingIndicator';
 
 export enum FETCH_STATUS {
   LOADING = 'loading',
@@ -44,7 +44,7 @@ export function useFetcher<TReturn>(
 ): Result<InferResponseType<TReturn>> & { refetch: () => void } {
   const { notifications } = useApmPluginContext().core;
   const { preservePreviousData = true } = options;
-  const id = useComponentId();
+  const { setIsLoading } = useLoadingIndicator();
 
   const callApmApi = useCallApmApi();
 
@@ -67,7 +67,7 @@ export function useFetcher<TReturn>(
         return;
       }
 
-      dispatchStatus({ id, isLoading: true });
+      setIsLoading(true);
 
       setResult(prevResult => ({
         data: preservePreviousData ? prevResult.data : undefined, // preserve data from previous state while loading next state
@@ -78,7 +78,7 @@ export function useFetcher<TReturn>(
       try {
         const data = await promise;
         if (!didCancel) {
-          dispatchStatus({ id, isLoading: false });
+          setIsLoading(false);
           setResult({
             data,
             status: FETCH_STATUS.SUCCESS,
@@ -109,7 +109,7 @@ export function useFetcher<TReturn>(
               </div>
             )
           });
-          dispatchStatus({ id, isLoading: false });
+          setIsLoading(false);
           setResult({
             data: undefined,
             status: FETCH_STATUS.FAILURE,
@@ -122,15 +122,15 @@ export function useFetcher<TReturn>(
     doFetch();
 
     return () => {
-      dispatchStatus({ id, isLoading: false });
+      setIsLoading(false);
       didCancel = true;
     };
     /* eslint-disable react-hooks/exhaustive-deps */
   }, [
     counter,
-    id,
     preservePreviousData,
     dispatchStatus,
+    setIsLoading,
     ...fnDeps
     /* eslint-enable react-hooks/exhaustive-deps */
   ]);

diff --git a/x-pack/legacy/plugins/apm/public/hooks/useLoadingIndicator.ts b/x-pack/legacy/plugins/apm/public/hooks/useLoadingIndicator.ts
@@ -0,0 +1,28 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+import { useContext, useMemo, useEffect } from 'react';
+import { LoadingIndicatorContext } from '../context/LoadingIndicatorContext';
+import { useComponentId } from './useComponentId';
+
+export function useLoadingIndicator() {
+  const { dispatchStatus } = useContext(LoadingIndicatorContext);
+  const id = useComponentId();
+
+  useEffect(() => {
+    return () => {
+      dispatchStatus({ id, isLoading: false });
+    };
+  }, [dispatchStatus, id]);
+
+  return useMemo(() => {
+    return {
+      setIsLoading: (loading: boolean) => {
+        dispatchStatus({ id, isLoading: loading });
+      }
+    };
+  }, [dispatchStatus, id]);
+}
diff --git a/x-pack/legacy/plugins/apm/server/lib/service_map/get_trace_sample_ids.ts b/x-pack/legacy/plugins/apm/server/lib/service_map/get_trace_sample_ids.ts
@@ -21,6 +21,7 @@ import {
   TRACE_ID
 } from '../../../common/elasticsearch_fieldnames';
 
+const MAX_CONNECTIONS_PER_REQUEST = 1000;
 const MAX_TRACES_TO_INSPECT = 1000;
 
 export async function getTraceSampleIds({
@@ -34,16 +35,9 @@ export async function getTraceSampleIds({
   environment?: string;
   setup: Setup & SetupTimeRange & SetupUIFilters;
 }) {
-  const isTop = !after;
+  const { start, end, client, indices } = setup;
 
-  const { start, end, client, indices, config } = setup;
-
-  const rangeEnd = end;
-  const rangeStart = isTop
-    ? rangeEnd - config['xpack.apm.serviceMapInitialTimeRange']
-    : start;
-
-  const rangeQuery = { range: rangeFilter(rangeStart, rangeEnd) };
+  const rangeQuery = { range: rangeFilter(start, end) };
 
   const query = {
     bool: {
@@ -71,10 +65,9 @@ export async function getTraceSampleIds({
     query.bool.filter.push({ term: { [SERVICE_ENVIRONMENT]: environment } });
   }
 
-  const afterObj =
-    after && after !== 'top'
-      ? { after: JSON.parse(Buffer.from(after, 'base64').toString()) }
-      : {};
+  const afterObj = after
+    ? { after: JSON.parse(Buffer.from(after, 'base64').toString()) }
+    : {};
 
   const params = {
     index: [indices['apm_oss.spanIndices']],
@@ -84,7 +77,7 @@ export async function getTraceSampleIds({
       aggs: {
         connections: {
           composite: {
-            size: 1000,
+            size: MAX_CONNECTIONS_PER_REQUEST,
             ...afterObj,
             sources: [
               { [SERVICE_NAME]: { terms: { field: SERVICE_NAME } } },
@@ -119,6 +112,7 @@ export async function getTraceSampleIds({
                 trace_ids: {
                   terms: {
                     field: TRACE_ID,
+                    size: 10,
                     execution_hint: 'map' as const,
                     // remove bias towards large traces by sorting on trace.id
                     // which will be random-esque
@@ -145,9 +139,11 @@ export async function getTraceSampleIds({
   const receivedAfterKey =
     tracesSampleResponse.aggregations?.connections.after_key;
 
-  if (!after) {
-    nextAfter = 'top';
-  } else if (receivedAfterKey) {
+  if (
+    receivedAfterKey &&
+    (tracesSampleResponse.aggregations?.connections.buckets.length ?? 0) >=
+      MAX_CONNECTIONS_PER_REQUEST
+  ) {
     nextAfter = Buffer.from(JSON.stringify(receivedAfterKey)).toString(
       'base64'
     );

diff --git a/x-pack/plugins/apm/server/index.ts b/x-pack/plugins/apm/server/index.ts
@@ -16,7 +16,6 @@ export const config = {
   },
   schema: schema.object({
     serviceMapEnabled: schema.boolean({ defaultValue: false }),
-    serviceMapInitialTimeRange: schema.number({ defaultValue: 60 * 1000 * 60 }), // last 1 hour
     autocreateApmIndexPattern: schema.boolean({ defaultValue: true }),
     ui: schema.object({
       enabled: schema.boolean({ defaultValue: true }),
@@ -38,7 +37,6 @@ export function mergeConfigs(apmOssConfig: APMOSSConfig, apmConfig: APMXPackConf
     'apm_oss.onboardingIndices': apmOssConfig.onboardingIndices,
     'apm_oss.indexPattern': apmOssConfig.indexPattern,
     'xpack.apm.serviceMapEnabled': apmConfig.serviceMapEnabled,
-    'xpack.apm.serviceMapInitialTimeRange': apmConfig.serviceMapInitialTimeRange,
     'xpack.apm.ui.enabled': apmConfig.ui.enabled,
     'xpack.apm.ui.maxTraceItems': apmConfig.ui.maxTraceItems,
     'xpack.apm.ui.transactionGroupBucketSize': apmConfig.ui.transactionGroupBucketSize,