-
Notifications
You must be signed in to change notification settings - Fork 16
chore: Update data-model #228
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| shouldRebuildTraceEventsGraph | ||
| ? StructuredTraceGraph.reCreateTraceEventsGraph(trace) | ||
| : StructuredTraceGraph.reCreateTraceEntitiesGraph(trace); | ||
| StructuredTraceGraph graph = cachedGraphThreadLocal.get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a chance this can be null?
To be safe should we just create a new StructuredTraceGraph if null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although that case shouldn't arise, but added a check for the cachedGraph in the first check
| StructuredTraceGraph graph = StructuredTraceGraph.createGraph(trace); | ||
| StructuredTraceGraph graph = new StructuredTraceGraph(trace); | ||
| LOG.debug( | ||
| "Time taken in building StructuredTraceGraph, duration_millis:{} for tenantId:{}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not related to this PR, but wrap the below debug statement inside a LOG.isDebugEnabled() to avoid the computation of Duration.between(start, Instant.now()).toMillis()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| } else { | ||
| graph.reCreateTraceEntitiesGraph(trace); | ||
| } | ||
| LOG.debug( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
wrap the below debug statement inside a LOG.isDebugEnabled()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Codecov Report
@@ Coverage Diff @@
## main #228 +/- ##
=========================================
Coverage 80.50% 80.50%
Complexity 1126 1126
=========================================
Files 101 101
Lines 4349 4349
Branches 400 400
=========================================
Hits 3501 3501
Misses 659 659
Partials 189 189
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| } else { | ||
| cachedGraph.reCreateTraceEntitiesGraph(trace); | ||
| } | ||
| if (LOG.isDebugEnabled()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this log be inside the if condition ? or may be fix the log statement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't get your point, what changes are you suggesting in the log statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it says building events graph but it could be either events or entities graph
No description provided.