Refactor model/trace-dag to prep for latency diffs #521

tiffon · 2020-02-17T08:45:04Z

Which problem is this PR solving?

Initial work for Comparing traces by latency #513 – Comparing traces by latency

Short description of the changes

Refactor TraceDag class to make it simpler and more flexible
Change DagNode to be a simple type instead of a class
Change the data on DagNode to extend DagNode instead of live off the data field
Base the data model for trace diffs on collections of DenseSpans instead of just the count of spans for a given node in the comparison DAG (e.g. 7 vs 4 are now arrays instead of just the count)
Lay some small ground-work (id-factories.tsx) for being more flexible in how the DAG is built from the trace, which is to say how spans are grouped

codecov · 2020-02-17T08:59:14Z

Codecov Report

Merging #521 into master will decrease coverage by 0.21%.
The diff coverage is 73.52%.

@@            Coverage Diff             @@
##           master     #521      +/-   ##
==========================================
- Coverage   93.00%   92.78%   -0.22%     
==========================================
  Files         197      197              
  Lines        4848     4839       -9     
  Branches     1177     1185       +8     
==========================================
- Hits         4509     4490      -19     
- Misses        299      309      +10     
  Partials       40       40

Impacted Files	Coverage Δ
...src/components/TracePage/TraceGraph/TraceGraph.tsx	`95.00% <ø> (ø)`
.../jaeger-ui/src/model/trace-dag/denseTransforms.tsx	`70.90% <ø> (ø)`
...ackages/jaeger-ui/src/model/trace-dag/TraceDag.tsx	`63.04% <62.79%> (-15.68%)`	⬇️
...nents/TracePage/TraceGraph/calculateTraceDagEV.tsx	`92.68% <83.33%> (-0.18%)`	⬇️
...components/TraceDiff/TraceDiffGraph/renderNode.tsx	`86.95% <100.00%> (+1.24%)`	⬆️
...s/TraceDiff/TraceDiffGraph/traceDiffGraphUtils.tsx	`83.33% <100.00%> (ø)`
...-ui/src/components/TracePage/TraceGraph/OpNode.tsx	`81.81% <100.00%> (-0.80%)`	⬇️
...kages/jaeger-ui/src/model/trace-dag/DenseTrace.tsx	`96.96% <100.00%> (ø)`
...kages/jaeger-ui/src/model/trace-dag/convPlexus.tsx	`100.00% <100.00%> (ø)`
...ges/jaeger-ui/src/model/trace-dag/id-factories.tsx	`100.00% <100.00%> (ø)`
... and 2 more

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 5f86618...d59380d. Read the comment docs.

everett980 · 2020-02-20T21:52:33Z

Hey @tiffon, unfortunately I won't have bandwidth to pore over this PR for two weeks, sorry.

If you intend on continuing to work on 513, you should probably branch off of this branch without waiting for review.

tiffon · 2020-02-21T03:49:32Z

@everett980 thanks for the heads up.

everett980

@tiffon Sorry for the delay in getting to this.

This diff looks good, thanks!

