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.

Sign up for GitHub

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] transactionType should be required on service-specific endpoints #86893

Merged
merged 4 commits into from
Jan 5, 2021
Merged

[APM] transactionType should be required on service-specific endpoints #86893

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6cfda7a
making transaction type required on some apis
cauemarcondes Dec 23, 2020
9e65952
Merge branch 'master' into apm-duplicate-api-calls
kibanamachine Dec 28, 2020
4762c95
Merge branch 'master' into apm-duplicate-api-calls
kibanamachine Jan 4, 2021
e553e8c
addressing PR comments
cauemarcondes Jan 4, 2021
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
2 changes: 1 addition & 1 deletion x-pack/plugins/apm/public/components/shared/charts/transaction_error_rate_chart/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export function TransactionErrorRateChart({
const { start, end, transactionName } = urlParams;

const { data, status } = useFetcher(() => {
if (serviceName && start && end) {
if (transactionType && serviceName && start && end) {
return callApmApi({
endpoint:
'GET /api/apm/services/{serviceName}/transactions/charts/error_rate',
Expand Down
6 changes: 4 additions & 2 deletions x-pack/plugins/apm/public/hooks/use_transaction_throughput_chart_fetcher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,20 @@ import { useFetcher } from './use_fetcher';
import { useUrlParams } from '../context/url_params_context/use_url_params';
import { getThrouputChartSelector } from '../selectors/throuput_chart_selectors';
import { useTheme } from './use_theme';
import { useApmServiceContext } from '../context/apm_service/use_apm_service_context';

export function useTransactionThroughputChartsFetcher() {
const { serviceName } = useParams<{ serviceName?: string }>();
const { transactionType } = useApmServiceContext();
const theme = useTheme();
const {
urlParams: { transactionType, start, end, transactionName },
urlParams: { start, end, transactionName },
uiFilters,
} = useUrlParams();

const { data, error, status } = useFetcher(
(callApmApi) => {
if (serviceName && start && end) {
if (transactionType && serviceName && start && end) {
return callApmApi({
endpoint:
'GET /api/apm/services/{serviceName}/transactions/charts/throughput',
Expand Down
5 changes: 0 additions & 5 deletions ...lib/services/get_service_transaction_groups/get_timeseries_data_for_transaction_groups.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,6 @@ export async function getTimeseriesDataForTransactionGroups({
size,
},
aggs: {
transaction_types: {
terms: {
field: TRANSACTION_TYPE,
},
},
timeseries: {
date_histogram: {
field: '@timestamp',
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/apm/server/lib/services/get_service_transaction_groups/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ export async function getServiceTransactionGroups({
start,
end,
latencyAggregationType,
transactionType,
}),
totalTransactionGroups,
isAggregationAccurate,
Expand Down
21 changes: 3 additions & 18 deletions ...ns/apm/server/lib/services/get_service_transaction_groups/merge_transaction_group_data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,10 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { LatencyAggregationType } from '../../../../common/latency_aggregation_types';
import { EVENT_OUTCOME } from '../../../../common/elasticsearch_fieldnames';

import {
TRANSACTION_PAGE_LOAD,
TRANSACTION_REQUEST,
} from '../../../../common/transaction_types';
import { LatencyAggregationType } from '../../../../common/latency_aggregation_types';
import { getLatencyValue } from '../../helpers/latency_aggregation_type';

import { TransactionGroupTimeseriesData } from './get_timeseries_data_for_transaction_groups';

import { TransactionGroupWithoutTimeseriesData } from './get_transaction_groups_for_page';

export function mergeTransactionGroupData({
Expand All @@ -23,12 +16,14 @@ export function mergeTransactionGroupData({
transactionGroups,
timeseriesData,
latencyAggregationType,
transactionType,
}: {
start: number;
end: number;
transactionGroups: TransactionGroupWithoutTimeseriesData[];
timeseriesData: TransactionGroupTimeseriesData;
latencyAggregationType: LatencyAggregationType;
transactionType: string;
Copy link
Member

Choose a reason for hiding this comment

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

I think there's still a terms aggregation on transaction.type in the original search that we might not need anymore. Can you have a look? (Might be that we are doing it in other places that can now be removed as well).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm going to remove this aggregation https://github.com/cauemarcondes/kibana/blob/apm-duplicate-api-calls/x-pack/plugins/apm/server/lib/services/get_service_transaction_groups/get_timeseries_data_for_transaction_groups.ts#L90-L94

}) {
const deltaAsMinutes = (end - start) / 1000 / 60;

Expand All @@ -37,16 +32,6 @@ export function mergeTransactionGroupData({
({ key }) => key === transactionGroup.name
);

const transactionTypes =
groupBucket?.transaction_types.buckets.map(
(bucket) => bucket.key as string
) ?? [];

const transactionType =
transactionTypes.find(
(type) => type === TRANSACTION_PAGE_LOAD || type === TRANSACTION_REQUEST
) ?? transactionTypes[0];

const timeseriesBuckets = groupBucket?.timeseries.buckets ?? [];

return timeseriesBuckets.reduce(
Expand Down
9 changes: 3 additions & 6 deletions x-pack/plugins/apm/server/lib/transactions/get_throughput_charts/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ async function searchThroughput({
intervalString,
}: {
serviceName: string;
transactionType: string | undefined;
transactionType: string;
transactionName: string | undefined;
setup: Setup & SetupTimeRange;
searchAggregatedTransactions: boolean;
Expand All @@ -48,17 +48,14 @@ async function searchThroughput({
...getDocumentTypeFilterForAggregatedTransactions(
searchAggregatedTransactions
),
{ term: { [TRANSACTION_TYPE]: transactionType } },
...setup.esFilter,
];

if (transactionName) {
filter.push({ term: { [TRANSACTION_NAME]: transactionName } });
}

if (transactionType) {
filter.push({ term: { [TRANSACTION_TYPE]: transactionType } });
}

const field = getTransactionDurationFieldForAggregatedTransactions(
searchAggregatedTransactions
);
Expand Down Expand Up @@ -104,7 +101,7 @@ export async function getThroughputCharts({
searchAggregatedTransactions,
}: {
serviceName: string;
transactionType: string | undefined;
transactionType: string;
transactionName: string | undefined;
setup: Setup & SetupTimeRange;
searchAggregatedTransactions: boolean;
Expand Down
24 changes: 7 additions & 17 deletions x-pack/plugins/apm/server/routes/transactions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,7 @@ export const transactionGroupsRoute = createRoute({
serviceName: t.string,
}),
query: t.intersection([
t.type({
transactionType: t.string,
}),
t.type({ transactionType: t.string }),
uiFiltersRt,
rangeRt,
]),
Expand Down Expand Up @@ -199,10 +197,8 @@ export const transactionThroughputChatsRoute = createRoute({
serviceName: t.string,
}),
query: t.intersection([
t.partial({
transactionType: t.string,
transactionName: t.string,
}),
t.type({ transactionType: t.string }),
t.partial({ transactionName: t.string }),
uiFiltersRt,
rangeRt,
]),
Expand Down Expand Up @@ -287,12 +283,8 @@ export const transactionChartsBreakdownRoute = createRoute({
serviceName: t.string,
}),
query: t.intersection([
t.type({
transactionType: t.string,
}),
t.partial({
transactionName: t.string,
}),
t.type({ transactionType: t.string }),
t.partial({ transactionName: t.string }),
uiFiltersRt,
rangeRt,
]),
Expand Down Expand Up @@ -322,10 +314,8 @@ export const transactionChartsErrorRateRoute = createRoute({
query: t.intersection([
uiFiltersRt,
rangeRt,
t.partial({
transactionType: t.string,
transactionName: t.string,
}),
t.type({ transactionType: t.string }),
t.partial({ transactionName: t.string }),
]),
}),
options: { tags: ['access:apm'] },
Expand Down
7 changes: 0 additions & 7 deletions x-pack/test/apm_api_integration/basic/tests/feature_controls.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,13 +126,6 @@ export default function featureControlsTests({ getService }: FtrProviderContext)
expectForbidden: expect403,
expectResponse: expect200,
},
{
req: {
url: `/api/apm/services/foo/transactions/charts/throughput?start=${start}&end=${end}&uiFilters=%7B%22environment%22%3A%22testing%22%7D`,
},
expectForbidden: expect403,
expectResponse: expect200,
Comment on lines -129 to -134
Copy link
Member

Choose a reason for hiding this comment

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

Why is this removed? Shouldn't we just add a transaction type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the one above already tests with a transaction type:

https://github.com/elastic/kibana/pull/86893/files/e553e8cccea4ca418164a9af044d6a461afb0f90#diff-00494678ca34f1d18e071f41cc255917ac4027a29f09d27777b5d5b0d940f013L124

},
{
req: {
url: `/api/apm/services/foo/transactions/charts/throughput?start=${start}&end=${end}&transactionType=bar&transactionName=baz&uiFilters=%7B%22environment%22%3A%22testing%22%7D`,
Expand Down
26 changes: 21 additions & 5 deletions x-pack/test/apm_api_integration/basic/tests/transactions/throughput.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/
import expect from '@kbn/expect';
import url from 'url';
import archives_metadata from '../../../common/archives_metadata';
import { PromiseReturnType } from '../../../../../plugins/observability/typings/common';
import { FtrProviderContext } from '../../../../common/ftr_provider_context';
Expand All @@ -16,15 +17,22 @@ export default function ApiTest({ getService }: FtrProviderContext) {
const metadata = archives_metadata[archiveName];

// url parameters
const start = encodeURIComponent(metadata.start);
const end = encodeURIComponent(metadata.end);
const uiFilters = encodeURIComponent(JSON.stringify({ environment: 'testing' }));
const { start, end } = metadata;
const uiFilters = JSON.stringify({ environment: 'testing' });

describe('Throughput', () => {
describe('when data is not loaded ', () => {
it('handles the empty state', async () => {
const response = await supertest.get(
`/api/apm/services/opbeans-node/transactions/charts/throughput?start=${start}&end=${end}&uiFilters=${uiFilters}`
url.format({
pathname: `/api/apm/services/opbeans-node/transactions/charts/throughput`,
query: {
start,
end,
uiFilters,
transactionType: 'request',
},
})
);

expect(response.status).to.be(200);
Expand All @@ -41,7 +49,15 @@ export default function ApiTest({ getService }: FtrProviderContext) {

before(async () => {
response = await supertest.get(
`/api/apm/services/opbeans-node/transactions/charts/throughput?start=${start}&end=${end}&uiFilters=${uiFilters}`
url.format({
pathname: `/api/apm/services/opbeans-node/transactions/charts/throughput`,
query: {
start,
end,
uiFilters,
transactionType: 'request',
},
})
);
});

Expand Down