Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions x-pack/plugins/apm/common/service_inventory.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { AgentName } from '../typings/es_schemas/ui/fields/agent';
import { ServiceHealthStatus } from './service_health_status';

export interface ServiceListItem {
serviceName: string;
healthStatus?: ServiceHealthStatus;
transactionType?: string;
agentName?: AgentName;
throughput?: number;
latency?: number | null;
transactionErrorRate?: number | null;
environments?: string[];
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,21 +12,20 @@ import uuid from 'uuid';
import { useAnomalyDetectionJobsContext } from '../../../context/anomaly_detection_jobs/use_anomaly_detection_jobs_context';
import { useLegacyUrlParams } from '../../../context/url_params_context/use_url_params';
import { useLocalStorage } from '../../../hooks/use_local_storage';
import { useAnyOfApmParams } from '../../../hooks/use_apm_params';
import { useApmParams } from '../../../hooks/use_apm_params';
import { FETCH_STATUS, useFetcher } from '../../../hooks/use_fetcher';
import { useTimeRange } from '../../../hooks/use_time_range';
import { SearchBar } from '../../shared/search_bar';
import { getTimeRangeComparison } from '../../shared/time_comparison/get_time_range_comparison';
import { ServiceList } from './service_list';
import { MLCallout, shouldDisplayMlCallout } from '../../shared/ml_callout';
import { joinByKey } from '../../../../common/utils/join_by_key';

const initialData = {
requestId: '',
mainStatisticsData: {
items: [],
hasHistoricalData: true,
hasLegacyData: false,
},
items: [],
hasHistoricalData: true,
hasLegacyData: false,
Comment on lines +27 to +28
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@sorenlouv sorenlouv Mar 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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)

};

function useServicesFetcher() {
Expand All @@ -36,7 +35,7 @@ function useServicesFetcher() {

const {
query: { rangeFrom, rangeTo, environment, kuery },
} = useAnyOfApmParams('/services/{serviceName}', '/services');
} = useApmParams('/services');

const { start, end } = useTimeRange({ rangeFrom, rangeTo });

Expand All @@ -47,7 +46,23 @@ function useServicesFetcher() {
comparisonType,
});

