fix: optimize the usage of ApiTraceGraph in the ingestion pipeline by kotharironak · Pull Request #195 · hypertrace/hypertrace-ingester

kotharironak · 2021-05-21T08:25:44Z

Description

As part of looking into handling large traces, we have observed a couple of optimization in our ingestion pipeline as described here - hypertrace/hypertrace#244

This PR addresses

addresses the issue of building ApiTraceGraph across enricher for once if the trace has not modified. (Case 2 of the above ticket)
it also modifies ApiTraceGraph internal Map data structure to use Id based index instead of using the entire object

Testing

local testing by ingesting traces to docker-compose setup

codecov · 2021-05-21T08:27:58Z

Codecov Report

Merging #195 (3d9b21d) into main (2123371) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               main     #195   +/-   ##
=========================================
  Coverage     78.92%   78.92%           
  Complexity     1128     1128           
=========================================
  Files           101      101           
  Lines          4370     4370           
  Branches        406      406           
=========================================
  Hits           3449     3449           
  Misses          732      732           
  Partials        189      189

Flag	Coverage Δ	Complexity Δ
unit	`78.92% <100.00%> (ø)`	`1128.00 <14.00> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
...ichment/enrichers/ErrorsAndExceptionsEnricher.java	`86.66% <100.00%> (ø)`	`19.00 <8.00> (ø)`
...richer/enrichment/enrichers/ExitCallsEnricher.java	`100.00% <100.00%> (ø)`	`8.00 <2.00> (ø)`
...nrichment/enrichers/endpoint/EndpointEnricher.java	`78.16% <100.00%> (ø)`	`15.00 <4.00> (ø)`

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 2123371...3d9b21d. Read the comment docs.

kotharironak · 2021-05-21T08:45:40Z

I have checked the application flow graph, services, endpoints, waterfall view of traces by running docker-compose

findingrish

LGTM!

tim-mwangi · 2021-05-21T17:05:33Z

@d-trace does this affect what you are working on?

d-trace · 2021-05-21T18:29:37Z

@d-trace does this affect what you are working on?

No @tim-mwangi. My logic at the moment does calculations based on StructuredTrace directly.

d-trace

LGTM

tim-mwangi · 2021-05-21T20:17:41Z

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class GraphBuilderUtil {


Is it possible to add unit tests for this util?

tim-mwangi · 2021-05-21T20:20:10Z

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ApiTraceGraphBuilder {


unit tests?

findingrish · 2021-05-24T08:56:01Z

+
+        // calls second time, this time it returns from cache
+        StructuredTraceGraphBuilder.buildGraph(underTestTrace);
+        structuredTraceGraphMockedStatic.verify(


this assertion should be placed before the second call?

It makes sure that after the second call, it was not incremented. Let me put at both the places. Done.

Yeah, it should be at both places

findingrish · 2021-05-24T08:58:22Z

+        // first call
+        ApiTraceGraph actual = ApiTraceGraphBuilder.buildGraph(underTestTrace);
+        Assertions.assertNotNull(actual);
+        Assertions.assertEquals(1, mockedConstruction.constructed().size());


what is this mockedConstruction.constructed().size() size representing?

The number of times new ApiTraceGraph(trace) is called.

findingrish · 2021-05-24T09:02:02Z

+  /**
+   * optimistic method of comparing two trace for considering rebuilding of entire graph structure.
+   */
+  static boolean isSameStructuredTrace(StructuredTrace cachedTrace, StructuredTrace trace) {


the method name could be misleading, we are trying to check if the components in the Trace are same as previous or not. Trace might have changed due to event enrichment or trace enrichment.

findingrish · 2021-05-24T09:11:22Z

+    when(underTestTrace.getEventEdgeList()).thenReturn(List.of(eventEdge));
+
+    boolean result = GraphBuilderUtil.isSameStructuredTrace(cachedTrace, underTestTrace);
+    Assertions.assertTrue(result);


Test the negative case as well?

The remains are covered with the other two tests, and this I have created as an end to cover up all. But, if you want, I can add one to the list item.

I think it can be taken up subsequently

findingrish · 2021-05-24T09:12:55Z

+        // calls second time, this time it returns from cache
+        StructuredTraceGraphBuilder.buildGraph(underTestTrace);
+        structuredTraceGraphMockedStatic.verify(
+            () -> StructuredTraceGraph.createGraph(underTestTrace), times(1));


I think, its also important to test that the cached structure is not returned when a different trace is passed?

Isn't the fist call doing the same StructuredTraceGraphBuilder.buildGraph as it has to test GraphBuilderUtil.isSameStructuredTrace and the first call cover that part and the method is independently covered in GraphBuilderUtilTest.

yeah the correctness is tested, this was more of a suggestion for completeness sake.

First test with empty cache, second call return from cache, third call with different trace again build structure.

findingrish · 2021-05-24T09:13:17Z

+        // second call
+        ApiTraceGraph second = ApiTraceGraphBuilder.buildGraph(underTestTrace);
+        Assertions.assertEquals(actual, second);
+        Assertions.assertEquals(1, mockedConstruction.constructed().size());


I think, its also important to test that the cached graph is not returned when a different trace is passed?

The first call is testing that we are going to if block where the cache is prepared. And GraphBuilderUtil.isSameStructuredTrace is independently tested in GraphBuilderUtilTest.

github-actions · 2021-05-24T11:51:34Z

Unit Test Results

  64 files ±0   64 suites ±0 49s ⏱️ -3s
314 tests ±0 314 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 94b8c05. ± Comparison against base commit 2123371.

fix: optimize the usage of ApiTraceGraph in ingestion pipeline

48e895d

kotharironak requested review from a team, findingrish, laxmanchekka and ravisingal May 21, 2021 08:25

chore: addresses the access modifier for logger to private

d4b96f1

This comment has been minimized.

Sign in to view

chore: reverted the temporal change in test case

d7a49b9

This comment has been minimized.

Sign in to view

findingrish previously approved these changes May 21, 2021

View reviewed changes

tim-mwangi requested a review from d-trace May 21, 2021 17:04

d-trace reviewed May 21, 2021

View reviewed changes

tim-mwangi reviewed May 21, 2021

View reviewed changes

fix: adds unit tests for newly added class

98a801d

kotharironak dismissed findingrish’s stale review via 98a801d May 24, 2021 08:41

This comment has been minimized.

Sign in to view

chore: address few formatting

07d3e0e

kotharironak requested review from d-trace, findingrish and tim-mwangi May 24, 2021 08:46

This comment has been minimized.

Sign in to view

findingrish reviewed May 24, 2021

View reviewed changes

address comments

3d9b21d

kotharironak requested a review from findingrish May 24, 2021 10:35

This comment has been minimized.

Sign in to view

findingrish approved these changes May 24, 2021

View reviewed changes

kotharironak merged commit 94b8c05 into main May 24, 2021

kotharironak deleted the optimize-api-trace-graph branch May 24, 2021 11:49

Conversation

kotharironak commented May 21, 2021

Description

Testing

Uh oh!

codecov bot commented May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kotharironak commented May 21, 2021

Uh oh!

findingrish left a comment

Choose a reason for hiding this comment

Uh oh!

tim-mwangi commented May 21, 2021

Uh oh!

d-trace commented May 21, 2021

Uh oh!

d-trace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kotharironak May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

github-actions bot commented May 24, 2021

Unit Test Results

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented May 21, 2021 •

edited

Loading

kotharironak May 24, 2021 •

edited

Loading