Create parser function and test with mock data #378

everett980 · 2019-05-10T15:59:53Z

Which problem is this PR solving?

Begin work on DDG, this PR parses expected network payload into The Model.

Short description of the changes

Parsing function and tests

Signed-off-by: Everett Ross <[email protected]>

codecov · 2019-05-10T16:16:12Z

Codecov Report

Merging #378 into master will increase coverage by 0.2%.
The diff coverage is 98.8%.

@@            Coverage Diff            @@
##           master     #378     +/-   ##
=========================================
+ Coverage    88.8%   89.01%   +0.2%     
=========================================
  Files         159      162      +3     
  Lines        3556     3640     +84     
  Branches      813      829     +16     
=========================================
+ Hits         3158     3240     +82     
- Misses        362      364      +2     
  Partials       36       36

Impacted Files	Coverage Δ
...i/src/model/ddg/transformDdgData.test.resources.js	`100% <100%> (ø)`
packages/jaeger-ui/src/model/ddg/PathElem.tsx	`100% <100%> (ø)`
...kages/jaeger-ui/src/model/ddg/transformDdgData.tsx	`98.11% <98.11%> (ø)`
...eViewer/TimelineHeaderRow/TimelineViewingLayer.tsx	`91.52% <0%> (-1.7%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf2baa2...54577cc. Read the comment docs.

tiffon

Looks great. Question about where this stuff should live...?

tiffon · 2019-05-12T05:07:17Z

packages/jaeger-ui/src/utils/DeepDependencyGraph/parse-payload.tsx

@@ -0,0 +1,112 @@
+// Copyright (c) 2019 Uber Technologies, Inc.


What do you think of moving this to src/model/ddg/? As there will be other files, seems like adding src/model/ddg/ would be useful.

The types could go into src/model/ddg/types.tsx? Probably you will have to create additional types, and the types in this file will need to be referenced by other files?

model seems like a better home for this than utils...?

packages/jaeger-ui/src/utils/DeepDependencyGraph/parse-payload.tsx

tiffon · 2019-05-12T05:14:15Z

packages/jaeger-ui/src/utils/DeepDependencyGraph/parse-payload.tsx

+
+type TServiceMap = Record<string, TService>;
+
+type TParsedPayload = {


This name is not very descriptive.

It seems like this should keep track of the DDG parameters (time-range, etc), as well?

Also, it would be nice to keep track of the hash of the data.

I had thought that the hash and request parameters would live in the URL? There isn't much need to have them in the redux state, unless I'm missing something.

I think it also needs to be in the redux store to match with the URL.

how come? i'm not sure what it is used for aside from validating that the BE response when sharing a link is the same response that the shared DDG was constructed from.

Also, seems like link sharing validation through the data hash is part of the Beta enhancement of shareable links and not part of the core MVP.

packages/jaeger-ui/src/utils/DeepDependencyGraph/parse-payload.tsx

packages/jaeger-ui/src/utils/DeepDependencyGraph/parse-payload.test.resources.js

packages/jaeger-ui/src/utils/DeepDependencyGraph/parse-payload.test.js

Signed-off-by: Everett Ross <[email protected]>

tiffon

Requesting a couple of changes. LMK if any of them look awry.

I raised an existential question about PathElem, it's inline in the file. LMK what you think.

packages/jaeger-ui/src/model/ddg/types.tsx

tiffon · 2019-05-15T04:52:22Z

packages/jaeger-ui/src/model/ddg/types.tsx

+
+export type TDdgPathElemsByDistance = Map<number, PathElem[]>;
+
+export type TDdgTransformedDdgData = {


Can this simply be TDdgModel?

Model is fairly generic, but definitely better than DdgTransformedDdgData. Will update to TDdgModel but still remain open to a better name.

packages/jaeger-ui/src/model/ddg/types.tsx

tiffon · 2019-05-15T05:15:39Z

packages/jaeger-ui/src/model/ddg/transform-ddg-data.tsx

+        (pathElemsByDistance.get(member.distance) as PathElem[]).push(member);
+      } else {
+        pathElemsByDistance.set(member.distance, [member]);
+      }


Same as above, the use of a local var can help typescript along:

let members = pathElemsByDistance.get(member.distance); if (members) { members.push(member); } else { pathElemsByDistance.set(member.distance, [member]); }

I think typescript just doesn't have a full appreciation for what Map#has() offers...

I think it's an issue with our TS config. This TS playground has no issue with the if (!map.has(key)) map.set(key, value); map.get(key); pattern:

https://www.typescriptlang.org/play/#src=const%20functionIndexer%3A%20Map%3C(any)%20%3D%3E%20any%2C%20number%20%3E%20%3D%20new%20Map()%3B%0D%0A%0D%0Afunction%20getFnIdx(fn%3A%20(any)%20%3D%3E%20any)%3A%20number%20%7B%0D%0A%20%20%20%20if%20(!functionIndexer.has(fn))%20%7B%0D%0A%20%20%20%20%20%20%20%20functionIndexer.set(fn%2C%20functionIndexer.size%3B%0D%0A%20%20%20%20%7D%0D%0A%20%20%20%20return%20functionIndexer.get(fn)%3B%0D%0A%7D

(Not sure why Github won't let me link to that using [label](link) even when using ctrl + k...)

Though I updated to the pattern you suggested and it works well. But then shortly below this in the while loop cannot be changed in the same way.

The playground link you provided errors if strictNullChecks is enabled.

An alternative approach:

// setup type PathElem = { visibilityIdx: number; } const pathElemsByDistance = new Map<number, PathElem[]>(); // stuff let upstream = 1; let downstream = 0; let visibilityIdx = 0; function setVisibilityIdx(pathElem: PathElem) { pathElem.visibilityIdx = visibilityIdx++; // eslint-disable-line no-param-reassign } let nextUp: PathElem[] | void = pathElemsByDistance.get(upstream); let nextDown: PathElem[] | void = pathElemsByDistance.get(downstream); while (nextUp || nextDown) { if (nextDown) { nextDown.forEach(setVisibilityIdx); nextDown = pathElemsByDistance.get(--downstream); } if (nextUp) { nextUp.forEach(setVisibilityIdx); nextUp = pathElemsByDistance.get(++upstream); } }

Playground – (But, you'll need to enable the strict null checks yourself)

Because as side-steps the type system, I'm thinking we should avoid using it. Or, if we do use it, provide a comment on why it's necessary.

What do you think?

I agree that we should avoid as as much as possible as it generally indicates something is mistyped. Fortunately it isn't a total sidestep but still should be avoided.

If we could somehow "teach" TS Map#has and any future related shortcomings of TS that'd be ideal, but failing that the code blocks in this thread serve as good examples for helping TS.

I updated the related code, but with a do { /* ... */ } while (/* ... */); loop instead and it works well.

packages/jaeger-ui/src/model/ddg/transform-ddg-data.tsx

tiffon · 2019-05-15T05:26:19Z

packages/jaeger-ui/src/model/ddg/types.tsx

+export class PathElem {
+  memberOf: TDdgPath;
+  operation: TDdgOperation;
+  pathIdx: number;


At first I thought this field referred to the index of the parent path, memberOf, within the TDdgTransformedDdgData.

Maybe memberIdx would indicate otherwise?

sure, will rename to memberIdx

Is there value in a TDdgPath knowing it's index in the TDdgModel? I've been building off of this PR for jag-500 (create edges and vertices for PPE with distinguished operations) and it seems like data structures that contain PathElems has been more useful than data structures containing Paths.

packages/jaeger-ui/src/model/ddg/types.tsx

tiffon · 2019-05-15T05:35:52Z

packages/jaeger-ui/src/model/ddg/types.tsx

+  }
+
+  get distance() {
+    return this.pathIdx - this.memberOf.focalIdx;


I like this, but on second thought, the PathElem knowing details about TDdgPath like focalIdx, and being dependent on how focal the index is represented, is unfortunate.

I was thinking of making TDdgPath a class as well, so that focalIdx can be a getter. The benefit of that would be the ability to enable/disable distinguishing operations without needing to recalculate all focalIdxs.
It is somewhat coupled, but the value is high. Through TypeScript (and good tests) it's pretty clear if/when this coupling is broken.

Scratch what I said about focalIdx as a getter, since if the focalIdx changes as a result of the focalNode changing, all visibilityIdxs would be invalid.

tiffon · 2019-05-15T05:39:23Z

I'm curious about merging this directly into master. We can merge to master, incrementally, and hide behind a feature flag. What do you think?

everett980 · 2019-05-15T14:50:08Z

I'm curious about merging this directly into master. We can merge to master, incrementally, and hide behind a feature flag. What do you think?

That sounds fine to me, would we omit DDG PRs from the CHANGELOG.md until the feature flag is removed? If not, there should be some indication that the changes are experimental.

Signed-off-by: Everett Ross <[email protected]>

tiffon

Looks great. Some small comments, mainly about sorting model.paths and the use of as, in general.

LMK if any comment looks awry.

Also, any thoughts re targeting master? I think we should do it.

tiffon · 2019-05-16T00:25:09Z

packages/jaeger-ui/src/model/ddg/transform-ddg-data.tsx

@@ -78,6 +79,7 @@ export default function transformDdgData(
    return path;
  });

+  // Assign visibility indices such there is a positive, dependent correlation between visibilityIdx and distance


I think, in order to ensure this is stable, we should sort model.paths before assigning the visibility indexes.

That sounds reasonable. I implemented the sort as:

const stringifyPayloadEntry = ({ service, operation }: TDdgPayloadEntry) => `${service}::${operation}`; /* ... */ const paths = payload .slice() .sort((a, b) => a .map(stringifyPayloadEntry) .join() .localeCompare(b.map(stringifyPayloadEntry).join()) )

It seems to work well and should work at scale. Did you have a different sort in mind?

Not so sure I'd say this will scale, localeCompare is dog slow:

https://jsperf.com/localecompare/38

Wow that is really slow! I had no idea. I will update it.
I think for our use of static ordering, not necessarily the most logical ordering (e.g.: aZ before aa is fine as long as it is consistent), (a, b) => a > b ? 1 : -1 should be a sufficient comparison to avoid needing to convert to lowercase unnecessarily.

tiffon · 2019-05-16T00:26:33Z

packages/jaeger-ui/src/model/ddg/transform-ddg-data.tsx

+        (pathElemsByDistance.get(member.distance) as PathElem[]).push(member);
+      } else {
+        pathElemsByDistance.set(member.distance, [member]);
+      }


The playground link you provided errors if strictNullChecks is enabled.

tiffon · 2019-05-16T00:42:46Z

packages/jaeger-ui/src/model/ddg/transform-ddg-data.tsx

+        (pathElemsByDistance.get(member.distance) as PathElem[]).push(member);
+      } else {
+        pathElemsByDistance.set(member.distance, [member]);
+      }


An alternative approach:

// setup type PathElem = { visibilityIdx: number; } const pathElemsByDistance = new Map<number, PathElem[]>(); // stuff let upstream = 1; let downstream = 0; let visibilityIdx = 0; function setVisibilityIdx(pathElem: PathElem) { pathElem.visibilityIdx = visibilityIdx++; // eslint-disable-line no-param-reassign } let nextUp: PathElem[] | void = pathElemsByDistance.get(upstream); let nextDown: PathElem[] | void = pathElemsByDistance.get(downstream); while (nextUp || nextDown) { if (nextDown) { nextDown.forEach(setVisibilityIdx); nextDown = pathElemsByDistance.get(--downstream); } if (nextUp) { nextUp.forEach(setVisibilityIdx); nextUp = pathElemsByDistance.get(++upstream); } }

Playground – (But, you'll need to enable the strict null checks yourself)

tiffon · 2019-05-16T00:45:50Z

packages/jaeger-ui/src/model/ddg/transform-ddg-data.tsx

+        (pathElemsByDistance.get(member.distance) as PathElem[]).push(member);
+      } else {
+        pathElemsByDistance.set(member.distance, [member]);
+      }


Because as side-steps the type system, I'm thinking we should avoid using it. Or, if we do use it, provide a comment on why it's necessary.

What do you think?

tiffon · 2019-05-16T01:02:14Z

packages/jaeger-ui/src/model/ddg/transform-ddg-data.tsx

-  TDdgTransformedDdgData,
-  PathElem,
-} from './types';
+import { PathElem, TDdgModel, TDdgPayload, TDdgPath, TDdgPathElemsByDistance, TDdgServiceMap } from './types';

 export default function transformDdgData(


Since we moved to the naming convention of filenames reflecting the default export, the file should be named transformDdgData.tsx. Unfortunately, that was not the initial approach taken, so it's not reflected, everywhere.

I see, I will rename. The usage of kebab-case vs PascalCase vs camelCase file names definitely seem inconsistent. It might be worth mentioning the style guide portion of CONTRIBUTING.md.

It probably is inconsistent due to not renaming everything when adopting the approach... The approach is to name based on default export, if there is one. If not, then a descriptive-name kebab-case name.

Yeah, we need to put a bit of time into a style-guide. I started some notes, here:

https://github.com/jaegertracing/jaeger-ui/blob/style-guide-wip/CONTRIBUTING.md

Created a ticket for it: #385

Signed-off-by: Everett Ross <[email protected]>

…th-mock-data

tiffon

Looks great! I'd suggest dropping the localeCompare and considering using a different delimiter for the stringify.

packages/jaeger-ui/src/model/ddg/transformDdgData.tsx

…th-mock-data

Signed-off-by: Everett Ross <[email protected]>

…arser-function-and-test-with-mock-data Create parser function and test with mock data Signed-off-by: vvvprabhakar <[email protected]>

everett980 added 4 commits May 3, 2019 17:22

WIP - Initial parsing function and tests

03d582d

Signed-off-by: Everett Ross <[email protected]>

WIP - finish test cases for payload parsing

2efd6f4

Signed-off-by: Everett Ross <[email protected]>

Clean up variable names, add new lines and kwargs

3c94555

Signed-off-by: Everett Ross <[email protected]>

Remove unused type, reorder variables

de92808

Signed-off-by: Everett Ross <[email protected]>

ghost assigned everett980 May 10, 2019

ghost added the review label May 10, 2019

everett980 requested a review from tiffon May 10, 2019 18:58

tiffon suggested changes May 12, 2019

View reviewed changes

everett980 added 4 commits May 13, 2019 13:51

WIP - change to maps, improve type names

757501f

Signed-off-by: Everett Ross <[email protected]>

WIP-ensure +correlation for visibilityIdx&distance

002f485

Signed-off-by: Everett Ross <[email protected]>

Move files, test PathElem class

2d138b1

Signed-off-by: Everett Ross <[email protected]>

Remove unnecessary up/down stream distance tracker

34a8dce

Signed-off-by: Everett Ross <[email protected]>

everett980 requested a review from tiffon May 14, 2019 16:06

Move and rename files

1e89801

Signed-off-by: Everett Ross <[email protected]>

tiffon suggested changes May 15, 2019

View reviewed changes

Improve TS usage, partially decouple PathElem

25ce34d

Signed-off-by: Everett Ross <[email protected]>

tiffon suggested changes May 16, 2019

View reviewed changes

everett980 added 2 commits May 16, 2019 14:20

Avoid typecasting, ensure stable visibilityIndices

3fd232f

Signed-off-by: Everett Ross <[email protected]>

Rename new files

b821203

Signed-off-by: Everett Ross <[email protected]>

everett980 changed the base branch from deep-dependency-graph to master May 16, 2019 22:04

Merge branch 'master' into jag-330-create-parser-function-and-test-wi…

29274c6

…th-mock-data

tiffon approved these changes May 22, 2019

View reviewed changes

packages/jaeger-ui/src/model/ddg/transformDdgData.tsx Outdated Show resolved Hide resolved

packages/jaeger-ui/src/model/ddg/transformDdgData.tsx Outdated Show resolved Hide resolved

everett980 added 2 commits May 24, 2019 11:20

Merge branch 'master' into jag-330-create-parser-function-and-test-wi…

7e77bc4

…th-mock-data

Improve payload sorting efficiency

54577cc

Signed-off-by: Everett Ross <[email protected]>

everett980 merged commit 1076705 into jaegertracing:master May 24, 2019

everett980 mentioned this pull request Nov 6, 2019

Implement Service-Oriented Deep Dependency Graph (DDG) #481

Closed

		@@ -0,0 +1,112 @@
		// Copyright (c) 2019 Uber Technologies, Inc.


		type TServiceMap = Record<string, TService>;

		type TParsedPayload = {


		export type TDdgPathElemsByDistance = Map<number, PathElem[]>;

		export type TDdgTransformedDdgData = {

Create parser function and test with mock data #378

Create parser function and test with mock data #378

Conversation

everett980 commented May 10, 2019

Which problem is this PR solving?

Short description of the changes

codecov bot commented May 10, 2019 • edited Loading

Codecov Report

tiffon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiffon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

everett980 May 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiffon commented May 15, 2019

everett980 commented May 15, 2019

tiffon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiffon left a comment

Choose a reason for hiding this comment

codecov bot commented May 10, 2019 •

edited

Loading

everett980 May 15, 2019 •

edited

Loading