Review feedback

elastic · Dec 8, 2020 · 8464326 · 8464326
1 parent dab72cd
commit 8464326
Show file tree

Hide file tree

Showing 9 changed files with 133 additions and 88 deletions.
diff --git a/.../apm/public/components/app/service_overview/service_overview_dependencies_table/index.tsx b/.../apm/public/components/app/service_overview/service_overview_dependencies_table/index.tsx
@@ -93,15 +93,15 @@ export function ServiceOverviewDependenciesTable({ serviceName }: Props) {
         return (
           <SparkPlotWithValueLabel
             color="euiColorVis1"
-            series={latency.timeseries ?? undefined}
+            series={latency.timeseries}
             valueLabel={asDuration(latency.value)}
           />
         );
       },
       sortable: true,
     },
     {
-      field: 'throughput_value',
+      field: 'throughputValue',
       name: i18n.translate(
         'xpack.apm.serviceOverview.dependenciesTableColumnThroughput',
         {
@@ -114,36 +114,36 @@ export function ServiceOverviewDependenciesTable({ serviceName }: Props) {
           <SparkPlotWithValueLabel
             compact
             color="euiColorVis0"
-            series={throughput.timeseries ?? undefined}
+            series={throughput.timeseries}
             valueLabel={asTransactionRate(throughput.value)}
           />
         );
       },
       sortable: true,
     },
     {
-      field: 'error_rate_value',
+      field: 'errorRateValue',
       name: i18n.translate(
         'xpack.apm.serviceOverview.dependenciesTableColumnErrorRate',
         {
           defaultMessage: 'Error rate',
         }
       ),
       width: px(unit * 10),
-      render: (_, { error_rate: errorRate }) => {
+      render: (_, { errorRate }) => {
         return (
           <SparkPlotWithValueLabel
             compact
             color="euiColorVis7"
-            series={errorRate.timeseries ?? undefined}
+            series={errorRate.timeseries}
             valueLabel={asPercent(errorRate.value, 1)}
           />
         );
       },
       sortable: true,
     },
     {
-      field: 'impact_value',
+      field: 'impactValue',
       name: i18n.translate(
         'xpack.apm.serviceOverview.dependenciesTableColumnImpact',
         {
@@ -186,10 +186,10 @@ export function ServiceOverviewDependenciesTable({ serviceName }: Props) {
   // need top-level sortable fields for the managed table
   const items = data.map((item) => ({
     ...item,
-    error_rate_value: item.error_rate.value,
-    latency_value: item.latency.value,
-    throughput_value: item.throughput.value,
-    impact_value: item.impact,
+    errorRateValue: item.errorRate.value,
+    latencyValue: item.latency.value,
+    throughputValue: item.throughput.value,
+    impactValue: item.impact,
   }));
 
   return (
@@ -235,7 +235,7 @@ export function ServiceOverviewDependenciesTable({ serviceName }: Props) {
               sorting={{
                 sort: {
                   direction: 'desc',
-                  field: 'impact_value',
+                  field: 'impactValue',
                 },
               }}
             />

diff --git a/x-pack/plugins/apm/public/components/shared/charts/spark_plot/index.tsx b/x-pack/plugins/apm/public/components/shared/charts/spark_plot/index.tsx
@@ -21,7 +21,7 @@ import { NOT_AVAILABLE_LABEL } from '../../../../../common/i18n';
 
 interface Props {
   color: string;
-  series?: Array<{ x: number; y: number | null }>;
+  series?: Array<{ x: number; y: number | null }> | null;
   width: string;
 }
 

diff --git a/...gins/apm/public/components/shared/charts/spark_plot/spark_plot_with_value_label/index.tsx b/...gins/apm/public/components/shared/charts/spark_plot/spark_plot_with_value_label/index.tsx
@@ -29,7 +29,7 @@ export function SparkPlotWithValueLabel({
   compact,
 }: {
   color: Color;
-  series?: Array<{ x: number; y: number | null }>;
+  series?: Array<{ x: number; y: number | null }> | null;
   valueLabel: React.ReactNode;
   compact?: boolean;
 }) {

diff --git a/x-pack/plugins/apm/public/components/shared/truncate_with_tooltip/index.tsx b/x-pack/plugins/apm/public/components/shared/truncate_with_tooltip/index.tsx
@@ -9,9 +9,11 @@ import React from 'react';
 import styled from 'styled-components';
 import { truncate } from '../../../style/variables';
 
+const tooltipAnchorClassname = '_apm_truncate_tooltip_anchor_';
+
 const TooltipWrapper = styled.div`
   width: 100%;
-  .euiToolTipAnchor {
+  .${tooltipAnchorClassname} {
     width: 100% !important;
     display: block !important;
   }
@@ -31,7 +33,11 @@ export function TruncateWithTooltip(props: Props) {
 
   return (
     <TooltipWrapper>
-      <EuiToolTip delay="long" content={text}>
+      <EuiToolTip
+        delay="long"
+        content={text}
+        anchorClassName={tooltipAnchorClassname}
+      >
         <ContentWrapper>{content || text}</ContentWrapper>
       </EuiToolTip>
     </TooltipWrapper>

diff --git a/x-pack/plugins/apm/server/lib/services/annotations/get_stored_annotations.ts b/x-pack/plugins/apm/server/lib/services/annotations/get_stored_annotations.ts
@@ -5,6 +5,7 @@
  */
 
 import { LegacyAPICaller, Logger } from 'kibana/server';
+import { rangeFilter } from '../../../../common/utils/range_filter';
 import { ESSearchResponse } from '../../../../../../typings/elasticsearch';
 import { Annotation as ESAnnotation } from '../../../../../observability/common/annotations';
 import { ScopedAnnotationsClient } from '../../../../../observability/server';
@@ -34,12 +35,7 @@ export async function getStoredAnnotations({
       bool: {
         filter: [
           {
-            range: {
-              '@timestamp': {
-                gte: setup.start,
-                lt: setup.end,
-              },
-            },
+            range: rangeFilter(setup.start, setup.end),
           },
           { term: { 'annotation.type': 'deployment' } },
           { term: { tags: 'apm' } },

diff --git a/x-pack/plugins/apm/server/lib/services/get_service_dependencies/get_destination_map.ts b/x-pack/plugins/apm/server/lib/services/get_service_dependencies/get_destination_map.ts
@@ -20,22 +20,20 @@ import {
 import { rangeFilter } from '../../../../common/utils/range_filter';
 import { ProcessorEvent } from '../../../../common/processor_event';
 import { getEnvironmentUiFilterES } from '../../helpers/convert_ui_filters/get_environment_ui_filter_es';
-import { APMEventClient } from '../../helpers/create_es_client/create_apm_event_client';
 import { joinByKey } from '../../../../common/utils/join_by_key';
+import { Setup, SetupTimeRange } from '../../helpers/setup_request';
 
 export const getDestinationMap = async ({
-  apmEventClient,
+  setup,
   serviceName,
-  start,
-  end,
   environment,
 }: {
-  apmEventClient: APMEventClient;
+  setup: Setup & SetupTimeRange;
   serviceName: string;
-  start: number;
-  end: number;
   environment: string;
 }) => {
+  const { start, end, apmEventClient } = setup;
+
   const response = await apmEventClient.search({
     apm: {
       events: [ProcessorEvent.span],
@@ -62,6 +60,8 @@ export const getDestinationMap = async ({
                   terms: { field: SPAN_DESTINATION_SERVICE_RESOURCE },
                 },
               },
+              // make sure we get samples for both successful
+              // and failed calls
               { [EVENT_OUTCOME]: { terms: { field: EVENT_OUTCOME } } },
             ],
           },
@@ -110,6 +110,7 @@ export const getDestinationMap = async ({
                 ),
               },
             },
+            { range: rangeFilter(start, end) },
           ],
         },
       },
@@ -133,13 +134,42 @@ export const getDestinationMap = async ({
     },
   }));
 
+  const joinedBySpanId = joinByKey(
+    [...outgoingConnections, ...joinByKey(incomingConnections, 'service')],
+    'id'
+  );
+
   const connections = joinByKey(
-    joinByKey(
-      [...outgoingConnections, ...joinByKey(incomingConnections, 'service')],
-      'id'
-    ),
+    joinedBySpanId,
     SPAN_DESTINATION_SERVICE_RESOURCE
-  );
+  ).map((connection) => {
+    const info = {
+      span: {
+        type: connection[SPAN_TYPE],
+        subtype: connection[SPAN_SUBTYPE],
+        destination: {
+          service: {
+            resource: connection[SPAN_DESTINATION_SERVICE_RESOURCE],
+          },
+        },
+      },
+    };
+
+    return {
+      ...info,
+      ...('service' in connection && connection.service
+        ? {
+            service: {
+              name: connection.service.name,
+              environment: connection.service.environment,
+            },
+            agent: {
+              name: connection.service.agentName,
+            },
+          }
+        : {}),
+    };
+  });
 
   // map span.destination.service.resource to an instrumented service (service.name, service.environment)
   // or an external service (span.type, span.subtype)

diff --git a/x-pack/plugins/apm/server/lib/services/get_service_dependencies/get_metrics.ts b/x-pack/plugins/apm/server/lib/services/get_service_dependencies/get_metrics.ts
@@ -15,25 +15,23 @@ import {
 import { rangeFilter } from '../../../../common/utils/range_filter';
 import { ProcessorEvent } from '../../../../common/processor_event';
 import { getEnvironmentUiFilterES } from '../../helpers/convert_ui_filters/get_environment_ui_filter_es';
-import { APMEventClient } from '../../helpers/create_es_client/create_apm_event_client';
 import { getBucketSize } from '../../helpers/get_bucket_size';
 import { EventOutcome } from '../../../../common/event_outcome';
+import { Setup, SetupTimeRange } from '../../helpers/setup_request';
 
 export const getMetrics = async ({
-  start,
-  end,
-  apmEventClient,
+  setup,
   serviceName,
   environment,
   numBuckets,
 }: {
-  start: number;
-  end: number;
+  setup: Setup & SetupTimeRange;
   serviceName: string;
-  apmEventClient: APMEventClient;
   environment: string;
   numBuckets: number;
 }) => {
+  const { start, end, apmEventClient } = setup;
+
   const response = await apmEventClient.search({
     apm: {
       events: [ProcessorEvent.metric],

diff --git a/x-pack/plugins/apm/server/lib/services/get_service_dependencies/index.ts b/x-pack/plugins/apm/server/lib/services/get_service_dependencies/index.ts
@@ -4,13 +4,9 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 import { ValuesType } from 'utility-types';
+import { isFiniteNumber } from '../../../../common/utils/is_finite_number';
 import { AgentName } from '../../../../typings/es_schemas/ui/fields/agent';
 import { joinByKey } from '../../../../common/utils/join_by_key';
-import {
-  SPAN_DESTINATION_SERVICE_RESOURCE,
-  SPAN_SUBTYPE,
-  SPAN_TYPE,
-} from '../../../../common/elasticsearch_fieldnames';
 import { Setup, SetupTimeRange } from '../../helpers/setup_request';
 import { getMetrics } from './get_metrics';
 import { getDestinationMap } from './get_destination_map';
@@ -25,7 +21,7 @@ export interface ServiceDependencyItem {
     value: number | null;
     timeseries: Array<{ x: number; y: number | null }>;
   };
-  error_rate: {
+  errorRate: {
     value: number | null;
     timeseries: Array<{ x: number; y: number | null }>;
   };
@@ -48,22 +44,18 @@ export async function getServiceDependencies({
   environment: string;
   numBuckets: number;
 }): Promise<ServiceDependencyItem[]> {
-  const { start, end, apmEventClient } = setup;
+  const { start, end } = setup;
 
   const [allMetrics, destinationMap] = await Promise.all([
     getMetrics({
-      start,
-      end,
-      apmEventClient,
+      setup,
       serviceName,
       environment,
       numBuckets,
     }),
     getDestinationMap({
-      apmEventClient,
+      setup,
       serviceName,
-      start,
-      end,
       environment,
     }),
   ]);
@@ -73,12 +65,18 @@ export async function getServiceDependencies({
 
     const destination = destinationMap[spanDestination];
 
-    return {
-      destination: destination
-        ? destination
-        : {
-            [SPAN_DESTINATION_SERVICE_RESOURCE]: metricItem.key,
+    const defaultInfo = {
+      span: {
+        destination: {
+          service: {
+            resource: metricItem.key,
           },
+        },
+      },
+    };
+
+    return {
+      destination: destination ? destination : defaultInfo,
       metrics: [metricItem],
     };
   }, []);
@@ -150,7 +148,7 @@ export async function getServiceDependencies({
             y: point.count > 0 ? point.count / deltaAsMinutes : null,
           })),
         },
-        error_rate: {
+        errorRate: {
           value:
             mergedMetrics.value.count > 0
               ? (mergedMetrics.value.error_count ?? 0) /
@@ -165,28 +163,30 @@ export async function getServiceDependencies({
 
       if ('service' in destination) {
         return {
-          name: destination.service!.name,
-          serviceName: destination.service!.name,
-          environment: destination.service!.environment,
-          agentName: destination.service!.agentName,
+          name: destination.service.name,
+          serviceName: destination.service.name,
+          environment: destination.service.environment,
+          agentName: destination.agent.name,
           ...destMetrics,
         };
       }
 
       return {
-        name: destination[SPAN_DESTINATION_SERVICE_RESOURCE],
-        spanType:
-          'span.type' in destination ? destination[SPAN_TYPE] : undefined,
-        spanSubtype:
-          'span.subtype' in destination ? destination[SPAN_SUBTYPE] : undefined,
+        name: destination.span.destination.service.resource!,
+        ...('span' in destination && 'type' in destination.span
+          ? {
+              spanType: destination.span.type,
+              spanSubtype: destination.span.subtype,
+            }
+          : {}),
         ...destMetrics,
       };
     }
   );
 
   const latencySums = metricsByResolvedAddress
     .map((metrics) => metrics.latency.value)
-    .filter((n) => n !== null) as number[];
+    .filter(isFiniteNumber);
 
   const minLatencySum = Math.min(...latencySums);
   const maxLatencySum = Math.max(...latencySums);