I left a few comments but all but one are about type names or copyright (BTW: didn't comment on each copyright addition) so it shouldn't take long to update.

everett980 · 2020-03-03T23:23:22Z

packages/jaeger-ui/src/components/TraceDiff/TraceDiffGraph/renderNode.tsx

@@ -1,3 +1,4 @@
+// Copyright (c) 2020 The Jaeger Authors.


I believe we don't typically update the Copyright for existing files. @yurishkuro please confirm

We don't have a firm policy. Some lawyers told me that the "correct" way to update copyrights is to add a year when the change is made, so one could end up with Copyright (c) 2017,2019,2020 The Jaeger Authors. Which seems rather heavy handed and somewhat pointless since one needs to go through Git history to figure out which parts of the code are C in which year.

A simpler solution is to use a range 2017-2020.

Simple replacing C with the current year appears to be wrong, but still done in many oss projects (and tooling).

So I would go with the range, if there was a year before.

Done, changed to use range from previous date but now is set to Jaeger Authors.

everett980 · 2020-03-03T23:23:54Z

packages/jaeger-ui/src/components/TraceDiff/TraceDiffGraph/traceDiffGraphUtils.tsx

@@ -1,3 +1,4 @@
+// Copyright (c) 2020 The Jaeger Authors.


same question about adding / updating licenses

everett980 · 2020-03-03T23:26:54Z

packages/jaeger-ui/src/components/TraceDiff/TraceDiffGraph/traceDiffGraphUtils.tsx

-function getUiFindVertexKeysFn(uiFind: string, vertices: TDagVertex<any>[]): Set<TVertexKey> {
+function getUiFindVertexKeysFn(
+  uiFind: string,
+  vertices: DagPlexusVertex<DenseSpanMembers>[]


👍 replacing any 🎉

everett980 · 2020-03-03T23:40:52Z

packages/jaeger-ui/src/components/TracePage/TraceGraph/types.tsx

@@ -1,3 +1,4 @@
+// Copyright (c) 2020 The Jaeger Authors.


same copyright question

everett980 · 2020-03-03T23:41:19Z

packages/jaeger-ui/src/model/trace-dag/TraceDag.tsx

@@ -1,3 +1,4 @@
+// Copyright (c) 2020 The Jaeger Authors.


same copyright question

everett980 · 2020-03-03T23:44:47Z

packages/jaeger-ui/src/model/trace-dag/types/index.tsx

@@ -32,3 +29,14 @@ export type DenseSpan = {
  skipToChild: boolean;
  children: Set<string>;
 };
+
+export type DenseSpanMembers = {


why not TDenseSpanMembers?

everett980 · 2020-03-03T23:50:18Z

packages/jaeger-ui/src/model/trace-dag/types/index.tsx

+  service: string;
+};
+
+export type DiffCounts = DenseSpanMembers & {


why not TDiffCounts?

everett980 · 2020-03-03T23:53:15Z

packages/jaeger-ui/src/model/trace-dag/types/index.tsx

+  service: string;
+};
+
+export type DiffCounts = DenseSpanMembers & {


Instead of having members, operation, and service on this and a & b, I think we can we get away with:

export type TDiffCounts = TDenseSpanMembers & { a: DenseSpan[] | null; b: DenseSpan[] | null; };

seems a bit simpler and would avoid duplicate data.

SGTM. Keen eye.

The thought behind a and b being TDagNodes is they're the input to the diff, in their original form. Your change reduces the data down to what's relevant, so I think it makes sense and is better. I have some notion of it being a nice idea to add one-vs-many comparisons. In that case, I was thinking the schema would be a Map with keys being trace IDs and values being the TDagNode from the corresponding trace. This morphed into using "a" and "b" mapped to the TDagNode. Which is not a very good rationale.

everett980 · 2020-03-03T23:54:00Z

packages/jaeger-ui/src/model/trace-dag/types/DagPlexusVertex.tsx


 // eslint-disable-next-line no-undef
-export default TDagVertex;
+export default DagPlexusVertex;


this should also have a T, TDagPlexusVertex

everett980 · 2020-03-03T23:54:26Z

packages/jaeger-ui/src/model/trace-dag/types/DagNode.tsx

+};
+
+// eslint-disable-next-line no-undef
+export default DagNode;


this should also have a T, TDagNode

everett980 · 2020-03-04T19:47:28Z

Also, I noticed the code coverage for the diff is below master's coverage. Do you plan on improving coverage when continuing with the feature work for #513 or should that be included in this diff?

- Refactor TraceDag class to make it simpler and more flexible - Change DagNode to be a simple type instead of a class - Change the data on DagNode to extend DagNode instead of live off the "data" field - Base the data model for trace diffs on collections of DenseSpans instead of just the count of spans in a vs b for a given node in the comparison DAG - Lay some small (id-factories.tsx) ground-work for being more flexible in how the DAG is built from the trace, which is to say how spans are grouped Signed-off-by: Joe Farro <joe@jf.io>

tiffon · 2020-03-22T07:52:54Z

@everett980 Damn, I sent an email to this conversation but it isn't here. Sorry for the major delay in responding.

The changes are made. LMK if you see anything else.

tiffon · 2020-03-22T07:54:02Z

@everett980 Regarding the test coverage, I was thinking to bulk it up when I add latency comparisons.

everett980 · 2020-03-26T19:07:08Z

Damn, I sent an email to this conversation but it isn't here. Sorry for the major delay in responding.

@tiffon no worries. In return, I managed to miss the GitHub emails for your comments.

Regarding the test coverage, I was thinking to bulk it up when I add latency comparisons.

SGTM

- Refactor TraceDag class to make it simpler and more flexible - Change DagNode to be a simple type instead of a class - Change the data on DagNode to extend DagNode instead of live off the "data" field - Base the data model for trace diffs on collections of DenseSpans instead of just the count of spans in a vs b for a given node in the comparison DAG - Lay some small (id-factories.tsx) ground-work for being more flexible in how the DAG is built from the trace, which is to say how spans are grouped Signed-off-by: Joe Farro <joe@jf.io> Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>

tiffon requested review from everett980 and yurishkuro February 17, 2020 08:45

tiffon force-pushed the trace-dag-refactor branch from a141445 to be6c215 Compare February 18, 2020 10:17

everett980 mentioned this pull request Feb 20, 2020

Comparing traces by latency #513

Open

everett980 requested changes Mar 4, 2020

View reviewed changes

tiffon force-pushed the trace-dag-refactor branch from be6c215 to dc39cbe Compare March 22, 2020 07:51

Merge branch 'master' into trace-dag-refactor

d59380d

everett980 merged commit 3ebb95d into jaegertracing:master Mar 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor model/trace-dag to prep for latency diffs #521

Refactor model/trace-dag to prep for latency diffs #521

tiffon commented Feb 17, 2020

codecov bot commented Feb 17, 2020 •

edited

Loading

everett980 commented Feb 20, 2020

tiffon commented Feb 21, 2020

everett980 left a comment

everett980 Mar 3, 2020

yurishkuro Mar 4, 2020

tiffon Mar 22, 2020

everett980 Mar 3, 2020

tiffon Mar 22, 2020

everett980 Mar 3, 2020

everett980 Mar 3, 2020

tiffon Mar 22, 2020

everett980 Mar 3, 2020

tiffon Mar 22, 2020

everett980 Mar 3, 2020

tiffon Mar 22, 2020

everett980 Mar 3, 2020

tiffon Mar 22, 2020

everett980 Mar 3, 2020

tiffon Mar 22, 2020

everett980 Mar 3, 2020

tiffon Mar 22, 2020

everett980 Mar 3, 2020

tiffon Mar 22, 2020

everett980 commented Mar 4, 2020

tiffon commented Mar 22, 2020 •

edited

Loading

tiffon commented Mar 22, 2020

everett980 commented Mar 26, 2020

Refactor model/trace-dag to prep for latency diffs #521

Refactor model/trace-dag to prep for latency diffs #521

Conversation

tiffon commented Feb 17, 2020

Which problem is this PR solving?

Short description of the changes

codecov bot commented Feb 17, 2020 • edited Loading

Codecov Report

everett980 commented Feb 20, 2020

tiffon commented Feb 21, 2020

everett980 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

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

everett980 commented Mar 4, 2020

tiffon commented Mar 22, 2020 • edited Loading

tiffon commented Mar 22, 2020

everett980 commented Mar 26, 2020

codecov bot commented Feb 17, 2020 •

edited

Loading

tiffon commented Mar 22, 2020 •

edited

Loading