const { data = initialData, status: mainStatisticsStatus } = useFetcher(
const sortedAndFilteredServicesFetch = useFetcher(
(callApmApi) => {
return callApmApi('GET /internal/apm/sorted_and_filtered_services', {
params: {
query: {
start,
end,
environment,
kuery,
},
},
});
},
[start, end, environment, kuery]
);

const mainStatisticsFetch = useFetcher(
(callApmApi) => {
if (start && end) {
return callApmApi('GET /internal/apm/services', {
Expand All @@ -62,17 +77,17 @@ function useServicesFetcher() {
}).then((mainStatisticsData) => {
return {
requestId: uuid(),
Copy link
Copy Markdown
Contributor

@sorenlouv sorenlouv Mar 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

mainStatisticsData,
...mainStatisticsData,
};
});
}
},
[environment, kuery, start, end]
);

const { mainStatisticsData, requestId } = data;
const { data: mainStatisticsData = initialData } = mainStatisticsFetch;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
const { data: mainStatisticsData = initialData } = mainStatisticsFetch;
const mainStatisticsData = mainStatisticsFetch.data ?? initialData;


const { data: comparisonData } = useFetcher(
const comparisonFetch = useFetcher(
(callApmApi) => {
if (start && end && mainStatisticsData.items.length) {
return callApmApi('GET /internal/apm/services/detailed_statistics', {
Expand All @@ -96,20 +111,23 @@ function useServicesFetcher() {
},
// only fetches detailed statistics when requestId is invalidated by main statistics api call or offset is changed
// eslint-disable-next-line react-hooks/exhaustive-deps
[requestId, offset],
[mainStatisticsData.requestId, offset],
{ preservePreviousData: false }
);

return {
mainStatisticsData,
mainStatisticsStatus,
comparisonData,
sortedAndFilteredServicesFetch,
mainStatisticsFetch,
comparisonFetch,
};
}

export function ServiceInventory() {
const { mainStatisticsData, mainStatisticsStatus, comparisonData } =
useServicesFetcher();
const {
sortedAndFilteredServicesFetch,
mainStatisticsFetch,
comparisonFetch,
} = useServicesFetcher();

const { anomalyDetectionSetupState } = useAnomalyDetectionJobsContext();

Expand All @@ -122,8 +140,13 @@ export function ServiceInventory() {
!userHasDismissedCallout &&
shouldDisplayMlCallout(anomalyDetectionSetupState);

const isLoading = mainStatisticsStatus === FETCH_STATUS.LOADING;
const isFailure = mainStatisticsStatus === FETCH_STATUS.FAILURE;
const isLoading =
sortedAndFilteredServicesFetch.status === FETCH_STATUS.LOADING ||
(sortedAndFilteredServicesFetch.status === FETCH_STATUS.SUCCESS &&
sortedAndFilteredServicesFetch.data?.services.length === 0 &&
mainStatisticsFetch.status === FETCH_STATUS.LOADING);

const isFailure = mainStatisticsFetch.status === FETCH_STATUS.FAILURE;
const noItemsMessage = (
<EuiEmptyPrompt
title={
Expand All @@ -137,6 +160,14 @@ export function ServiceInventory() {
/>
);

const items = joinByKey(
[
...(sortedAndFilteredServicesFetch.data?.services ?? []),
...(mainStatisticsFetch.data?.items ?? []),
],
'serviceName'
);

return (
<>
<SearchBar showTimeComparison />
Expand All @@ -154,8 +185,8 @@ export function ServiceInventory() {
<ServiceList
isLoading={isLoading}
isFailure={isFailure}
items={mainStatisticsData.items}
comparisonData={comparisonData}
items={items}
comparisonData={comparisonFetch?.data}
noItemsMessage={noItemsMessage}
/>
</EuiFlexItem>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ const stories: Meta<{}> = {
switch (endpoint) {
case '/internal/apm/services':
return { items: [] };

case '/internal/apm/sorted_and_filtered_services':
return { services: [] };

default:
return {};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import { i18n } from '@kbn/i18n';
import { TypeOf } from '@kbn/typed-react-router-config';
import { orderBy } from 'lodash';
import React, { useMemo } from 'react';
import { ValuesType } from 'utility-types';
import { NOT_AVAILABLE_LABEL } from '../../../../../common/i18n';
import { ServiceHealthStatus } from '../../../../../common/service_health_status';
import {
Expand Down Expand Up @@ -46,14 +45,11 @@ import {
getTimeSeriesColor,
} from '../../../shared/charts/helper/get_timeseries_color';
import { HealthBadge } from './health_badge';
import { ServiceListItem } from '../../../../../common/service_inventory';

type ServiceListAPIResponse = APIReturnType<'GET /internal/apm/services'>;
type Items = ServiceListAPIResponse['items'];
type ServicesDetailedStatisticsAPIResponse =
APIReturnType<'GET /internal/apm/services/detailed_statistics'>;

type ServiceListItem = ValuesType<Items>;

function formatString(value?: string | null) {
return value || NOT_AVAILABLE_LABEL;
}
Expand Down Expand Up @@ -239,7 +235,7 @@ export function getServiceColumns({
}

interface Props {
items: Items;
items: ServiceListItem[];
comparisonData?: ServicesDetailedStatisticsAPIResponse;
noItemsMessage?: React.ReactNode;
isLoading: boolean;
Expand Down Expand Up @@ -287,9 +283,8 @@ export function ServiceList({
]
);

const initialSortField = displayHealthStatus
? 'healthStatus'
: 'transactionsPerMinute';
const initialSortField = displayHealthStatus ? 'healthStatus' : 'serviceName';
const initialSortDirection = displayHealthStatus ? 'desc' : 'asc';
Comment on lines +286 to +287
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
const initialSortField = displayHealthStatus ? 'healthStatus' : 'serviceName';
const initialSortDirection = displayHealthStatus ? 'desc' : 'asc';
const initialSort = displayHealthStatus
? {field: 'healthStatus', direction: 'desc' }
: {field: 'serviceName', direction: 'asc' }


return (
<EuiFlexGroup gutterSize="xs" direction="column" responsive={false}>
Expand Down Expand Up @@ -336,9 +331,9 @@ export function ServiceList({
items={items}
noItemsMessage={noItemsMessage}
initialSortField={initialSortField}
initialSortDirection="desc"
initialSortDirection={initialSortDirection}
sortFn={(itemsToSort, sortField, sortDirection) => {
// For healthStatus, sort items by healthStatus first, then by TPM
// For healthStatus, sort items by healthStatus first, then by name
return sortField === 'healthStatus'
? orderBy(
itemsToSort,
Expand All @@ -348,9 +343,9 @@ export function ServiceList({
? SERVICE_HEALTH_STATUS_ORDER.indexOf(item.healthStatus)
: -1;
},
(item) => item.throughput ?? 0,
(item) => item.serviceName.toLowerCase(),
],
[sortDirection, sortDirection]
[sortDirection, sortDirection === 'asc' ? 'desc' : 'asc']
)
: orderBy(
itemsToSort,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,16 @@
*/

import { getSeverity } from '../../../../common/anomaly_detection';
import { getServiceHealthStatus } from '../../../../common/service_health_status';
import {
getServiceHealthStatus,
ServiceHealthStatus,
} from '../../../../common/service_health_status';
import { getServiceAnomalies } from '../../../routes/service_map/get_service_anomalies';
import { ServicesItemsSetup } from './get_services_items';

interface AggregationParams {
environment: string;
setup: ServicesItemsSetup;
searchAggregatedTransactions: boolean;
start: number;
end: number;
}
Expand All @@ -23,7 +25,9 @@ export const getHealthStatuses = async ({
setup,
start,
end,
}: AggregationParams) => {
}: AggregationParams): Promise<
Array<{ serviceName: string; healthStatus: ServiceHealthStatus }>
> => {
if (!setup.ml) {
return [];
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { Logger } from '@kbn/logging';
import { SERVICE_NAME } from '../../../../common/elasticsearch_fieldnames';
import { ENVIRONMENT_ALL } from '../../../../common/environment_filter_values';
import { Environment } from '../../../../common/environment_rt';
import { ProcessorEvent } from '../../../../common/processor_event';
import { joinByKey } from '../../../../common/utils/join_by_key';
import { Setup } from '../../../lib/helpers/setup_request';
import { getHealthStatuses } from './get_health_statuses';

export async function getSortedAndFilteredServices({
setup,
start,
end,
environment,
logger,
}: {
setup: Setup;
start: number;
end: number;
environment: Environment;
logger: Logger;
}) {
const { apmEventClient } = setup;

async function getServicesFromTermsEnum() {
if (environment !== ENVIRONMENT_ALL.value) {
return [];
}
const response = await apmEventClient.termsEnum(
'get_services_from_terms_enum',
{
apm: {
events: [
ProcessorEvent.transaction,
ProcessorEvent.span,
ProcessorEvent.metric,
ProcessorEvent.error,
],
},
body: {
size: 500,
field: SERVICE_NAME,
},
}
);

return response.terms;
}

const [servicesWithHealthStatuses, serviceNamesFromTermsEnum] =
await Promise.all([
getHealthStatuses({
setup,
start,
end,
environment,
}).catch((error) => {
logger.error(error);
return [];
}),
getServicesFromTermsEnum(),
]);

const services = joinByKey(
[
...servicesWithHealthStatuses,
...serviceNamesFromTermsEnum.map((serviceName) => ({ serviceName })),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

],
'serviceName'
);

return services;
}
Loading