Skip to content

Commit

Permalink
Move remaining uses of serviceName away from urlParams (#77248)
Browse files Browse the repository at this point in the history
* Move remaining uses of serviceName away from urlParams

There were a few of these that were either missed or lost in merge conflict resolution.

I went through everything that's used as a path parameter and made sure it wasn't being used anywhere with `urlParams`.

Previously none of the charts were working, now they all are.

Looks like #67791 introduced a find/replace change that broke APM's e2e tests. This reverts that change.
  • Loading branch information
smith authored Sep 15, 2020
1 parent 5a2f7ae commit 273806b
Show file tree
Hide file tree
Showing 15 changed files with 45 additions and 39 deletions.
2 changes: 1 addition & 1 deletion x-pack/plugins/apm/e2e/cypress/integration/apm.feature
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
KibanaFeature: APM
Feature: APM

Scenario: Transaction duration charts
Given a user browses the APM UI application
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export const DEFAULT_TIMEOUT = 60 * 1000;

Given(`a user browses the APM UI application`, () => {
// open service overview page
loginAndWaitForPage(`/app/apm#/services`, {
loginAndWaitForPage(`/app/apm/services`, {
from: '2020-06-01T14:59:32.686Z',
to: '2020-06-16T16:59:36.219Z',
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ Given(`a user browses the APM UI application for RUM Data`, () => {
// open service overview page
const RANGE_FROM = 'now-24h';
const RANGE_TO = 'now';
loginAndWaitForPage(`/app/apm#/rum-preview`, {
loginAndWaitForPage(`/app/apm/rum-preview`, {
from: RANGE_FROM,
to: RANGE_TO,
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,11 @@ const CoreVitalsThresholds = {
export function CoreVitals() {
const { urlParams, uiFilters } = useUrlParams();

const { start, end, serviceName } = urlParams;
const { start, end } = urlParams;

const { data, status } = useFetcher(
(callApmApi) => {
const { serviceName } = uiFilters;
if (start && end && serviceName) {
return callApmApi({
pathname: '/api/apm/rum-client/web-core-vitals',
Expand All @@ -34,7 +35,7 @@ export function CoreVitals() {
}
return Promise.resolve(null);
},
[start, end, serviceName, uiFilters]
[start, end, uiFilters]
);

const { lcp, lcpRanks, fid, fidRanks, cls, clsRanks } = data || {};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,7 @@ export function Buttons({
}: ButtonsProps) {
const { core } = useApmPluginContext();
const { basePath } = core.http;
// The params may contain the service name. We want to use the selected node's
// service name in the button URLs, so make a copy and set the
// `serviceName` property.
const urlParams = { ...useUrlParams().urlParams } as APMQueryParams;
urlParams.serviceName = selectedNodeServiceName;
const urlParams = useUrlParams().urlParams as APMQueryParams;

const detailsUrl = getAPMHref({
basePath,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,9 @@
import { EuiSuperDatePicker } from '@elastic/eui';
import { isEmpty, isEqual, pickBy } from 'lodash';
import React from 'react';
import { useHistory } from 'react-router-dom';
import { useHistory, useLocation } from 'react-router-dom';
import { UI_SETTINGS } from '../../../../../../../src/plugins/data/common';
import { useApmPluginContext } from '../../../hooks/useApmPluginContext';
import { useLocation } from '../../../hooks/useLocation';
import { useUrlParams } from '../../../hooks/useUrlParams';
import { clearCache } from '../../../services/rest/callApi';
import { fromQuery, toQuery } from '../Links/url_helpers';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,11 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import { EuiTitle } from '@elastic/eui';
import { EuiPanel, EuiSpacer, EuiTitle } from '@elastic/eui';
import theme from '@elastic/eui/dist/eui_theme_light.json';
import { i18n } from '@kbn/i18n';
import React, { useCallback } from 'react';
import { EuiPanel } from '@elastic/eui';
import { EuiSpacer } from '@elastic/eui';
import { useParams } from 'react-router-dom';
import { asPercent } from '../../../../../common/utils/formatters';
import { useChartsSync } from '../../../../hooks/useChartsSync';
import { useFetcher } from '../../../../hooks/useFetcher';
Expand All @@ -22,16 +21,11 @@ const tickFormatY = (y?: number) => {
};

export function ErroneousTransactionsRateChart() {
const { serviceName } = useParams<{ serviceName?: string }>();
const { urlParams, uiFilters } = useUrlParams();
const syncedChartsProps = useChartsSync();

const {
serviceName,
start,
end,
transactionType,
transactionName,
} = urlParams;
const { start, end, transactionType, transactionName } = urlParams;

const { data } = useFetcher(() => {
if (serviceName && start && end) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@ import { renderHook } from '@testing-library/react-hooks';
import theme from '@elastic/eui/dist/eui_theme_light.json';
import * as useFetcherModule from './useFetcher';
import { useAvgDurationByBrowser } from './useAvgDurationByBrowser';
import React, { ReactNode } from 'react';
import { MemoryRouter } from 'react-router-dom';

function Wrapper({ children }: { children?: ReactNode }) {
return <MemoryRouter>{children}</MemoryRouter>;
}

describe('useAvgDurationByBrowser', () => {
it('returns data', () => {
Expand All @@ -19,7 +25,9 @@ describe('useAvgDurationByBrowser', () => {
refetch: () => {},
status: 'success' as useFetcherModule.FETCH_STATUS,
});
const { result } = renderHook(() => useAvgDurationByBrowser());
const { result } = renderHook(() => useAvgDurationByBrowser(), {
wrapper: Wrapper,
});

expect(result.current.data).toEqual([
{
Expand Down
10 changes: 6 additions & 4 deletions x-pack/plugins/apm/public/hooks/useAvgDurationByBrowser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,13 @@
*/

import theme from '@elastic/eui/dist/eui_theme_light.json';
import { useFetcher } from './useFetcher';
import { useUrlParams } from './useUrlParams';
import { useParams } from 'react-router-dom';
import { getVizColorForIndex } from '../../common/viz_colors';
// eslint-disable-next-line @kbn/eslint/no-restricted-paths
import { AvgDurationByBrowserAPIResponse } from '../../server/lib/transactions/avg_duration_by_browser';
import { TimeSeries } from '../../typings/timeseries';
import { getVizColorForIndex } from '../../common/viz_colors';
import { useFetcher } from './useFetcher';
import { useUrlParams } from './useUrlParams';

function toTimeSeries(data?: AvgDurationByBrowserAPIResponse): TimeSeries[] {
if (!data) {
Expand All @@ -27,8 +28,9 @@ function toTimeSeries(data?: AvgDurationByBrowserAPIResponse): TimeSeries[] {
}

export function useAvgDurationByBrowser() {
const { serviceName } = useParams<{ serviceName?: string }>();
const {
urlParams: { serviceName, start, end, transactionName },
urlParams: { start, end, transactionName },
uiFilters,
} = useUrlParams();

Expand Down
5 changes: 3 additions & 2 deletions x-pack/plugins/apm/public/hooks/useAvgDurationByCountry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,14 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { useParams } from 'react-router-dom';
import { useFetcher } from './useFetcher';
import { useUrlParams } from './useUrlParams';

export function useAvgDurationByCountry() {
const { serviceName } = useParams<{ serviceName?: string }>();
const {
urlParams: { serviceName, start, end, transactionName },
urlParams: { start, end, transactionName },
uiFilters,
} = useUrlParams();

Expand Down
4 changes: 3 additions & 1 deletion x-pack/plugins/apm/public/hooks/useTransactionBreakdown.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { useParams } from 'react-router-dom';
import { useFetcher } from './useFetcher';
import { useUrlParams } from './useUrlParams';

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

Expand Down
4 changes: 3 additions & 1 deletion x-pack/plugins/apm/public/hooks/useTransactionCharts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,15 @@
*/

import { useMemo } from 'react';
import { useParams } from 'react-router-dom';
import { getTransactionCharts } from '../selectors/chartSelectors';
import { useFetcher } from './useFetcher';
import { useUrlParams } from './useUrlParams';

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

Expand Down
9 changes: 5 additions & 4 deletions x-pack/plugins/apm/public/hooks/useTransactionDistribution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { IUrlParams } from '../context/UrlParamsContext/types';
import { useFetcher } from './useFetcher';
import { useUiFilters } from '../context/UrlParamsContext';
import { useParams } from 'react-router-dom';
// eslint-disable-next-line @kbn/eslint/no-restricted-paths
import { TransactionDistributionAPIResponse } from '../../server/lib/transactions/distribution';
import { useUiFilters } from '../context/UrlParamsContext';
import { IUrlParams } from '../context/UrlParamsContext/types';
import { useFetcher } from './useFetcher';

const INITIAL_DATA = {
buckets: [] as TransactionDistributionAPIResponse['buckets'],
Expand All @@ -17,8 +18,8 @@ const INITIAL_DATA = {
};

export function useTransactionDistribution(urlParams: IUrlParams) {
const { serviceName } = useParams<{ serviceName?: string }>();
const {
serviceName,
start,
end,
transactionType,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ describe('Observability dashboard data', () => {
);
const response = await fetchOverviewPageData(params);
expect(response).toEqual({
appLink: '/app/apm#/services?rangeFrom=now-15m&rangeTo=now',
appLink: '/app/apm/services?rangeFrom=now-15m&rangeTo=now',
stats: {
services: {
type: 'number',
Expand Down Expand Up @@ -82,7 +82,7 @@ describe('Observability dashboard data', () => {
);
const response = await fetchOverviewPageData(params);
expect(response).toEqual({
appLink: '/app/apm#/services?rangeFrom=now-15m&rangeTo=now',
appLink: '/app/apm/services?rangeFrom=now-15m&rangeTo=now',
stats: {
services: {
type: 'number',
Expand All @@ -109,7 +109,7 @@ describe('Observability dashboard data', () => {
);
const response = await fetchOverviewPageData(params);
expect(response).toEqual({
appLink: '/app/apm#/services?rangeFrom=now-15m&rangeTo=now',
appLink: '/app/apm/services?rangeFrom=now-15m&rangeTo=now',
stats: {
services: {
type: 'number',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export const fetchOverviewPageData = async ({
const { serviceCount, transactionCoordinates } = data;

return {
appLink: `/app/apm#/services?rangeFrom=${relativeTime.start}&rangeTo=${relativeTime.end}`,
appLink: `/app/apm/services?rangeFrom=${relativeTime.start}&rangeTo=${relativeTime.end}`,
stats: {
services: {
type: 'number',
Expand Down

0 comments on commit 273806b

Please sign in to comment.