Skip to content

Conversation

@findingrish
Copy link
Contributor

No description provided.

@findingrish findingrish requested a review from a team July 20, 2021 08:38
@codecov
Copy link

codecov bot commented Jul 20, 2021

Codecov Report

Merging #227 (21adf29) into main (2055b11) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               main     #227   +/-   ##
=========================================
  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           
Flag Coverage Δ
unit 80.50% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out 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 2055b11...21adf29. Read the comment docs.

@github-actions

This comment has been minimized.

// does not need to build the full traversal graph, just get the parents mapping
StructuredTraceGraph graph = buildGraph(trace);

if (null == graph.getTraceEntitiesGraph() || null == graph.getTraceEventsGraph()) {
Copy link
Contributor

@kotharironak kotharironak Jul 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can happen from multiple places, and we have seen in other enrichers too. So, should we move this to the data-model class?
Basically, can we move the logging to buildGraph method of StructureTraceGraph?

And can we do two things?

  • can we make the default arg constructor private?
public StructuredTraceGraph() {
  }
  • And change the createGraph method as below?
public static StructuredTraceGraph createGraph(StructuredTrace trace) {
   TraceEventsGraph traceEventsGraph = TraceEventsGraph.createGraph(trace);
    TraceEntitiesGraph traceEntitiesGraph = TraceEntitiesGraph.createGraph(trace);
    graph = new StructuredTraceGraph();
    graph.traceEventsGraph = traceEventsGraph;
    graph.traceEntitiesGraph = traceEntitiesGraph;
    return graph;
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the log, on the other suggestion should we first observe the log and then make changes?

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.


private static void debugGraph(
String logPrefix, StructuredTraceGraph graph, StructuredTrace trace) {
if (null == graph.getTraceEntitiesGraph() || null == graph.getTraceEventsGraph()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets add graph != null too?

return cachedGraphThreadLocal.get();
}

private static void debugGraph(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: we will remove this after debugging.

@github-actions

This comment has been minimized.

@findingrish findingrish merged commit 0c1c4e0 into main Jul 20, 2021
@findingrish findingrish deleted the api-attribute-enricher-logs branch July 20, 2021 11:02
@github-actions
Copy link

Unit Test Results

  69 files  ±0    69 suites  ±0   47s ⏱️ -5s
359 tests ±0  359 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 0c1c4e0. ± Comparison against base commit 2055b11.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants