From 334379e82bf9fdcfd0d1e7fa41c798c2dd431828 Mon Sep 17 00:00:00 2001 From: Everett Ross Date: Mon, 2 Mar 2020 17:22:04 -0500 Subject: [PATCH 1/4] Add span linking to search results Signed-off-by: Everett Ross --- .../SearchTracePage/SearchResults/index.js | 10 +- .../src/components/SearchTracePage/index.js | 7 +- .../components/SearchTracePage/index.test.js | 1 + .../components/SearchTracePage/url.test.js | 133 ++++++++++++++++++ .../src/components/SearchTracePage/url.tsx | 48 ++++++- .../TracePage/url/ReferenceLink.test.js | 2 +- .../TracePage/url/ReferenceLink.tsx | 4 +- .../src/components/TracePage/url/index.tsx | 14 +- 8 files changed, 203 insertions(+), 16 deletions(-) create mode 100644 packages/jaeger-ui/src/components/SearchTracePage/url.test.js diff --git a/packages/jaeger-ui/src/components/SearchTracePage/SearchResults/index.js b/packages/jaeger-ui/src/components/SearchTracePage/SearchResults/index.js index 713307ea31..9f4bfd467f 100644 --- a/packages/jaeger-ui/src/components/SearchTracePage/SearchResults/index.js +++ b/packages/jaeger-ui/src/components/SearchTracePage/SearchResults/index.js @@ -56,6 +56,7 @@ type SearchResultsProps = { queryOfResults?: SearchQuery, showStandaloneLink: boolean, skipMessage?: boolean, + spanLinks?: Record | undefined | null, traces: TraceSummary[], }; @@ -91,7 +92,7 @@ export const sortFormSelector = formValueSelector('traceResultsSort'); export class UnconnectedSearchResults extends React.PureComponent { props: SearchResultsProps; - static defaultProps = { skipMessage: false, queryOfResults: undefined }; + static defaultProps = { skipMessage: false, spanLinks: undefined, queryOfResults: undefined }; toggleComparison = (traceID: string, remove: boolean) => { const { cohortAddTrace, cohortRemoveTrace } = this.props; @@ -123,6 +124,7 @@ export class UnconnectedSearchResults extends React.PureComponent 0; const showErrors = errors && !loadingTraces; @@ -132,6 +132,7 @@ export class SearchTracePageImpl extends Component { queryOfResults={queryOfResults} showStandaloneLink={Boolean(embedded)} skipMessage={isHomepage} + spanLinks={urlQueryParams && urlQueryParams.spanLinks} traces={traceResults} /> )} @@ -232,7 +233,7 @@ const stateServicesXformer = memoizeOne(stateServices => { // export to test export function mapStateToProps(state) { const { embedded, router, services: stServices, traceDiff } = state; - const query = queryString.parse(router.location.search); + const query = getUrlState(router.location.search); const isHomepage = !Object.keys(query).length; const { query: queryOfResults, traces, maxDuration, traceError, loadingTraces } = stateTraceXformer( state.trace diff --git a/packages/jaeger-ui/src/components/SearchTracePage/index.test.js b/packages/jaeger-ui/src/components/SearchTracePage/index.test.js index 95035816fa..1d52250f94 100644 --- a/packages/jaeger-ui/src/components/SearchTracePage/index.test.js +++ b/packages/jaeger-ui/src/components/SearchTracePage/index.test.js @@ -105,6 +105,7 @@ describe('', () => { expect(historyPush.mock.calls.length).toBe(1); expect(historyPush.mock.calls[0][0]).toEqual({ pathname: `/trace/${traceID}`, + search: undefined, state: { fromSearch: '/search?' }, }); }); diff --git a/packages/jaeger-ui/src/components/SearchTracePage/url.test.js b/packages/jaeger-ui/src/components/SearchTracePage/url.test.js new file mode 100644 index 0000000000..ccdd0fb68e --- /dev/null +++ b/packages/jaeger-ui/src/components/SearchTracePage/url.test.js @@ -0,0 +1,133 @@ +// Copyright (c) 2020 The Jaeger Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import { getUrl, getUrlState, isSameQuery } from './url'; + +describe('SearchTracePage/url', () => { + const span0 = 'span-0'; + const span1 = 'span-1'; + const span2 = 'span-2'; + const trace0 = 'trace-0'; + const trace1 = 'trace-1'; + const trace2 = 'trace-2'; + + describe('getUrl', () => { + it('handles no args given', () => { + expect(getUrl()).toBe('/search'); + }); + + it('handles empty args', () => { + expect(getUrl({})).toBe('/search?'); + }); + + it('includes provided args', () => { + const paramA = 'aParam'; + const paramB = 'bParam'; + expect(getUrl({ paramA, paramB })).toBe(`/search?paramA=${paramA}¶mB=${paramB}`); + }); + + it('converts spanLink and traceID to traceID and span', () => { + expect( + getUrl({ + traceID: [trace0], + spanLinks: { + [trace0]: span0, + }, + }) + ).toBe(`/search?span=${span0}%40${trace0}`); + }); + + it('handles missing traceID for spanLinks', () => { + expect( + getUrl({ + spanLinks: { + [trace0]: span0, + }, + }) + ).toBe(`/search?span=${span0}%40${trace0}`); + }); + + it('converts spanLink and other traceID to traceID and span', () => { + expect( + getUrl({ + traceID: [trace0, trace2], + spanLinks: { + [trace0]: span0, + }, + }) + ).toBe(`/search?span=${span0}%40${trace0}&traceID=${trace2}`); + }); + + it('converts spanLinks to traceID and span', () => { + expect( + getUrl({ + traceID: [trace0, trace1, trace2], + spanLinks: { + [trace0]: `${span0} ${span1}`, + [trace1]: span2, + }, + }) + ).toBe(`/search?span=${span0}%2C${span1}%40${trace0}&span=${span2}%40${trace1}&traceID=${trace2}`); + }); + }); + + describe('getUrlState', () => { + it('gets search params', () => { + const service = 'svc-0'; + const operation = 'op-0'; + expect(getUrlState(`service=${service}&operation=${operation}`)).toEqual({ service, operation }); + }); + + it('converts span to traceID and spanLinks', () => { + expect(getUrlState(`span=${span0}%40${trace0}`)).toEqual({ + traceID: [trace0], + spanLinks: { + [trace0]: span0, + }, + }); + }); + + it('converts multiple spans to traceID and spanLinks', () => { + expect( + getUrlState(`span=${span0}%2C${span1}%40${trace0}&span=${span2}%40${trace1}&traceID=${trace2}`) + ).toEqual({ + traceID: expect.arrayContaining([trace0, trace1, trace2]), + spanLinks: { + [trace0]: `${span0} ${span1}`, + [trace1]: span2, + }, + }); + }); + + it('handles duplicate traceIDs', () => { + expect( + getUrlState( + `span=${span0}%40${trace0}&span=${span1}%40${trace0}&span=${span2}%40${trace1}&traceID=${trace1}&traceID=${trace2}` + ) + ).toEqual({ + traceID: expect.arrayContaining([trace0, trace1, trace2]), + spanLinks: { + [trace0]: `${span0} ${span1}`, + [trace1]: span2, + }, + }); + }); + }); + + describe('isSameQuery', () => { + it('returns `false` if only one argument is falsy', () => { + expect(isSameQuery({})).toBe(false); + }); + }); +}); diff --git a/packages/jaeger-ui/src/components/SearchTracePage/url.tsx b/packages/jaeger-ui/src/components/SearchTracePage/url.tsx index 8f00cceadf..6128cff7c2 100644 --- a/packages/jaeger-ui/src/components/SearchTracePage/url.tsx +++ b/packages/jaeger-ui/src/components/SearchTracePage/url.tsx @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +import memoizeOne from 'memoize-one'; import queryString from 'query-string'; import { matchPath } from 'react-router-dom'; @@ -31,11 +32,52 @@ export function matches(path: string) { return Boolean(matchPath(path, ROUTE_MATCHER)); } -export function getUrl(query?: Record | null | undefined) { - const search = query ? `?${queryString.stringify(query)}` : ''; - return prefixUrl(`/search${search}`); +type TUrlState = Record> & { + traceID?: string | string[]; + spanLinks?: Record; +}; + +export function getUrl(query?: TUrlState) { + const searchUrl = prefixUrl(`/search`); + if (!query) return searchUrl; + + const { traceID, spanLinks, ...rest } = query; + const ids = traceID + ? (Array.isArray(traceID) ? traceID : [traceID]).filter((id: string) => !spanLinks || !spanLinks[id]) + : []; + const stringifyArg = { + ...rest, + span: + spanLinks && + Object.keys(spanLinks).reduce((res: string[], trace: string) => { + return [...res, `${spanLinks[trace].replace(/ /g, ',')}@${trace}`]; + }, []), + traceID: ids.length ? ids : undefined, + }; + return `${searchUrl}?${queryString.stringify(stringifyArg)}`; } +export const getUrlState: (search: string) => TUrlState = memoizeOne(function getUrlState( + search: string +): TUrlState { + const { traceID, span, ...rest } = queryString.parse(search); + const rv: TUrlState = { ...rest }; + const traceIDs = new Set(!traceID || Array.isArray(traceID) ? traceID : [traceID]); + const spanLinks: Record = {}; + if (span && span.length) { + (Array.isArray(span) ? span : [span]).forEach(s => { + const [spansStr, trace] = s.split('@'); + const uiFind = spansStr.replace(/,/g, ' '); + traceIDs.add(trace); + if (spanLinks[trace]) spanLinks[trace] = spanLinks[trace].concat(' ', uiFind); + else spanLinks[trace] = uiFind; + }); + rv.spanLinks = spanLinks; + } + if (traceIDs.size) rv.traceID = [...traceIDs]; + return rv; +}); + export function isSameQuery(a: SearchQuery, b: SearchQuery) { if (Boolean(a) !== Boolean(b)) { return false; diff --git a/packages/jaeger-ui/src/components/TracePage/url/ReferenceLink.test.js b/packages/jaeger-ui/src/components/TracePage/url/ReferenceLink.test.js index 64e9980ef2..ffc849414c 100644 --- a/packages/jaeger-ui/src/components/TracePage/url/ReferenceLink.test.js +++ b/packages/jaeger-ui/src/components/TracePage/url/ReferenceLink.test.js @@ -45,7 +45,7 @@ describe(ReferenceLink, () => { it('render for external trace', () => { const component = shallow(); - const link = component.find('a[href="/trace/trace2/uiFind?=span2"]'); + const link = component.find('a[href="/trace/trace2?uiFind=span2"]'); expect(link.length).toBe(1); }); }); diff --git a/packages/jaeger-ui/src/components/TracePage/url/ReferenceLink.tsx b/packages/jaeger-ui/src/components/TracePage/url/ReferenceLink.tsx index 93cb551f09..68fc8e6e06 100644 --- a/packages/jaeger-ui/src/components/TracePage/url/ReferenceLink.tsx +++ b/packages/jaeger-ui/src/components/TracePage/url/ReferenceLink.tsx @@ -24,8 +24,6 @@ type ReferenceLinkProps = { onClick?: () => void; }; -const linkToExternalSpan = (traceID: string, spanID: string) => `${getUrl(traceID)}/uiFind?=${spanID}`; - export default function ReferenceLink(props: ReferenceLinkProps) { const { reference, children, className, focusSpan, ...otherProps } = props; delete otherProps.onClick; @@ -38,7 +36,7 @@ export default function ReferenceLink(props: ReferenceLinkProps) { } return ( | TNil) { +export function getLocation(id: string, state: Record | TNil, uiFind?: string) { return { state, - pathname: prefixUrl(`/trace/${id}`), + pathname: getUrl(id), + search: uiFind && queryString.stringify({ uiFind }), }; } From 7307421f1137477d2c21654be10b08ee1b310d25 Mon Sep 17 00:00:00 2001 From: Everett Ross Date: Tue, 3 Mar 2020 11:40:06 -0500 Subject: [PATCH 2/4] Add more url tests Signed-off-by: Everett Ross --- .../components/SearchTracePage/url.test.js | 58 ++++++++++++++++++- .../src/components/SearchTracePage/url.tsx | 9 +-- .../components/TracePage/url/index.test.js | 51 ++++++++++++++++ 3 files changed, 112 insertions(+), 6 deletions(-) create mode 100644 packages/jaeger-ui/src/components/TracePage/url/index.test.js diff --git a/packages/jaeger-ui/src/components/SearchTracePage/url.test.js b/packages/jaeger-ui/src/components/SearchTracePage/url.test.js index ccdd0fb68e..f78da64288 100644 --- a/packages/jaeger-ui/src/components/SearchTracePage/url.test.js +++ b/packages/jaeger-ui/src/components/SearchTracePage/url.test.js @@ -37,10 +37,18 @@ describe('SearchTracePage/url', () => { expect(getUrl({ paramA, paramB })).toBe(`/search?paramA=${paramA}¶mB=${paramB}`); }); + it('preserves traceID without spanLinks', () => { + expect( + getUrl({ + traceID: trace0, + }) + ).toBe(`/search?traceID=${trace0}`); + }); + it('converts spanLink and traceID to traceID and span', () => { expect( getUrl({ - traceID: [trace0], + traceID: trace0, spanLinks: { [trace0]: span0, }, @@ -58,6 +66,14 @@ describe('SearchTracePage/url', () => { ).toBe(`/search?span=${span0}%40${trace0}`); }); + it('handles empty spanLinks', () => { + expect( + getUrl({ + spanLinks: {}, + }) + ).toBe(`/search?`); + }); + it('converts spanLink and other traceID to traceID and span', () => { expect( getUrl({ @@ -126,8 +142,46 @@ describe('SearchTracePage/url', () => { }); describe('isSameQuery', () => { + const queryKeys = [ + 'end', + 'limit', + 'lookback', + 'maxDuration', + 'minDuration', + 'operation', + 'service', + 'start', + 'tags', + ]; + const otherKey = 'other-key'; + const baseQuery = queryKeys.reduce( + (res, curr, i) => ({ + ...res, + [curr]: i % 2 ? curr : i, + }), + { [otherKey]: otherKey } + ); + it('returns `false` if only one argument is falsy', () => { - expect(isSameQuery({})).toBe(false); + expect(isSameQuery(baseQuery)).toBe(false); + }); + + it('returns `false` if a considered key is changed or omitted', () => { + queryKeys.forEach(key => { + // eslint-disable-next-line camelcase + const { [key]: _omitted, ...rest } = baseQuery; + expect(isSameQuery(baseQuery, rest)).toBe(false); + expect(isSameQuery(baseQuery, { ...rest, [key]: 'changed' })).toBe(false); + }); + }); + + it('returns `true` if no considered keys are changed or omitted', () => { + expect(isSameQuery(baseQuery, { ...baseQuery })).toBe(true); + + // eslint-disable-next-line camelcase + const { [otherKey]: _omitted, ...copy } = baseQuery; + expect(isSameQuery(baseQuery, copy)).toBe(true); + expect(isSameQuery(baseQuery, { ...copy, [otherKey]: 'changed' })).toBe(true); }); }); }); diff --git a/packages/jaeger-ui/src/components/SearchTracePage/url.tsx b/packages/jaeger-ui/src/components/SearchTracePage/url.tsx index 6128cff7c2..915a76a100 100644 --- a/packages/jaeger-ui/src/components/SearchTracePage/url.tsx +++ b/packages/jaeger-ui/src/components/SearchTracePage/url.tsx @@ -42,9 +42,10 @@ export function getUrl(query?: TUrlState) { if (!query) return searchUrl; const { traceID, spanLinks, ...rest } = query; - const ids = traceID - ? (Array.isArray(traceID) ? traceID : [traceID]).filter((id: string) => !spanLinks || !spanLinks[id]) - : []; + let ids = traceID; + if (spanLinks && traceID) { + ids = (Array.isArray(traceID) ? traceID : [traceID]).filter((id: string) => !spanLinks[id]); + } const stringifyArg = { ...rest, span: @@ -52,7 +53,7 @@ export function getUrl(query?: TUrlState) { Object.keys(spanLinks).reduce((res: string[], trace: string) => { return [...res, `${spanLinks[trace].replace(/ /g, ',')}@${trace}`]; }, []), - traceID: ids.length ? ids : undefined, + traceID: ids && ids.length ? ids : undefined, }; return `${searchUrl}?${queryString.stringify(stringifyArg)}`; } diff --git a/packages/jaeger-ui/src/components/TracePage/url/index.test.js b/packages/jaeger-ui/src/components/TracePage/url/index.test.js new file mode 100644 index 0000000000..ad26337175 --- /dev/null +++ b/packages/jaeger-ui/src/components/TracePage/url/index.test.js @@ -0,0 +1,51 @@ +// Copyright (c) 2020 The Jaeger Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import { getLocation, getUrl } from '.'; + +describe('TracePage/url', () => { + const traceID = 'trace-id'; + const uiFind = 'ui-find'; + + describe('getUrl', () => { + it('includes traceID without uiFind', () => { + expect(getUrl(traceID)).toBe(`/trace/${traceID}`); + }); + + it('includes traceID and uiFind', () => { + expect(getUrl(traceID, uiFind)).toBe(`/trace/${traceID}?uiFind=${uiFind}`); + }); + }); + + describe('getLocation', () => { + const state = { + from: 'some-url', + }; + + it('passes provided state with correct pathname, without uiFind', () => { + expect(getLocation(traceID, state)).toEqual({ + state, + pathname: getUrl(traceID), + }); + }); + + it('passes provided state with correct pathname with uiFind', () => { + expect(getLocation(traceID, state, uiFind)).toEqual({ + state, + pathname: getUrl(traceID), + search: `uiFind=${uiFind}`, + }); + }); + }); +}); From a8ad2b774154f7543cbba14880fb65a05aec9e29 Mon Sep 17 00:00:00 2001 From: Everett Ross Date: Tue, 3 Mar 2020 13:48:11 -0500 Subject: [PATCH 3/4] Change span csv to space seperated value Signed-off-by: Everett Ross --- .../jaeger-ui/src/components/SearchTracePage/url.test.js | 4 ++-- packages/jaeger-ui/src/components/SearchTracePage/url.tsx | 7 +++---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/jaeger-ui/src/components/SearchTracePage/url.test.js b/packages/jaeger-ui/src/components/SearchTracePage/url.test.js index f78da64288..36c9a5b834 100644 --- a/packages/jaeger-ui/src/components/SearchTracePage/url.test.js +++ b/packages/jaeger-ui/src/components/SearchTracePage/url.test.js @@ -94,7 +94,7 @@ describe('SearchTracePage/url', () => { [trace1]: span2, }, }) - ).toBe(`/search?span=${span0}%2C${span1}%40${trace0}&span=${span2}%40${trace1}&traceID=${trace2}`); + ).toBe(`/search?span=${span0}%20${span1}%40${trace0}&span=${span2}%40${trace1}&traceID=${trace2}`); }); }); @@ -116,7 +116,7 @@ describe('SearchTracePage/url', () => { it('converts multiple spans to traceID and spanLinks', () => { expect( - getUrlState(`span=${span0}%2C${span1}%40${trace0}&span=${span2}%40${trace1}&traceID=${trace2}`) + getUrlState(`span=${span0}%20${span1}%40${trace0}&span=${span2}%40${trace1}&traceID=${trace2}`) ).toEqual({ traceID: expect.arrayContaining([trace0, trace1, trace2]), spanLinks: { diff --git a/packages/jaeger-ui/src/components/SearchTracePage/url.tsx b/packages/jaeger-ui/src/components/SearchTracePage/url.tsx index 915a76a100..a7b2b91bfe 100644 --- a/packages/jaeger-ui/src/components/SearchTracePage/url.tsx +++ b/packages/jaeger-ui/src/components/SearchTracePage/url.tsx @@ -51,7 +51,7 @@ export function getUrl(query?: TUrlState) { span: spanLinks && Object.keys(spanLinks).reduce((res: string[], trace: string) => { - return [...res, `${spanLinks[trace].replace(/ /g, ',')}@${trace}`]; + return [...res, `${spanLinks[trace]}@${trace}`]; }, []), traceID: ids && ids.length ? ids : undefined, }; @@ -68,10 +68,9 @@ export const getUrlState: (search: string) => TUrlState = memoizeOne(function ge if (span && span.length) { (Array.isArray(span) ? span : [span]).forEach(s => { const [spansStr, trace] = s.split('@'); - const uiFind = spansStr.replace(/,/g, ' '); traceIDs.add(trace); - if (spanLinks[trace]) spanLinks[trace] = spanLinks[trace].concat(' ', uiFind); - else spanLinks[trace] = uiFind; + if (spanLinks[trace]) spanLinks[trace] = spanLinks[trace].concat(' ', spansStr); + else spanLinks[trace] = spansStr; }); rv.spanLinks = spanLinks; } From 24f68a442c17451cfd018ceb5d362d29de6cc372 Mon Sep 17 00:00:00 2001 From: Everett Ross Date: Tue, 3 Mar 2020 18:19:25 -0500 Subject: [PATCH 4/4] Handle span param without spanIDs Signed-off-by: Everett Ross --- .../src/components/SearchTracePage/url.test.js | 11 +++++++++++ .../jaeger-ui/src/components/SearchTracePage/url.tsx | 6 ++++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/packages/jaeger-ui/src/components/SearchTracePage/url.test.js b/packages/jaeger-ui/src/components/SearchTracePage/url.test.js index 36c9a5b834..0710255c2e 100644 --- a/packages/jaeger-ui/src/components/SearchTracePage/url.test.js +++ b/packages/jaeger-ui/src/components/SearchTracePage/url.test.js @@ -126,6 +126,17 @@ describe('SearchTracePage/url', () => { }); }); + it('converts span param without spanIDs to just traceID', () => { + expect(getUrlState(`span=${span0}%20${span1}%40${trace0}&span=%40${trace1}&traceID=${trace2}`)).toEqual( + { + traceID: expect.arrayContaining([trace0, trace1, trace2]), + spanLinks: { + [trace0]: `${span0} ${span1}`, + }, + } + ); + }); + it('handles duplicate traceIDs', () => { expect( getUrlState( diff --git a/packages/jaeger-ui/src/components/SearchTracePage/url.tsx b/packages/jaeger-ui/src/components/SearchTracePage/url.tsx index a7b2b91bfe..f1567d2b48 100644 --- a/packages/jaeger-ui/src/components/SearchTracePage/url.tsx +++ b/packages/jaeger-ui/src/components/SearchTracePage/url.tsx @@ -69,8 +69,10 @@ export const getUrlState: (search: string) => TUrlState = memoizeOne(function ge (Array.isArray(span) ? span : [span]).forEach(s => { const [spansStr, trace] = s.split('@'); traceIDs.add(trace); - if (spanLinks[trace]) spanLinks[trace] = spanLinks[trace].concat(' ', spansStr); - else spanLinks[trace] = spansStr; + if (spansStr) { + if (spanLinks[trace]) spanLinks[trace] = spanLinks[trace].concat(' ', spansStr); + else spanLinks[trace] = spansStr; + } }); rv.spanLinks = spanLinks; }