Optimisation: Share state between view-generators #108
Conversation
Codecov Report
@@ Coverage Diff @@
## main #108 +/- ##
============================================
+ Coverage 68.61% 69.38% +0.76%
- Complexity 821 830 +9
============================================
Files 84 85 +1
Lines 3486 3505 +19
Branches 367 369 +2
============================================
+ Hits 2392 2432 +40
+ Misses 950 927 -23
- Partials 144 146 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
findingrish
requested review from
buchi-busireddy,
kotharironak,
laxmanchekka,
skjindal93 and
surajpuvvada
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Unit tests? |
This comment has been minimized.
This comment has been minimized.
|
|
||
| // Construct ApiTraceGraph and look at all the head spans within each ApiNode | ||
| ApiTraceGraph apiTraceGraph = new ApiTraceGraph(structuredTrace); | ||
| ApiTraceGraph apiTraceGraph = ViewGeneratorState.getApiTraceGraph(structuredTrace); |
There was a problem hiding this comment.
I think it will be easy not to make functions static, and use ViewGeneratorState as a singleton class. It would really simplify writing the unit tests for the class, because then you can easily mock ViewGeneratorState class
There was a problem hiding this comment.
I will give it a try, and if it works will make the changes in a subsequent pr.
There was a problem hiding this comment.
If it is made a singleton, remember that it should be stateless :)
| return traceStateThreadLocal.get(); | ||
| } | ||
|
|
||
| private static boolean isDifferentTrace(StructuredTrace cached, StructuredTrace trace) { |
There was a problem hiding this comment.
When will this happen in view generators?
There was a problem hiding this comment.
When the same trace is getting processed by multiple view-generators, this would return false, and we save on some repeated operations.
When a new trace starts getting processed, this would return false, leading to rebuilding of TraceState and ApiTraceGraph
There was a problem hiding this comment.
I was mainly concerned/curious about the customer id check, but I guess that's just done for safety? Because, a trace being different means, a different trace id
There was a problem hiding this comment.
yes, just an additional check
There was a problem hiding this comment.
I think checking for traceId is also not required, since the input trace doesn't get modified in this service. So all we need is to check for same object.
|
Tagging the corresponding issue - #102 |
| assertNotNull(traceState); | ||
| assertEquals(trace, traceState.getTrace()); | ||
|
|
||
| StructuredTrace modifiedTrace = getTestTrace(customerId, traceId1); |
There was a problem hiding this comment.
I think it's not modifiedTrace, its a new one, right?
There was a problem hiding this comment.
I got it, it's the same one.
findingrish
dismissed stale reviews from kotharironak and skjindal93
via
6907695
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Currently we run, all the view-generators in individual services.
Although we do have a possibility of running view generator together in a single service using
MultiViewGeneratorLauncher,though that won't be very optimal due to repeated operations amongst the view-generators.
This pr tries to optimise that by caching shared state in a threadlocal, thus ensuring that for a given Trace, those operations are performed once.
Note: this changes should not affect in the case when view-generators have to be individually deployed