From 4a217f35d0d41681cfa530592ca36f152a617bc0 Mon Sep 17 00:00:00 2001 From: Everett Ross Date: Mon, 30 Sep 2019 17:00:59 -0400 Subject: [PATCH 1/3] Add and consume hash to withhold visEncoding Signed-off-by: Everett Ross --- packages/jaeger-ui/package.json | 2 + .../DeepDependencies/Header/index.css | 1 - .../components/DeepDependencies/index.test.js | 46 +++++++++---- .../src/components/DeepDependencies/index.tsx | 15 +++-- .../components/DeepDependencies/url.test.js | 66 ++++++++++++++----- .../src/components/DeepDependencies/url.tsx | 20 +++++- .../src/model/ddg/GraphModel/index.test.js | 49 ++++++++++++-- .../src/model/ddg/GraphModel/index.tsx | 10 +++ packages/jaeger-ui/src/model/ddg/types.tsx | 1 + yarn.lock | 18 +++-- 10 files changed, 178 insertions(+), 50 deletions(-) diff --git a/packages/jaeger-ui/package.json b/packages/jaeger-ui/package.json index 470fb8d95c..da1759c3ac 100644 --- a/packages/jaeger-ui/package.json +++ b/packages/jaeger-ui/package.json @@ -42,6 +42,7 @@ "@types/lodash": "^4.14.123", "@types/memoize-one": "4.1.1", "@types/moment": "^2.13.0", + "@types/object-hash": "^1.3.0", "@types/react-copy-to-clipboard": "^4.2.6", "@types/react-icons": "2.2.7", "@types/react-redux": "^5.0.6", @@ -72,6 +73,7 @@ "match-sorter": "^3.1.1", "memoize-one": "^5.0.0", "moment": "^2.18.1", + "object-hash": "^1.3.1", "prop-types": "^15.5.10", "query-string": "^6.3.0", "raven-js": "^3.22.1", diff --git a/packages/jaeger-ui/src/components/DeepDependencies/Header/index.css b/packages/jaeger-ui/src/components/DeepDependencies/Header/index.css index 19f50a1d90..f9d53274f0 100644 --- a/packages/jaeger-ui/src/components/DeepDependencies/Header/index.css +++ b/packages/jaeger-ui/src/components/DeepDependencies/Header/index.css @@ -21,7 +21,6 @@ limitations under the License. box-shadow: 0 1px 3px 0 rgba(0, 0, 0, 0.1); display: flex; flex-direction: column; - flex: 1 1; position: relative; z-index: 10; } diff --git a/packages/jaeger-ui/src/components/DeepDependencies/index.test.js b/packages/jaeger-ui/src/components/DeepDependencies/index.test.js index 1de8724a93..878ccf572d 100644 --- a/packages/jaeger-ui/src/components/DeepDependencies/index.test.js +++ b/packages/jaeger-ui/src/components/DeepDependencies/index.test.js @@ -135,6 +135,23 @@ describe('DeepDependencyGraphPage', () => { expect(props.history.push).toHaveBeenCalledTimes(1); }); + it('includes props.graph.hash iff it is truthy', () => { + ddgPageImpl.updateUrlState({}); + expect(getUrlSpy).not.toHaveBeenLastCalledWith(expect.objectContaining({ hash: expect.anything() })); + + const hash = 'testHash'; + const propsWithHash = { + ...props, + graph: { + ...props.graph, + hash, + }, + }; + const ddgPageWithHash = new DeepDependencyGraphPageImpl(propsWithHash); + ddgPageWithHash.updateUrlState({}); + expect(getUrlSpy).toHaveBeenLastCalledWith(expect.objectContaining({ hash })); + }); + describe('setDistance', () => { const mockNewEncoding = '1'; let encodeDistanceSpy; @@ -351,12 +368,22 @@ describe('DeepDependencyGraphPage', () => { }, }; const ownProps = { location: { search } }; - const mockGraphModel = { getVisible: () => ({}) }; + const mockGraph = { hash: 'testHash', getVisible: () => ({}) }; + const doneState = _set( + { ...state }, + ['ddg', getStateEntryKey({ service, operation, start: 0, end: 0 })], + { + model: {}, + state: fetchedState.DONE, + } + ); let getUrlStateSpy; + let sanitizeUrlStateSpy; let makeGraphSpy; beforeAll(() => { getUrlStateSpy = jest.spyOn(url, 'getUrlState'); + sanitizeUrlStateSpy = jest.spyOn(url, 'sanitizeUrlState'); makeGraphSpy = jest.spyOn(GraphModel, 'makeGraph'); }); @@ -364,7 +391,7 @@ describe('DeepDependencyGraphPage', () => { getUrlStateSpy.mockReset(); getUrlStateSpy.mockReturnValue(expected.urlState); makeGraphSpy.mockReset(); - makeGraphSpy.mockReturnValue(mockGraphModel); + makeGraphSpy.mockReturnValue(mockGraph); }); it('uses gets relevant params from location.search', () => { @@ -404,16 +431,8 @@ describe('DeepDependencyGraphPage', () => { const result = mapStateToProps(reduxState, ownProps); expect(result.graph).toBe(undefined); - const doneState = _set( - { ...state }, - ['ddg', getStateEntryKey({ service, operation, start: 0, end: 0 })], - { - model: {}, - state: fetchedState.DONE, - } - ); const doneResult = mapStateToProps(doneState, ownProps); - expect(doneResult.graph).toBe(mockGraphModel); + expect(doneResult.graph).toBe(mockGraph); }); it('includes services and operationsForService', () => { @@ -421,5 +440,10 @@ describe('DeepDependencyGraphPage', () => { expect.objectContaining({ operationsForService, services }) ); }); + + it('sanitizes urlState', () => { + mapStateToProps(doneState, ownProps); + expect(sanitizeUrlStateSpy).toHaveBeenLastCalledWith(expected.urlState, mockGraph.hash); + }); }); }); diff --git a/packages/jaeger-ui/src/components/DeepDependencies/index.tsx b/packages/jaeger-ui/src/components/DeepDependencies/index.tsx index 5d638fdab2..f26411bbb1 100644 --- a/packages/jaeger-ui/src/components/DeepDependencies/index.tsx +++ b/packages/jaeger-ui/src/components/DeepDependencies/index.tsx @@ -20,7 +20,7 @@ import { connect } from 'react-redux'; import Header from './Header'; import Graph from './Graph'; -import { getUrl, getUrlState } from './url'; +import { getUrl, getUrlState, sanitizeUrlState } from './url'; import ErrorMessage from '../common/ErrorMessage'; import LoadingIndicator from '../common/LoadingIndicator'; import { extractUiFindFromState, TExtractUiFindFromStateReturn } from '../common/UiFindInput'; @@ -79,8 +79,6 @@ export class DeepDependencyGraphPageImpl extends React.PureComponent { } } - headerWrapper: React.RefObject = React.createRef(); - constructor(props: TProps) { super(props); DeepDependencyGraphPageImpl.fetchModelIfStale(props); @@ -173,8 +171,11 @@ export class DeepDependencyGraphPageImpl extends React.PureComponent { toggleShowOperations = (enable: boolean) => this.updateUrlState({ showOp: enable }); updateUrlState = (newValues: Partial) => { - const { uiFind, urlState, history } = this.props; - history.push(getUrl({ uiFind, ...urlState, ...newValues })); + const { graph, history, uiFind, urlState } = this.props; + const getUrlArg = { uiFind, ...urlState, ...newValues }; + const hash = graph && graph.hash; + if (hash) getUrlArg.hash = hash; + history.push(getUrl(getUrlArg)); }; render() { @@ -225,7 +226,7 @@ export class DeepDependencyGraphPageImpl extends React.PureComponent { return (
-
+
{ describe('matches', () => { @@ -65,9 +65,9 @@ describe('DeepDependencyGraph/url', () => { }); describe('getUrlState', () => { - const search = 'test search'; const density = 'test density'; const end = '900'; + const hash = 'test hash'; const operation = 'operationName'; const service = 'serviceName'; const showOp = '0'; @@ -76,6 +76,7 @@ describe('DeepDependencyGraph/url', () => { const acceptableParams = { density, end, + hash, operation, service, showOp, @@ -85,12 +86,15 @@ describe('DeepDependencyGraph/url', () => { const expectedParams = { density, end: Number.parseInt(end, 10), + hash, operation, service, showOp: Boolean(+showOp), start: Number.parseInt(start, 10), visEncoding, }; + let count = 0; + const getSearch = () => `test search ${count++}`; let warnSpy; let parseSpy; @@ -109,18 +113,18 @@ describe('DeepDependencyGraph/url', () => { }); it('gets all values from queryString', () => { + const search = getSearch(); parseSpy.mockReturnValue(acceptableParams); expect(getUrlState(search)).toEqual(expectedParams); expect(parseSpy).toHaveBeenCalledWith(search); }); it('handles absent values', () => { - ['end', 'operation', 'service', 'start', 'visEncoding'].forEach(param => { + ['end', 'hash', 'operation', 'service', 'start', 'visEncoding'].forEach(param => { const { [param]: unused, ...rest } = expectedParams; const { [param]: alsoUnused, ...rv } = acceptableParams; parseSpy.mockReturnValue(rv); - expect(getUrlState(search)).toEqual(rest); - expect(parseSpy).toHaveBeenLastCalledWith(search); + expect(getUrlState(getSearch())).toEqual(rest); }); }); @@ -128,16 +132,14 @@ describe('DeepDependencyGraph/url', () => { const { showOp: unused, ...rest } = expectedParams; const { showOp: alsoUnused, ...rv } = acceptableParams; parseSpy.mockReturnValue(rv); - expect(getUrlState(search)).toEqual({ ...rest, showOp: true }); - expect(parseSpy).toHaveBeenLastCalledWith(search); + expect(getUrlState(getSearch())).toEqual({ ...rest, showOp: true }); }); it("defaults `density` to 'ppe'", () => { const { density: unused, ...rest } = expectedParams; const { density: alsoUnused, ...rv } = acceptableParams; parseSpy.mockReturnValue(rv); - expect(getUrlState(search)).toEqual({ ...rest, density: 'ppe' }); - expect(parseSpy).toHaveBeenLastCalledWith(search); + expect(getUrlState(getSearch())).toEqual({ ...rest, density: 'ppe' }); }); it('ignores extraneous query parameters', () => { @@ -148,31 +150,59 @@ describe('DeepDependencyGraph/url', () => { ...extraneous, ...acceptableParams, }); - expect(getUrlState(search)).toEqual(expect.not.objectContaining(extraneous)); - expect(parseSpy).toHaveBeenCalledWith(search); + expect(getUrlState(getSearch())).toEqual(expect.not.objectContaining(extraneous)); }); it('omits falsy values', () => { - ['end', 'operation', 'service', 'start', 'visEncoding'].forEach(param => { + ['end', 'hash', 'operation', 'service', 'start', 'visEncoding'].forEach(param => { [null, undefined, ''].forEach(falsyPossibility => { parseSpy.mockReturnValue({ ...expectedParams, [param]: falsyPossibility }); - expect(Reflect.has(getUrlState(search), param)).toBe(false); - expect(parseSpy).toHaveBeenLastCalledWith(search); + expect(Reflect.has(getUrlState(getSearch()), param)).toBe(false); }); }); }); it('handles and warns on duplicate values', () => { - ['end', 'operation', 'service', 'showOp', 'start', 'visEncoding'].forEach(param => { + ['end', 'hash', 'operation', 'service', 'showOp', 'start', 'visEncoding'].forEach(param => { const secondParam = `second ${acceptableParams[param]}`; parseSpy.mockReturnValue({ ...acceptableParams, [param]: [acceptableParams[param], secondParam], }); - expect(getUrlState(search)[param]).toBe(expectedParams[param]); - expect(warnSpy).toHaveBeenLastCalledWith(expect.stringContaining(secondParam)); - expect(parseSpy).toHaveBeenLastCalledWith(search); + expect(getUrlState(getSearch())[param]).toBe(expectedParams[param]); }); }); + + it('memoizes correctly', () => { + const search = getSearch(); + parseSpy.mockReturnValue(acceptableParams); + expect(getUrlState(search)).toBe(getUrlState(search)); + expect(getUrlState(getSearch())).not.toBe(getUrlState(search)); + }); + }); + + describe('sanitizeUrlState', () => { + const hash = 'test hash'; + const urlStateWithoutVisEncoding = { + hash, + operation: 'test operation', + service: 'test service', + }; + const urlState = { + ...urlStateWithoutVisEncoding, + visEncoding: 'test visEncoding', + }; + + it('returns provided state without visEncoding if hash was not provided', () => { + expect(sanitizeUrlState(urlState)).toEqual(urlStateWithoutVisEncoding); + }); + + it('returns provided state without visEncoding if provided hash does not match urlState hash', () => { + expect(sanitizeUrlState(urlState, `not ${hash}`)).toEqual(urlStateWithoutVisEncoding); + }); + + it('returns provided state visEncoding if provided hash does matches urlState hash', () => { + expect(sanitizeUrlState(urlState, hash)).toBe(urlState); + }); }); }); diff --git a/packages/jaeger-ui/src/components/DeepDependencies/url.tsx b/packages/jaeger-ui/src/components/DeepDependencies/url.tsx index 74e630da31..9f37dda2b8 100644 --- a/packages/jaeger-ui/src/components/DeepDependencies/url.tsx +++ b/packages/jaeger-ui/src/components/DeepDependencies/url.tsx @@ -13,6 +13,7 @@ // limitations under the License. import _isEmpty from 'lodash/isEmpty'; +import memoizeOne from 'memoize-one'; import queryString from 'query-string'; import { matchPath } from 'react-router-dom'; @@ -49,10 +50,11 @@ function firstParam(arg: string | string[]): string { return arg; } -export function getUrlState(search: string): TDdgSparseUrlState { +export const getUrlState = memoizeOne(function getUrlState(search: string): TDdgSparseUrlState { const { density = EDdgDensity.PreventPathEntanglement, end, + hash, operation, service, showOp = '1', @@ -66,6 +68,9 @@ export function getUrlState(search: string): TDdgSparseUrlState { if (end) { rv.end = Number.parseInt(firstParam(end), 10); } + if (hash) { + rv.hash = firstParam(hash); + } if (operation) { rv.operation = firstParam(operation); } @@ -79,4 +84,15 @@ export function getUrlState(search: string): TDdgSparseUrlState { rv.visEncoding = firstParam(visEncoding); } return rv; -} +}); + +export const sanitizeUrlState = memoizeOne(function sanitizeUrlStateImpl( + state: TDdgSparseUrlState, + hash?: string +): TDdgSparseUrlState { + if (hash && state.hash === hash) { + return state; + } + const { visEncoding, ...sanitized } = state; // eslint-disable-line @typescript-eslint/no-unused-vars + return sanitized; +}); diff --git a/packages/jaeger-ui/src/model/ddg/GraphModel/index.test.js b/packages/jaeger-ui/src/model/ddg/GraphModel/index.test.js index 4711d43b92..c3b3332ed2 100644 --- a/packages/jaeger-ui/src/model/ddg/GraphModel/index.test.js +++ b/packages/jaeger-ui/src/model/ddg/GraphModel/index.test.js @@ -69,12 +69,13 @@ describe('GraphModel', () => { } describe('constructor', () => { + const testGraph = new GraphModel({ + ddgModel: simpleModel, + density: EDdgDensity.PreventPathEntanglement, + showOp: true, + }); + it('creates five vertices and four edges for one-path ddg', () => { - const testGraph = new GraphModel({ - ddgModel: simpleModel, - density: EDdgDensity.PreventPathEntanglement, - showOp: true, - }); validateGraph(testGraph, [ { visIndices: [0], @@ -97,6 +98,44 @@ describe('GraphModel', () => { }, ]); }); + + describe('hash', () => { + it('creates equal hashes if model data is equivalent', () => { + const simpleModelEquivalent = transformDdgData( + [simplePath].map((path, i) => ({ path, trace_id: `trace${i}` })), + focalPayloadElem + ); + const testGraphEquivalent = new GraphModel({ + ddgModel: simpleModelEquivalent, + density: EDdgDensity.PreventPathEntanglement, + showOp: true, + }); + + expect(simpleModelEquivalent).not.toEqual(simpleModel); + expect(testGraphEquivalent).not.toBe(testGraph); + expect(testGraphEquivalent.hash).toBe(testGraph.hash); + }); + + it('creates different hashes if model data is different', () => { + const diffModelGraph = new GraphModel({ + ddgModel: doubleFocalModel, + density: EDdgDensity.PreventPathEntanglement, + showOp: true, + }); + + expect(diffModelGraph.hash).not.toBe(testGraph.hash); + }); + + it('creates different hashes if density configurations are different', () => { + const diffDensityGraph = new GraphModel({ + ddgModel: simpleModel, + density: EDdgDensity.UpstreamVsDownstream, + showOp: false, + }); + + expect(diffDensityGraph.hash).not.toBe(testGraph.hash); + }); + }); }); describe('convergent paths', () => { diff --git a/packages/jaeger-ui/src/model/ddg/GraphModel/index.tsx b/packages/jaeger-ui/src/model/ddg/GraphModel/index.tsx index 201b34f45e..317f0b7882 100644 --- a/packages/jaeger-ui/src/model/ddg/GraphModel/index.tsx +++ b/packages/jaeger-ui/src/model/ddg/GraphModel/index.tsx @@ -13,6 +13,7 @@ // limitations under the License. import memoize from 'lru-memoize'; +import objectHash from 'object-hash'; import { TEdge } from '@jaegertracing/plexus/lib/types'; @@ -30,6 +31,7 @@ export default class GraphModel { private readonly getPathElemHasher = getPathElemHasher; readonly density: EDdgDensity; readonly distanceToPathElems: TDdgDistanceToPathElems; + readonly hash: string; readonly pathElemToEdge: Map; readonly pathElemToVertex: Map; readonly showOp: boolean; @@ -101,6 +103,14 @@ export default class GraphModel { } }); + const objHasherArg: Record = {}; + this.vertexToPathElems.forEach((pathElems, vertex) => { + objHasherArg[vertex.key] = Array.from(pathElems) + .map(el => el.visibilityIdx) + .sort((a: number, b: number) => a - b); + }); + this.hash = objectHash(objHasherArg).slice(0, 16); + Object.freeze(this.distanceToPathElems); Object.freeze(this.pathElemToEdge); Object.freeze(this.pathElemToVertex); diff --git a/packages/jaeger-ui/src/model/ddg/types.tsx b/packages/jaeger-ui/src/model/ddg/types.tsx index 8de824774f..0c4a15eb82 100644 --- a/packages/jaeger-ui/src/model/ddg/types.tsx +++ b/packages/jaeger-ui/src/model/ddg/types.tsx @@ -93,6 +93,7 @@ export type TDdgVertex = TVertex<{ export type TDdgSparseUrlState = { density: EDdgDensity; end?: number; + hash?: string; operation?: string; service?: string; showOp: boolean; diff --git a/yarn.lock b/yarn.lock index ee4e8e4c5b..d955a2de05 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1539,6 +1539,11 @@ dependencies: moment "*" +"@types/object-hash@^1.3.0": + version "1.3.0" + resolved "https://registry.yarnpkg.com/@types%2fobject-hash/-/object-hash-1.3.0.tgz#b20db2074129f71829d61ff404e618c4ac3d73cf" + integrity sha1-sg2yB0Ep9xgp1h/0BOYYxKw9c88= + "@types/prop-types@*": version "15.7.0" resolved "https://registry.yarnpkg.com/@types/prop-types/-/prop-types-15.7.0.tgz#4c48fed958d6dcf9487195a0ef6456d5f6e0163a" @@ -5564,7 +5569,7 @@ fsevents@^1.2.3, fsevents@^1.2.7: fstream@1.0.12, fstream@^1.0.0, fstream@^1.0.12: version "1.0.12" resolved "https://registry.yarnpkg.com/fstream/-/fstream-1.0.12.tgz#4e8ba8ee2d48be4f7d0de505455548eae5932045" - integrity sha512-WvJ193OHa0GHPEL+AycEJgxvBEwyfRkN1vhjca23OaPVMCaLCXTd5qAu82AjTcgP1UJmytkOKb63Ypde7raDIg== + integrity sha1-Touo7i1Ivk99DeUFRVVI6uWTIEU= dependencies: graceful-fs "^4.1.2" inherits "~2.0.0" @@ -5888,7 +5893,7 @@ handle-thing@^2.0.0: handlebars@4.1.2, handlebars@^4.0.3, handlebars@^4.1.0: version "4.1.2" resolved "https://registry.yarnpkg.com/handlebars/-/handlebars-4.1.2.tgz#b6b37c1ced0306b221e094fc7aca3ec23b131b67" - integrity sha512-nvfrjqvt9xQ8Z/w0ijewdD/vvWDTOweBUm96NTr66Wfvo1mJenBLwcYmPs3TIBP5ruzYGD7Hx/DaM9RmhroGPw== + integrity sha1-trN8HO0DBrIh4JT8eso+wjsTG2c= dependencies: neo-async "^2.6.0" optimist "^0.6.1" @@ -7223,7 +7228,7 @@ js-tokens@^3.0.2: js-yaml@3.13.1, js-yaml@^3.12.0, js-yaml@^3.7.0, js-yaml@^3.9.0: version "3.13.1" resolved "https://registry.yarnpkg.com/js-yaml/-/js-yaml-3.13.1.tgz#aff151b30bfdfa8e49e05da22e7415e9dfa37847" - integrity sha512-YfbcO7jXDdyj0DGxYVSlSeQNHbD7XPWvrVWeVUujrQEoZzWJIRrCPoyk6kL6IAjAG2IolMK4T0hNUe0HOUs5Jw== + integrity sha1-r/FRswv9+o5J4F2iLnQV6d+jeEc= dependencies: argparse "^1.0.7" esprima "^4.0.0" @@ -7777,7 +7782,7 @@ lodash.uniq@^4.5.0: lodash@4.17.11, lodash@4.x, "lodash@>=3.5 <5", lodash@^3.10.0, lodash@^4.15.0, lodash@^4.16.5, lodash@^4.17.10, lodash@^4.17.11, lodash@^4.17.3, lodash@^4.17.4, lodash@^4.17.5, lodash@^4.2.1: version "4.17.11" resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.17.11.tgz#b39ea6229ef607ecd89e2c8df12536891cac9b8d" - integrity sha512-cQKh8igo5QUhZ7lg38DYWAxMvjSAKG0A8wGSVimP07SIUEK2UO+arSRKbRZWtelMtN5V0Hkwh5ryOto/SshYIg== + integrity sha1-s56mIp72B+zYniyN8SU2iRysm40= logfmt@^1.2.0: version "1.2.1" @@ -8593,9 +8598,10 @@ object-copy@^0.1.0: define-property "^0.2.5" kind-of "^3.0.3" -object-hash@^1.1.4: +object-hash@^1.1.4, object-hash@^1.3.1: version "1.3.1" resolved "https://registry.yarnpkg.com/object-hash/-/object-hash-1.3.1.tgz#fde452098a951cb145f039bb7d455449ddc126df" + integrity sha1-/eRSCYqVHLFF8Dm7fUVUSd3BJt8= object-inspect@^1.6.0: version "1.6.0" @@ -12107,7 +12113,7 @@ tapable@^1.0.0, tapable@^1.1.0: tar@2.2.2, tar@^2.0.0: version "2.2.2" resolved "https://registry.yarnpkg.com/tar/-/tar-2.2.2.tgz#0ca8848562c7299b8b446ff6a4d60cdbb23edc40" - integrity sha512-FCEhQ/4rE1zYv9rYXJw/msRqsnmlje5jHP6huWeBZ704jUTy02c5AZyWujpMR1ax6mVw9NyJMfuK2CMDWVIfgA== + integrity sha1-DKiEhWLHKZuLRG/2pNYM27I+3EA= dependencies: block-stream "*" fstream "^1.0.12" From de91e5aff0b4857f71f0fbdc6e66be59a09ef7cb Mon Sep 17 00:00:00 2001 From: Everett Ross Date: Tue, 1 Oct 2019 10:50:21 -0400 Subject: [PATCH 2/3] Move `not` for better test Signed-off-by: Everett Ross --- .../jaeger-ui/src/components/DeepDependencies/index.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/jaeger-ui/src/components/DeepDependencies/index.test.js b/packages/jaeger-ui/src/components/DeepDependencies/index.test.js index 878ccf572d..a166344e6e 100644 --- a/packages/jaeger-ui/src/components/DeepDependencies/index.test.js +++ b/packages/jaeger-ui/src/components/DeepDependencies/index.test.js @@ -137,7 +137,7 @@ describe('DeepDependencyGraphPage', () => { it('includes props.graph.hash iff it is truthy', () => { ddgPageImpl.updateUrlState({}); - expect(getUrlSpy).not.toHaveBeenLastCalledWith(expect.objectContaining({ hash: expect.anything() })); + expect(getUrlSpy).toHaveBeenLastCalledWith(expect.not.objectContaining({ hash: expect.anything() })); const hash = 'testHash'; const propsWithHash = { From 21a31bf385d8c172b17712ab8937e00121d37d74 Mon Sep 17 00:00:00 2001 From: Everett Ross Date: Fri, 11 Oct 2019 17:10:28 -0400 Subject: [PATCH 3/3] Move hash to graphState to ignore density Signed-off-by: Everett Ross --- .../components/DeepDependencies/index.test.js | 18 +++++---- .../src/components/DeepDependencies/index.tsx | 6 +-- .../src/model/ddg/GraphModel/index.test.js | 38 ------------------- .../src/model/ddg/GraphModel/index.tsx | 10 ----- .../src/model/ddg/transformDdgData.test.js | 14 +++++++ .../src/model/ddg/transformDdgData.tsx | 9 +++++ packages/jaeger-ui/src/model/ddg/types.tsx | 1 + 7 files changed, 38 insertions(+), 58 deletions(-) diff --git a/packages/jaeger-ui/src/components/DeepDependencies/index.test.js b/packages/jaeger-ui/src/components/DeepDependencies/index.test.js index a166344e6e..5ca6bad1b4 100644 --- a/packages/jaeger-ui/src/components/DeepDependencies/index.test.js +++ b/packages/jaeger-ui/src/components/DeepDependencies/index.test.js @@ -135,16 +135,19 @@ describe('DeepDependencyGraphPage', () => { expect(props.history.push).toHaveBeenCalledTimes(1); }); - it('includes props.graph.hash iff it is truthy', () => { + it('includes props.graphState.model.hash iff it is truthy', () => { ddgPageImpl.updateUrlState({}); expect(getUrlSpy).toHaveBeenLastCalledWith(expect.not.objectContaining({ hash: expect.anything() })); const hash = 'testHash'; const propsWithHash = { ...props, - graph: { - ...props.graph, - hash, + graphState: { + ...props.graphState, + model: { + ...props.graphState.model, + hash, + }, }, }; const ddgPageWithHash = new DeepDependencyGraphPageImpl(propsWithHash); @@ -368,12 +371,13 @@ describe('DeepDependencyGraphPage', () => { }, }; const ownProps = { location: { search } }; - const mockGraph = { hash: 'testHash', getVisible: () => ({}) }; + const mockGraph = { getVisible: () => ({}) }; + const hash = 'testHash'; const doneState = _set( { ...state }, ['ddg', getStateEntryKey({ service, operation, start: 0, end: 0 })], { - model: {}, + model: { hash }, state: fetchedState.DONE, } ); @@ -443,7 +447,7 @@ describe('DeepDependencyGraphPage', () => { it('sanitizes urlState', () => { mapStateToProps(doneState, ownProps); - expect(sanitizeUrlStateSpy).toHaveBeenLastCalledWith(expected.urlState, mockGraph.hash); + expect(sanitizeUrlStateSpy).toHaveBeenLastCalledWith(expected.urlState, hash); }); }); }); diff --git a/packages/jaeger-ui/src/components/DeepDependencies/index.tsx b/packages/jaeger-ui/src/components/DeepDependencies/index.tsx index f26411bbb1..ae64951b87 100644 --- a/packages/jaeger-ui/src/components/DeepDependencies/index.tsx +++ b/packages/jaeger-ui/src/components/DeepDependencies/index.tsx @@ -171,9 +171,9 @@ export class DeepDependencyGraphPageImpl extends React.PureComponent { toggleShowOperations = (enable: boolean) => this.updateUrlState({ showOp: enable }); updateUrlState = (newValues: Partial) => { - const { graph, history, uiFind, urlState } = this.props; + const { graphState, history, uiFind, urlState } = this.props; const getUrlArg = { uiFind, ...urlState, ...newValues }; - const hash = graph && graph.hash; + const hash = _get(graphState, 'model.hash'); if (hash) getUrlArg.hash = hash; history.push(getUrl(getUrlArg)); }; @@ -273,7 +273,7 @@ export function mapStateToProps(state: ReduxState, ownProps: TOwnProps): TReduxP graphState, services, operationsForService, - urlState: sanitizeUrlState(urlState, graph && graph.hash), + urlState: sanitizeUrlState(urlState, _get(graphState, 'model.hash')), ...extractUiFindFromState(state), }; } diff --git a/packages/jaeger-ui/src/model/ddg/GraphModel/index.test.js b/packages/jaeger-ui/src/model/ddg/GraphModel/index.test.js index c3b3332ed2..b5d85cd73e 100644 --- a/packages/jaeger-ui/src/model/ddg/GraphModel/index.test.js +++ b/packages/jaeger-ui/src/model/ddg/GraphModel/index.test.js @@ -98,44 +98,6 @@ describe('GraphModel', () => { }, ]); }); - - describe('hash', () => { - it('creates equal hashes if model data is equivalent', () => { - const simpleModelEquivalent = transformDdgData( - [simplePath].map((path, i) => ({ path, trace_id: `trace${i}` })), - focalPayloadElem - ); - const testGraphEquivalent = new GraphModel({ - ddgModel: simpleModelEquivalent, - density: EDdgDensity.PreventPathEntanglement, - showOp: true, - }); - - expect(simpleModelEquivalent).not.toEqual(simpleModel); - expect(testGraphEquivalent).not.toBe(testGraph); - expect(testGraphEquivalent.hash).toBe(testGraph.hash); - }); - - it('creates different hashes if model data is different', () => { - const diffModelGraph = new GraphModel({ - ddgModel: doubleFocalModel, - density: EDdgDensity.PreventPathEntanglement, - showOp: true, - }); - - expect(diffModelGraph.hash).not.toBe(testGraph.hash); - }); - - it('creates different hashes if density configurations are different', () => { - const diffDensityGraph = new GraphModel({ - ddgModel: simpleModel, - density: EDdgDensity.UpstreamVsDownstream, - showOp: false, - }); - - expect(diffDensityGraph.hash).not.toBe(testGraph.hash); - }); - }); }); describe('convergent paths', () => { diff --git a/packages/jaeger-ui/src/model/ddg/GraphModel/index.tsx b/packages/jaeger-ui/src/model/ddg/GraphModel/index.tsx index 317f0b7882..201b34f45e 100644 --- a/packages/jaeger-ui/src/model/ddg/GraphModel/index.tsx +++ b/packages/jaeger-ui/src/model/ddg/GraphModel/index.tsx @@ -13,7 +13,6 @@ // limitations under the License. import memoize from 'lru-memoize'; -import objectHash from 'object-hash'; import { TEdge } from '@jaegertracing/plexus/lib/types'; @@ -31,7 +30,6 @@ export default class GraphModel { private readonly getPathElemHasher = getPathElemHasher; readonly density: EDdgDensity; readonly distanceToPathElems: TDdgDistanceToPathElems; - readonly hash: string; readonly pathElemToEdge: Map; readonly pathElemToVertex: Map; readonly showOp: boolean; @@ -103,14 +101,6 @@ export default class GraphModel { } }); - const objHasherArg: Record = {}; - this.vertexToPathElems.forEach((pathElems, vertex) => { - objHasherArg[vertex.key] = Array.from(pathElems) - .map(el => el.visibilityIdx) - .sort((a: number, b: number) => a - b); - }); - this.hash = objectHash(objHasherArg).slice(0, 16); - Object.freeze(this.distanceToPathElems); Object.freeze(this.pathElemToEdge); Object.freeze(this.pathElemToVertex); diff --git a/packages/jaeger-ui/src/model/ddg/transformDdgData.test.js b/packages/jaeger-ui/src/model/ddg/transformDdgData.test.js index 5920bc668d..22ab519a89 100644 --- a/packages/jaeger-ui/src/model/ddg/transformDdgData.test.js +++ b/packages/jaeger-ui/src/model/ddg/transformDdgData.test.js @@ -169,4 +169,18 @@ describe('transform ddg data', () => { transformDdgData([simplePath, noFocalPath, doubleFocalPath].map(testResources.wrap), focalPayloadElem) ).toThrowError(); }); + + it('creates equal hashes iff paths are equivalent', () => { + const { focalPayloadElem, doubleFocalPath, longSimplePath, simplePath, wrap } = testResources; + const simpleModel = transformDdgData([simplePath, longSimplePath].map(wrap), focalPayloadElem); + const reverseModel = transformDdgData([longSimplePath, simplePath].map(wrap), focalPayloadElem); + + expect(reverseModel).not.toEqual(simpleModel); + expect(reverseModel).not.toBe(simpleModel); + expect(reverseModel.hash).toBe(simpleModel.hash); + + const diffModel = transformDdgData([doubleFocalPath].map(wrap), focalPayloadElem); + + expect(diffModel.hash).not.toBe(simpleModel.hash); + }); }); diff --git a/packages/jaeger-ui/src/model/ddg/transformDdgData.tsx b/packages/jaeger-ui/src/model/ddg/transformDdgData.tsx index 902bc1977c..801996f3a5 100644 --- a/packages/jaeger-ui/src/model/ddg/transformDdgData.tsx +++ b/packages/jaeger-ui/src/model/ddg/transformDdgData.tsx @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +import objectHash from 'object-hash'; + import { PathElem, TDdgModel, @@ -31,6 +33,7 @@ export default function transformDdgData( const serviceMap: TDdgServiceMap = new Map(); const distanceToPathElems: TDdgDistanceToPathElems = new Map(); const pathCompareValues: Map = new Map(); + const hashArg: string[] = []; const paths = payload .sort(({ path: a }, { path: b }) => { @@ -50,6 +53,9 @@ export default function transformDdgData( }) // eslint-disable-next-line camelcase .map(({ path: payloadPath, trace_id }) => { + // Default value necessary as sort is not called if there is only one path + hashArg.push(pathCompareValues.get(payloadPath) || payloadPath.map(stringifyEntry).join()); + // Path with stand-in values is necessary for assigning PathElem.memberOf const path: TDdgPath = { focalIdx: -1, members: [], traceID: trace_id }; @@ -124,8 +130,11 @@ export default function transformDdgData( if (upstreamElems) upstreamElems.forEach(setIdx); } while (downstreamElems || upstreamElems); + const hash = objectHash(hashArg).slice(0, 16); + return { paths, + hash, distanceToPathElems, services: serviceMap, visIdxToPathElem, diff --git a/packages/jaeger-ui/src/model/ddg/types.tsx b/packages/jaeger-ui/src/model/ddg/types.tsx index 0c4a15eb82..5ea5a4c7cb 100644 --- a/packages/jaeger-ui/src/model/ddg/types.tsx +++ b/packages/jaeger-ui/src/model/ddg/types.tsx @@ -78,6 +78,7 @@ export type TDdgDistanceToPathElems = Map; export type TDdgModel = { distanceToPathElems: TDdgDistanceToPathElems; + hash: string; paths: TDdgPath[]; services: TDdgServiceMap; visIdxToPathElem: PathElem[];