Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[APM] Service overview: Ensure transactionType is used for by every component #85665

Merged
merged 14 commits into from
Dec 18, 2020
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ import * as useTransactionBreakdownHooks from '../../shared/charts/transaction_b
import { renderWithTheme } from '../../../utils/testHelpers';
import { ServiceOverview } from './';
import { waitFor } from '@testing-library/dom';
import * as callApmApi from '../../../services/rest/createCallApmApi';
import * as callApmApiModule from '../../../services/rest/createCallApmApi';
import * as useApmServiceContextHooks from '../../../context/apm_service/use_apm_service_context';

const KibanaReactContext = createKibanaReactContext({
usageCollection: { reportUiCounter: () => {} },
Expand Down Expand Up @@ -56,6 +57,13 @@ function Wrapper({ children }: { children?: ReactNode }) {

describe('ServiceOverview', () => {
it('renders', async () => {
jest
.spyOn(useApmServiceContextHooks, 'useApmServiceContext')
.mockReturnValue({
agentName: 'java',
transactionType: 'request',
transactionTypes: ['request'],
});
jest
.spyOn(useAnnotationsHooks, 'useAnnotationsContext')
.mockReturnValue({ annotations: [] });
Expand All @@ -78,17 +86,23 @@ describe('ServiceOverview', () => {
isAggregationAccurate: true,
},
'GET /api/apm/services/{serviceName}/dependencies': [],
// eslint-disable-next-line @typescript-eslint/naming-convention
'GET /api/apm/services/{serviceName}/service_overview_instances': [],
Copy link
Member

Choose a reason for hiding this comment

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

What is eslint complaining about here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Property name GET /api/apm/services/{serviceName}/error_groups must match one of the following formats: camelCase, PascalCase, snake_case, UPPER_CASE

};

jest.spyOn(callApmApi, 'createCallApmApi').mockImplementation(() => {});
jest
.spyOn(callApmApiModule, 'createCallApmApi')
.mockImplementation(() => {});

jest.spyOn(callApmApi, 'callApmApi').mockImplementation(({ endpoint }) => {
const response = calls[endpoint as keyof typeof calls];
const callApmApi = jest
.spyOn(callApmApiModule, 'callApmApi')
.mockImplementation(({ endpoint }) => {
const response = calls[endpoint as keyof typeof calls];

return response
? Promise.resolve(response)
: Promise.reject(`Response for ${endpoint} is not defined`);
});
return response
? Promise.resolve(response)
: Promise.reject(`Response for ${endpoint} is not defined`);
});
jest
.spyOn(useTransactionBreakdownHooks, 'useTransactionBreakdown')
.mockReturnValue({
Expand All @@ -105,9 +119,7 @@ describe('ServiceOverview', () => {
);

await waitFor(() =>
expect(callApmApi.callApmApi).toHaveBeenCalledTimes(
Object.keys(calls).length
)
expect(callApmApi).toHaveBeenCalledTimes(Object.keys(calls).length)
);

expect((await findAllByText('Latency')).length).toBeGreaterThan(0);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@ import React, { useState } from 'react';
import styled from 'styled-components';
import { EuiBasicTable } from '@elastic/eui';
import { asInteger } from '../../../../../common/utils/formatters';
import { FETCH_STATUS, useFetcher } from '../../../../hooks/use_fetcher';
import { useApmServiceContext } from '../../../../context/apm_service/use_apm_service_context';
import { useUrlParams } from '../../../../context/url_params_context/use_url_params';
import { FETCH_STATUS, useFetcher } from '../../../../hooks/use_fetcher';
import { callApmApi } from '../../../../services/rest/createCallApmApi';
import { px, truncate, unit } from '../../../../style/variables';
import { SparkPlot } from '../../../shared/charts/spark_plot';
Expand Down Expand Up @@ -67,7 +68,7 @@ export function ServiceOverviewErrorsTable({ serviceName }: Props) {
urlParams: { start, end },
uiFilters,
} = useUrlParams();

const { transactionType } = useApmServiceContext();
const [tableOptions, setTableOptions] = useState<{
pageIndex: number;
sort: {
Expand Down Expand Up @@ -153,7 +154,7 @@ export function ServiceOverviewErrorsTable({ serviceName }: Props) {
},
status,
} = useFetcher(() => {
if (!start || !end) {
if (!start || !end || !transactionType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For readability, something like:

const hasRequiredParams = start && end && transactionType;
if (!hasRequiredParams) {
  return;
}

Copy link
Member

Choose a reason for hiding this comment

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

We are already using the "or not" style so I think it's fine to keep it. At least that's consistent

return;
}

Expand All @@ -170,6 +171,7 @@ export function ServiceOverviewErrorsTable({ serviceName }: Props) {
pageIndex: tableOptions.pageIndex,
sortField: tableOptions.sort.field,
sortDirection: tableOptions.sort.direction,
transactionType,
},
},
}).then((response) => {
Expand All @@ -193,6 +195,7 @@ export function ServiceOverviewErrorsTable({ serviceName }: Props) {
tableOptions.pageIndex,
tableOptions.sort.field,
tableOptions.sort.direction,
transactionType,
]);

const {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import { TableFetchWrapper } from '../../../shared/table_fetch_wrapper';
import { TableLinkFlexItem } from '../table_link_flex_item';
import { SparkPlot } from '../../../shared/charts/spark_plot';
import { ImpactBar } from '../../../shared/ImpactBar';
import { useApmServiceContext } from '../../../../context/apm_service/use_apm_service_context';
import { ServiceOverviewTableContainer } from '../service_overview_table_container';

type ServiceTransactionGroupItem = ValuesType<
Expand All @@ -46,7 +47,7 @@ interface Props {
serviceName: string;
}

type SortField = 'latency' | 'throughput' | 'errorRate' | 'impact';
type SortField = 'name' | 'latency' | 'throughput' | 'errorRate' | 'impact';
type SortDirection = 'asc' | 'desc';

const PAGE_SIZE = 5;
Expand Down Expand Up @@ -98,7 +99,9 @@ function getLatencyAggregationTypeLabel(
}
}

export function ServiceOverviewTransactionsTable({ serviceName }: Props) {
export function ServiceOverviewTransactionsTable(props: Props) {
const { serviceName } = props;
const { transactionType } = useApmServiceContext();
const latencyAggregationType = useLatencyAggregationType();
const {
uiFilters,
Expand Down Expand Up @@ -127,7 +130,7 @@ export function ServiceOverviewTransactionsTable({ serviceName }: Props) {
},
status,
} = useFetcher(() => {
if (!start || !end || !latencyAggregationType) {
if (!start || !end || !latencyAggregationType || !transactionType) {
return;
}

Expand All @@ -145,6 +148,7 @@ export function ServiceOverviewTransactionsTable({ serviceName }: Props) {
pageIndex: tableOptions.pageIndex,
sortField: tableOptions.sort.field,
sortDirection: tableOptions.sort.direction,
transactionType,
latencyAggregationType,
},
},
Expand All @@ -169,6 +173,7 @@ export function ServiceOverviewTransactionsTable({ serviceName }: Props) {
tableOptions.pageIndex,
tableOptions.sort.field,
tableOptions.sort.direction,
transactionType,
latencyAggregationType,
]);

Expand All @@ -187,14 +192,14 @@ export function ServiceOverviewTransactionsTable({ serviceName }: Props) {
defaultMessage: 'Name',
}
),
render: (_, { name, transactionType }) => {
render: (_, { name, transactionType: type }) => {
return (
<TransactionGroupLinkWrapper>
<EuiToolTip delay="long" content={name}>
<StyledTransactionDetailLink
serviceName={serviceName}
transactionName={name}
transactionType={transactionType}
transactionType={type}
>
{name}
</StyledTransactionDetailLink>
Expand Down Expand Up @@ -239,7 +244,7 @@ export function ServiceOverviewTransactionsTable({ serviceName }: Props) {
},
},
{
field: 'error_rate',
field: 'errorRate',
name: i18n.translate(
'xpack.apm.serviceOverview.transactionsTableColumnErrorRate',
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { useTheme } from '../../../../hooks/use_theme';
import { useUrlParams } from '../../../../context/url_params_context/use_url_params';
import { callApmApi } from '../../../../services/rest/createCallApmApi';
import { TimeseriesChart } from '../timeseries_chart';
import { useApmServiceContext } from '../../../../context/apm_service/use_apm_service_context';

function yLabelFormat(y?: number | null) {
return asPercent(y || 0, 1);
Expand All @@ -31,8 +32,8 @@ export function TransactionErrorRateChart({
const theme = useTheme();
const { serviceName } = useParams<{ serviceName?: string }>();
const { urlParams, uiFilters } = useUrlParams();

const { start, end, transactionType, transactionName } = urlParams;
const { transactionType } = useApmServiceContext();
const { start, end, transactionName } = urlParams;

const { data, status } = useFetcher(() => {
if (serviceName && start && end) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
ERROR_GROUP_ID,
ERROR_LOG_MESSAGE,
SERVICE_NAME,
TRANSACTION_TYPE,
} from '../../../../common/elasticsearch_fieldnames';
import { Setup, SetupTimeRange } from '../../helpers/setup_request';
import { getBucketSize } from '../../helpers/get_bucket_size';
Expand All @@ -32,6 +33,7 @@ export async function getServiceErrorGroups({
pageIndex,
sortDirection,
sortField,
transactionType,
}: {
serviceName: string;
setup: Setup & SetupTimeRange;
Expand All @@ -40,6 +42,7 @@ export async function getServiceErrorGroups({
numBuckets: number;
sortDirection: 'asc' | 'desc';
sortField: 'name' | 'last_seen' | 'occurrences';
transactionType: string;
}) {
const { apmEventClient, start, end, esFilter } = setup;

Expand All @@ -55,6 +58,7 @@ export async function getServiceErrorGroups({
bool: {
filter: [
{ term: { [SERVICE_NAME]: serviceName } },
{ term: { [TRANSACTION_TYPE]: transactionType } },
{ range: rangeFilter(start, end) },
...esFilter,
],
Expand Down Expand Up @@ -127,6 +131,7 @@ export async function getServiceErrorGroups({
filter: [
{ terms: { [ERROR_GROUP_ID]: sortedErrorGroupIds } },
{ term: { [SERVICE_NAME]: serviceName } },
{ term: { [TRANSACTION_TYPE]: transactionType } },
{ range: rangeFilter(start, end) },
...esFilter,
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export async function getTimeseriesDataForTransactionGroups({
searchAggregatedTransactions,
size,
numBuckets,
transactionType,
latencyAggregationType,
}: {
apmEventClient: APMEventClient;
Expand All @@ -49,6 +50,7 @@ export async function getTimeseriesDataForTransactionGroups({
searchAggregatedTransactions: boolean;
size: number;
numBuckets: number;
transactionType: string;
latencyAggregationType: LatencyAggregationType;
}) {
const { intervalString } = getBucketSize({ start, end, numBuckets });
Expand All @@ -72,6 +74,7 @@ export async function getTimeseriesDataForTransactionGroups({
filter: [
{ terms: { [TRANSACTION_NAME]: transactionNames } },
{ term: { [SERVICE_NAME]: serviceName } },
{ term: { [TRANSACTION_TYPE]: transactionType } },
{ range: rangeFilter(start, end) },
...esFilter,
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
EVENT_OUTCOME,
SERVICE_NAME,
TRANSACTION_NAME,
TRANSACTION_TYPE,
} from '../../../../common/elasticsearch_fieldnames';
import {
getProcessorEventForAggregatedTransactions,
Expand All @@ -26,6 +27,7 @@ import {
} from '../../helpers/latency_aggregation_type';

export type ServiceOverviewTransactionGroupSortField =
| 'name'
| 'latency'
| 'throughput'
| 'errorRate'
Expand All @@ -46,6 +48,7 @@ export async function getTransactionGroupsForPage({
sortDirection,
pageIndex,
size,
transactionType,
latencyAggregationType,
}: {
apmEventClient: APMEventClient;
Expand All @@ -58,6 +61,7 @@ export async function getTransactionGroupsForPage({
sortDirection: 'asc' | 'desc';
pageIndex: number;
size: number;
transactionType: string;
latencyAggregationType: LatencyAggregationType;
}) {
const field = getTransactionDurationFieldForAggregatedTransactions(
Expand All @@ -78,6 +82,7 @@ export async function getTransactionGroupsForPage({
bool: {
filter: [
{ term: { [SERVICE_NAME]: serviceName } },
{ term: { [TRANSACTION_TYPE]: transactionType } },
{ range: rangeFilter(start, end) },
...esFilter,
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export async function getServiceTransactionGroups({
sortDirection,
sortField,
searchAggregatedTransactions,
transactionType,
latencyAggregationType,
}: {
serviceName: string;
Expand All @@ -32,6 +33,7 @@ export async function getServiceTransactionGroups({
sortDirection: 'asc' | 'desc';
sortField: ServiceOverviewTransactionGroupSortField;
searchAggregatedTransactions: boolean;
transactionType: string;
latencyAggregationType: LatencyAggregationType;
}) {
const { apmEventClient, start, end, esFilter } = setup;
Expand All @@ -51,6 +53,7 @@ export async function getServiceTransactionGroups({
sortDirection,
size,
searchAggregatedTransactions,
transactionType,
latencyAggregationType,
});

Expand All @@ -66,6 +69,7 @@ export async function getServiceTransactionGroups({
serviceName,
size,
transactionNames,
transactionType,
latencyAggregationType,
});

Expand Down
11 changes: 10 additions & 1 deletion x-pack/plugins/apm/server/routes/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,7 @@ export const serviceErrorGroupsRoute = createRoute({
t.literal('occurrences'),
t.literal('name'),
]),
transactionType: t.string,
}),
]),
}),
Expand All @@ -234,7 +235,14 @@ export const serviceErrorGroupsRoute = createRoute({

const {
path: { serviceName },
query: { size, numBuckets, pageIndex, sortDirection, sortField },
query: {
numBuckets,
pageIndex,
size,
sortDirection,
sortField,
transactionType,
},
} = context.params;
return getServiceErrorGroups({
serviceName,
Expand All @@ -244,6 +252,7 @@ export const serviceErrorGroupsRoute = createRoute({
pageIndex,
sortDirection,
sortField,
transactionType,
});
},
});
Expand Down
Loading