Skip to content

Commit

Permalink
[APM] Navigation from Span to Service breaks due to wrong transaction…
Browse files Browse the repository at this point in the history
…Type (#136569)

* fixing bug

* addressing pr comments

* fix bug

* renaming
  • Loading branch information
cauemarcondes authored Jul 25, 2022
1 parent 36260fb commit b8b57d5
Show file tree
Hide file tree
Showing 4 changed files with 82 additions and 36 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
import { EuiFlexGroup, EuiFlexItem, EuiLink, EuiPanel } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import React from 'react';
import { useHistory } from 'react-router-dom';
import {
isRumAgentName,
isMobileAgentName,
Expand All @@ -31,7 +30,6 @@ import { useApmParams } from '../../../hooks/use_apm_params';
import { AggregatedTransactionsBadge } from '../../shared/aggregated_transactions_badge';
import { useApmRouter } from '../../../hooks/use_apm_router';
import { useTimeRange } from '../../../hooks/use_time_range';
import { replace } from '../../shared/links/url_helpers';

/**
* The height a chart should be if it's next to a table with 5 rows and a title.
Expand All @@ -40,33 +38,16 @@ import { replace } from '../../shared/links/url_helpers';
export const chartHeight = 288;

export function ServiceOverview() {
const {
agentName,
serviceName,
transactionType,
fallbackToTransactions,
runtimeName,
} = useApmServiceContext();
const { agentName, serviceName, fallbackToTransactions, runtimeName } =
useApmServiceContext();

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

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

const history = useHistory();

// redirect to first transaction type
if (!transactionTypeFromUrl && transactionType) {
replace(history, { query: { transactionType } });
}

const latencyChartHeight = 200;

// The default EuiFlexGroup breaks at 768, but we want to break at 1200, so we
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,17 @@ export function FlyoutTopLevelProperties({ transaction }: Props) {
val: (
<ServiceLink
agentName={transaction.agent.name}
query={{ ...query, serviceGroup, environment: nextEnvironment }}
query={{
kuery: query.kuery,
latencyAggregationType,
offset: query.offset,
rangeFrom: query.rangeFrom,
rangeTo: query.rangeTo,
comparisonEnabled: query.comparisonEnabled,
transactionType: transaction.transaction.type,
serviceGroup,
environment: nextEnvironment,
}}
serviceName={transaction.service.name}
/>
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,26 +5,49 @@
* 2.0.
*/

import { getTransactionType } from './apm_service_context';
import { getOrRedirectToTransactionType } from './apm_service_context';
import { createMemoryHistory } from 'history';

describe('getOrRedirectToTransactionType', () => {
const history = createMemoryHistory();
jest.spyOn(history, 'replace');

describe('getTransactionType', () => {
describe('with transaction type in url', () => {
it('returns the transaction type in the url ', () => {
expect(
getTransactionType({
transactionTypes: ['worker', 'request'],
getOrRedirectToTransactionType({
transactionTypes: ['worker', 'request', 'custom'],
transactionType: 'custom',
agentName: 'nodejs',
history,
})
).toBe('custom');
expect(history.replace).not.toHaveBeenCalled();
});

it('updates the transaction type in the url when it is not one of the options returned by the API', () => {
expect(
getOrRedirectToTransactionType({
transactionTypes: ['worker', 'request'],
transactionType: 'custom',
agentName: 'nodejs',
history,
})
).toBe('request');
expect(history.replace).toHaveBeenCalledWith(
expect.objectContaining({
search: 'transactionType=request',
})
);
});
});

describe('with no transaction types', () => {
it('returns undefined', () => {
expect(
getTransactionType({
getOrRedirectToTransactionType({
transactionTypes: [],
history,
})
).toBeUndefined();
});
Expand All @@ -34,34 +57,52 @@ describe('getTransactionType', () => {
describe('with default transaction type', () => {
it('returns "request"', () => {
expect(
getTransactionType({
getOrRedirectToTransactionType({
transactionTypes: ['worker', 'request'],
agentName: 'nodejs',
history,
})
).toEqual('request');
expect(history.replace).toHaveBeenCalledWith(
expect.objectContaining({
search: 'transactionType=request',
})
);
});
});

describe('with no default transaction type', () => {
it('returns the first type', () => {
expect(
getTransactionType({
getOrRedirectToTransactionType({
transactionTypes: ['worker', 'custom'],
agentName: 'nodejs',
history,
})
).toEqual('worker');
expect(history.replace).toHaveBeenCalledWith(
expect.objectContaining({
search: 'transactionType=worker',
})
);
});
});
});

describe('with a rum agent', () => {
it('returns "page-load"', () => {
expect(
getTransactionType({
getOrRedirectToTransactionType({
transactionTypes: ['http-request', 'page-load'],
agentName: 'js-base',
history,
})
).toEqual('page-load');
expect(history.replace).toHaveBeenCalledWith(
expect.objectContaining({
search: 'transactionType=page-load',
})
);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
*/

import React, { createContext, ReactNode } from 'react';
import { useHistory } from 'react-router-dom';
import { History } from 'history';
import { isRumAgentName } from '../../../common/agent_name';
import {
TRANSACTION_PAGE_LOAD,
Expand All @@ -16,6 +18,7 @@ import { useServiceAgentFetcher } from './use_service_agent_fetcher';
import { useApmParams } from '../../hooks/use_apm_params';
import { useTimeRange } from '../../hooks/use_time_range';
import { useFallbackToTransactionsFetcher } from '../../hooks/use_fallback_to_transactions_fetcher';
import { replace } from '../../components/shared/links/url_helpers';

export interface APMServiceContextValue {
serviceName: string;
Expand All @@ -37,6 +40,8 @@ export function ApmServiceContextProvider({
}: {
children: ReactNode;
}) {
const history = useHistory();

const {
path: { serviceName },
query,
Expand All @@ -57,10 +62,11 @@ export function ApmServiceContextProvider({
end,
});

const transactionType = getTransactionType({
const transactionType = getOrRedirectToTransactionType({
transactionType: query.transactionType,
transactionTypes,
agentName,
history,
});

const { fallbackToTransactions } = useFallbackToTransactionsFetcher({
Expand All @@ -82,16 +88,18 @@ export function ApmServiceContextProvider({
);
}

export function getTransactionType({
export function getOrRedirectToTransactionType({
transactionType,
transactionTypes,
agentName,
history,
}: {
transactionType?: string;
transactionTypes: string[];
agentName?: string;
history: History;
}) {
if (transactionType) {
if (transactionType && transactionTypes.includes(transactionType)) {
return transactionType;
}

Expand All @@ -105,7 +113,13 @@ export function getTransactionType({
: TRANSACTION_REQUEST;

// If the default transaction type is not in transactionTypes the first in the list is returned
return transactionTypes.includes(defaultTransactionType)
const currentTransactionType = transactionTypes.includes(
defaultTransactionType
)
? defaultTransactionType
: transactionTypes[0];

// Replace transactionType in the URL in case it is not one of the types returned by the API
replace(history, { query: { transactionType: currentTransactionType } });
return currentTransactionType;
}

0 comments on commit b8b57d5

Please sign in to comment.