From 48e895dcad91debee8073ddc843ded8f32a0997b Mon Sep 17 00:00:00 2001 From: Ronak Date: Fri, 21 May 2021 13:50:48 +0530 Subject: [PATCH 1/6] fix: optimize the usage of ApiTraceGraph in ingestion pipeline --- .../trace/util/ApiTraceGraph.java | 40 ++++++++++++------- .../trace/util/ApiTraceGraphBuilder.java | 31 ++++++++++++++ .../trace/util/GraphBuilderUtil.java | 40 +++++++++++++++++++ .../util/StructuredTraceGraphBuilder.java | 40 +++++-------------- .../ErrorsAndExceptionsEnricher.java | 3 +- .../enrichers/ExitCallsEnricher.java | 3 +- .../enrichers/endpoint/EndpointEnricher.java | 4 +- .../enrichers/ExitCallsEnricherTest.java | 3 +- 8 files changed, 115 insertions(+), 49 deletions(-) create mode 100644 hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/ApiTraceGraphBuilder.java create mode 100644 hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/GraphBuilderUtil.java diff --git a/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/ApiTraceGraph.java b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/ApiTraceGraph.java index 33fb90ea5..cb56c111d 100644 --- a/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/ApiTraceGraph.java +++ b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/ApiTraceGraph.java @@ -42,10 +42,15 @@ public class ApiTraceGraph { // set of outbound edges for apiNode private final Map> apiNodeIdxToEdges; private final Map eventIdToIndexInTrace; - // map of api node to index in api nodes list, since we want to build edges between api nodes - private final Map, Integer> apiNodeToIndex; - // map of entry boundary event to api node. each api node has one entry boundary event - private final Map> entryBoundaryToApiNode; + + // map of head event id of api node to index in api node list, helps in building edges between api + // nodes + private final Map apiNodeHeadEventIdToIndex; + + // map of event id of entry api boundary event to api node. each api node has at most one entry + // boundary event + private final Map> entryApiBoundaryEventIdToApiNode; + private final Table traceEdgeTable; // set of exit boundary events of apiNode, with no outgoing edge to any apiNode private final Set apiExitBoundaryEventIdxWithNoOutgoingEdge; @@ -64,8 +69,8 @@ public ApiTraceGraph(StructuredTrace trace) { apiNodeIdxToEdges = Maps.newHashMap(); eventIdToIndexInTrace = Maps.newHashMap(); - apiNodeToIndex = Maps.newHashMap(); - entryBoundaryToApiNode = Maps.newHashMap(); + apiNodeHeadEventIdToIndex = Maps.newHashMap(); + entryApiBoundaryEventIdToApiNode = Maps.newHashMap(); traceEdgeTable = HashBasedTable.create(); apiExitBoundaryEventIdxWithNoOutgoingEdge = Sets.newHashSet(); @@ -109,11 +114,13 @@ public List getApiEntryBoundaryEventsWithIncomingEdge() { } public List getOutboundEdgesForApiNode(ApiNode apiNode) { - int idx = apiNodeToIndex.get(apiNode); + int idx = apiNodeHeadEventIdToIndex.get(apiNode.getHeadEvent().getEventId()); if (!apiNodeIdxToEdges.containsKey(idx)) { return Collections.emptyList(); } - return apiNodeIdxToEdges.get(apiNodeToIndex.get(apiNode)).stream() + return apiNodeIdxToEdges + .get(apiNodeHeadEventIdToIndex.get(apiNode.getHeadEvent().getEventId())) + .stream() .map(apiNodeEventEdgeList::get) .collect(Collectors.toList()); } @@ -281,7 +288,7 @@ private void buildApiNodeEdges(StructuredTraceGraph graph) { if (EnrichedSpanUtils.isEntryApiBoundary(exitBoundaryEventChild)) { // get the api node exit boundary event is connecting to ApiNode destinationApiNode = - entryBoundaryToApiNode.get(exitBoundaryEventChild); + entryApiBoundaryEventIdToApiNode.get(exitBoundaryEventChild.getEventId()); Optional edgeBetweenApiNodes = createEdgeBetweenApiNodes( @@ -292,7 +299,9 @@ private void buildApiNodeEdges(StructuredTraceGraph graph) { apiExitBoundaryEventIdxWithOutgoingEdge.add(edge.getSrcEventIndex()); apiEntryBoundaryEventIdxWithIncomingEdge.add(edge.getTgtEventIndex()); apiNodeIdxToEdges - .computeIfAbsent(apiNodeToIndex.get(apiNode), v -> Sets.newHashSet()) + .computeIfAbsent( + apiNodeHeadEventIdToIndex.get(apiNode.getHeadEvent().getEventId()), + v -> Sets.newHashSet()) .add(apiNodeEventEdgeList.size() - 1); }); } else { @@ -322,8 +331,9 @@ private Optional createEdgeBetweenApiNodes( Event entryBoundaryEventOfDestinationApiNode) { if (destinationApiNode != null) { // get the indexes in apiNodes list to create an edge - Integer srcIndex = apiNodeToIndex.get(srcApiNode); - Integer targetIndex = apiNodeToIndex.get(destinationApiNode); + Integer srcIndex = apiNodeHeadEventIdToIndex.get(srcApiNode.getHeadEvent().getEventId()); + Integer targetIndex = + apiNodeHeadEventIdToIndex.get(destinationApiNode.getHeadEvent().getEventId()); // Get the actual edge from trace connecting exitBoundaryEvent and child Integer srcIndexInTrace = @@ -381,8 +391,10 @@ private void buildApiExitBoundaryEventWithNoOutgoingEdge() { private void buildApiNodeToIndexMap() { for (int i = 0; i < apiNodeList.size(); i++) { ApiNode apiNode = apiNodeList.get(i); - apiNode.getEntryApiBoundaryEvent().ifPresent(e -> entryBoundaryToApiNode.put(e, apiNode)); - apiNodeToIndex.put(apiNode, i); + apiNode + .getEntryApiBoundaryEvent() + .ifPresent(e -> entryApiBoundaryEventIdToApiNode.put(e.getEventId(), apiNode)); + apiNodeHeadEventIdToIndex.put(apiNode.getHeadEvent().getEventId(), i); } } diff --git a/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/ApiTraceGraphBuilder.java b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/ApiTraceGraphBuilder.java new file mode 100644 index 000000000..9123df517 --- /dev/null +++ b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/ApiTraceGraphBuilder.java @@ -0,0 +1,31 @@ +package org.hypertrace.traceenricher.trace.util; + +import java.time.Duration; +import java.time.Instant; +import java.util.concurrent.TimeUnit; +import org.hypertrace.core.datamodel.StructuredTrace; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class ApiTraceGraphBuilder { + public static final Logger LOG = LoggerFactory.getLogger(ApiTraceGraphBuilder.class); + + private static ThreadLocal cachedGraph = new ThreadLocal<>(); + private static ThreadLocal cachedTrace = new ThreadLocal<>(); + + public static ApiTraceGraph buildGraph(StructuredTrace trace) { + if (!GraphBuilderUtil.isSameStructuredTrace(cachedTrace.get(), trace)) { + Instant start = Instant.now(); + ApiTraceGraph graph = new ApiTraceGraph(trace); + LOG.debug( + "Time taken in building ApiTraceGraph:{} for tenantId:{}", + Duration.between(start, Instant.now()).toMillis(), + TimeUnit.MILLISECONDS, + trace.getCustomerId()); + cachedTrace.set(StructuredTrace.newBuilder(trace).build()); + cachedGraph.set(graph); + return graph; + } + return cachedGraph.get(); + } +} diff --git a/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/GraphBuilderUtil.java b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/GraphBuilderUtil.java new file mode 100644 index 000000000..48e4219d0 --- /dev/null +++ b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/GraphBuilderUtil.java @@ -0,0 +1,40 @@ +package org.hypertrace.traceenricher.trace.util; + +import org.hypertrace.core.datamodel.StructuredTrace; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class GraphBuilderUtil { + public static final Logger LOG = LoggerFactory.getLogger(GraphBuilderUtil.class); + + /** + * optimistic method of comparing two trace for considering rebuilding of entire graph structure. + */ + static boolean isSameStructuredTrace(StructuredTrace cachedTrace, StructuredTrace trace) { + if (cachedTrace == null || trace == null) { + LOG.debug("Cached and Input trace are not same. Reason: one of the input is null"); + return false; + } + // is processed and cached are same trace? + if (!cachedTrace.getCustomerId().equals(trace.getCustomerId()) + || !cachedTrace.getTraceId().equals(trace.getTraceId())) { + LOG.debug( + "Cached and Input trace are not same. Reason: doesn't not match either traceId or tenantId"); + return false; + } + + // trace internally changed (full trace comparision is costly, so we are doing only with + // required fields) + if (cachedTrace.getEntityList().size() != trace.getEntityList().size() + || cachedTrace.getEventList().size() != trace.getEventList().size() + || cachedTrace.getEntityEdgeList().size() != trace.getEntityEdgeList().size() + || cachedTrace.getEntityEventEdgeList().size() != trace.getEntityEventEdgeList().size() + || cachedTrace.getEventEdgeList().size() != trace.getEventEdgeList().size()) { + LOG.debug( + "Cached and Input trace are not same. Reason: they are having different size either for event or entities"); + return false; + } + + return true; + } +} diff --git a/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/StructuredTraceGraphBuilder.java b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/StructuredTraceGraphBuilder.java index d8d208ecc..f6e25e581 100644 --- a/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/StructuredTraceGraphBuilder.java +++ b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/StructuredTraceGraphBuilder.java @@ -1,52 +1,32 @@ package org.hypertrace.traceenricher.trace.util; +import java.time.Duration; +import java.time.Instant; +import java.util.concurrent.TimeUnit; import org.hypertrace.core.datamodel.StructuredTrace; import org.hypertrace.core.datamodel.shared.StructuredTraceGraph; import org.slf4j.Logger; import org.slf4j.LoggerFactory; public class StructuredTraceGraphBuilder { - private static final Logger LOG = LoggerFactory.getLogger(StructuredTraceGraphBuilder.class); + public static final Logger LOG = LoggerFactory.getLogger(StructuredTraceGraphBuilder.class); private static ThreadLocal cachedGraph = new ThreadLocal<>(); private static ThreadLocal cachedTrace = new ThreadLocal<>(); public static StructuredTraceGraph buildGraph(StructuredTrace trace) { - // trace doesn't exist - if (cachedTrace.get() == null) { - LOG.debug("Building structured trace graph. Reason: no cached trace"); + if (!GraphBuilderUtil.isSameStructuredTrace(cachedTrace.get(), trace)) { + Instant start = Instant.now(); StructuredTraceGraph graph = StructuredTraceGraph.createGraph(trace); - cachedTrace.set(StructuredTrace.newBuilder(trace).build()); - cachedGraph.set(graph); - return graph; - } - - // is processed and cached are same trace? - if (!cachedTrace.get().getCustomerId().equals(trace.getCustomerId()) - || !cachedTrace.get().getTraceId().equals(trace.getTraceId())) { - LOG.debug( - "Building structured trace graph. Reason: cached trace and current trace doesn't not match"); - StructuredTraceGraph graph = StructuredTraceGraph.createGraph(trace); - cachedTrace.set(StructuredTrace.newBuilder(trace).build()); - cachedGraph.set(graph); - return graph; - } - - // trace internally changed - if (cachedTrace.get().getEntityList().size() != trace.getEntityList().size() - || cachedTrace.get().getEventList().size() != trace.getEventList().size() - || cachedTrace.get().getEntityEdgeList().size() != trace.getEntityEdgeList().size() - || cachedTrace.get().getEntityEventEdgeList().size() - != trace.getEntityEventEdgeList().size() - || cachedTrace.get().getEventEdgeList().size() != trace.getEventEdgeList().size()) { LOG.debug( - "Building structured trace graph. Reason: cached trace and current trace have different size"); - StructuredTraceGraph graph = StructuredTraceGraph.createGraph(trace); + "Time taken in building StructuredTraceGraph:{} for tenantId:{}", + Duration.between(start, Instant.now()).toMillis(), + TimeUnit.MILLISECONDS, + trace.getCustomerId()); cachedTrace.set(StructuredTrace.newBuilder(trace).build()); cachedGraph.set(graph); return graph; } - return cachedGraph.get(); } } diff --git a/hypertrace-trace-enricher/hypertrace-trace-enricher-impl/src/main/java/org/hypertrace/traceenricher/enrichment/enrichers/ErrorsAndExceptionsEnricher.java b/hypertrace-trace-enricher/hypertrace-trace-enricher-impl/src/main/java/org/hypertrace/traceenricher/enrichment/enrichers/ErrorsAndExceptionsEnricher.java index d9d5d6e3b..751a9d3f6 100644 --- a/hypertrace-trace-enricher/hypertrace-trace-enricher-impl/src/main/java/org/hypertrace/traceenricher/enrichment/enrichers/ErrorsAndExceptionsEnricher.java +++ b/hypertrace-trace-enricher/hypertrace-trace-enricher-impl/src/main/java/org/hypertrace/traceenricher/enrichment/enrichers/ErrorsAndExceptionsEnricher.java @@ -18,6 +18,7 @@ import org.hypertrace.traceenricher.enrichedspan.constants.v1.ErrorMetrics; import org.hypertrace.traceenricher.enrichment.AbstractTraceEnricher; import org.hypertrace.traceenricher.trace.util.ApiTraceGraph; +import org.hypertrace.traceenricher.trace.util.ApiTraceGraphBuilder; import org.hypertrace.traceenricher.util.Constants; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -98,7 +99,7 @@ public void enrichTrace(StructuredTrace trace) { // has errored out but the entry span in transaction might be fine (server responded but // client couldn't process it). Those cases should be handled in future. - ApiTraceGraph apiTraceGraph = new ApiTraceGraph(trace); + ApiTraceGraph apiTraceGraph = ApiTraceGraphBuilder.buildGraph(trace); for (ApiNode apiNode : apiTraceGraph.getApiNodeList()) { Optional entryEvent = apiNode.getEntryApiBoundaryEvent(); int apiTraceErrorCount = diff --git a/hypertrace-trace-enricher/hypertrace-trace-enricher-impl/src/main/java/org/hypertrace/traceenricher/enrichment/enrichers/ExitCallsEnricher.java b/hypertrace-trace-enricher/hypertrace-trace-enricher-impl/src/main/java/org/hypertrace/traceenricher/enrichment/enrichers/ExitCallsEnricher.java index d2bed5a0b..37b7a2224 100644 --- a/hypertrace-trace-enricher/hypertrace-trace-enricher-impl/src/main/java/org/hypertrace/traceenricher/enrichment/enrichers/ExitCallsEnricher.java +++ b/hypertrace-trace-enricher/hypertrace-trace-enricher-impl/src/main/java/org/hypertrace/traceenricher/enrichment/enrichers/ExitCallsEnricher.java @@ -18,6 +18,7 @@ import org.hypertrace.traceenricher.enrichedspan.constants.utils.EnrichedSpanUtils; import org.hypertrace.traceenricher.enrichment.AbstractTraceEnricher; import org.hypertrace.traceenricher.trace.util.ApiTraceGraph; +import org.hypertrace.traceenricher.trace.util.ApiTraceGraphBuilder; public class ExitCallsEnricher extends AbstractTraceEnricher { @@ -56,7 +57,7 @@ public void enrichTrace(StructuredTrace trace) { * */ Map computeApiExitCallCount(StructuredTrace trace) { - ApiTraceGraph apiTraceGraph = new ApiTraceGraph(trace); + ApiTraceGraph apiTraceGraph = ApiTraceGraphBuilder.buildGraph(trace); // event -> api exit call count for the corresponding api_node Map eventToExitInfo = Maps.newHashMap(); diff --git a/hypertrace-trace-enricher/hypertrace-trace-enricher-impl/src/main/java/org/hypertrace/traceenricher/enrichment/enrichers/endpoint/EndpointEnricher.java b/hypertrace-trace-enricher/hypertrace-trace-enricher-impl/src/main/java/org/hypertrace/traceenricher/enrichment/enrichers/endpoint/EndpointEnricher.java index f19cb6322..b5640c2f7 100644 --- a/hypertrace-trace-enricher/hypertrace-trace-enricher-impl/src/main/java/org/hypertrace/traceenricher/enrichment/enrichers/endpoint/EndpointEnricher.java +++ b/hypertrace-trace-enricher/hypertrace-trace-enricher-impl/src/main/java/org/hypertrace/traceenricher/enrichment/enrichers/endpoint/EndpointEnricher.java @@ -19,7 +19,7 @@ import org.hypertrace.traceenricher.enrichedspan.constants.utils.EnrichedSpanUtils; import org.hypertrace.traceenricher.enrichment.AbstractTraceEnricher; import org.hypertrace.traceenricher.enrichment.clients.ClientRegistry; -import org.hypertrace.traceenricher.trace.util.ApiTraceGraph; +import org.hypertrace.traceenricher.trace.util.ApiTraceGraphBuilder; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -155,7 +155,7 @@ private OperationNameBasedEndpointDiscoverer getOperationNameBasedEndpointDiscov */ @Override public void enrichTrace(StructuredTrace trace) { - List> apiNodes = new ApiTraceGraph(trace).getApiNodeList(); + List> apiNodes = ApiTraceGraphBuilder.buildGraph(trace).getApiNodeList(); for (ApiNode apiNode : apiNodes) { Optional optionalEvent = apiNode.getEntryApiBoundaryEvent(); if (optionalEvent.isEmpty()) { diff --git a/hypertrace-trace-enricher/hypertrace-trace-enricher-impl/src/test/java/org/hypertrace/traceenricher/enrichment/enrichers/ExitCallsEnricherTest.java b/hypertrace-trace-enricher/hypertrace-trace-enricher-impl/src/test/java/org/hypertrace/traceenricher/enrichment/enrichers/ExitCallsEnricherTest.java index 95d82a287..1366215fa 100644 --- a/hypertrace-trace-enricher/hypertrace-trace-enricher-impl/src/test/java/org/hypertrace/traceenricher/enrichment/enrichers/ExitCallsEnricherTest.java +++ b/hypertrace-trace-enricher/hypertrace-trace-enricher-impl/src/test/java/org/hypertrace/traceenricher/enrichment/enrichers/ExitCallsEnricherTest.java @@ -20,6 +20,7 @@ import org.hypertrace.traceenricher.enrichedspan.constants.utils.EnrichedSpanUtils; import org.hypertrace.traceenricher.enrichment.enrichers.ExitCallsEnricher.ApiExitCallInfo; import org.hypertrace.traceenricher.trace.util.ApiTraceGraph; +import org.hypertrace.traceenricher.trace.util.ApiTraceGraphBuilder; import org.junit.jupiter.api.Test; public class ExitCallsEnricherTest { @@ -42,7 +43,7 @@ public void testEnrichTrace_HotrodTrace() throws IOException { private void verifyComputeApiExitInfo_HotrodTrace( StructuredTrace trace, ExitCallsEnricher exitCallsEnricher) { - ApiTraceGraph apiTraceGraph = new ApiTraceGraph(trace); + ApiTraceGraph apiTraceGraph = ApiTraceGraphBuilder.buildGraph(trace); // this trace has 12 api nodes // api edges // 0 -> [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12] From d4b96f1e1ff7e78b16cde474f65f323c0f394ceb Mon Sep 17 00:00:00 2001 From: Ronak Date: Fri, 21 May 2021 13:57:56 +0530 Subject: [PATCH 2/6] chore: addresses the access modifier for logger to private --- .../traceenricher/trace/util/ApiTraceGraphBuilder.java | 2 +- .../hypertrace/traceenricher/trace/util/GraphBuilderUtil.java | 2 +- .../traceenricher/trace/util/StructuredTraceGraphBuilder.java | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/ApiTraceGraphBuilder.java b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/ApiTraceGraphBuilder.java index 9123df517..564feaafd 100644 --- a/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/ApiTraceGraphBuilder.java +++ b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/ApiTraceGraphBuilder.java @@ -8,7 +8,7 @@ import org.slf4j.LoggerFactory; public class ApiTraceGraphBuilder { - public static final Logger LOG = LoggerFactory.getLogger(ApiTraceGraphBuilder.class); + private static final Logger LOG = LoggerFactory.getLogger(ApiTraceGraphBuilder.class); private static ThreadLocal cachedGraph = new ThreadLocal<>(); private static ThreadLocal cachedTrace = new ThreadLocal<>(); diff --git a/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/GraphBuilderUtil.java b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/GraphBuilderUtil.java index 48e4219d0..13e689d32 100644 --- a/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/GraphBuilderUtil.java +++ b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/GraphBuilderUtil.java @@ -5,7 +5,7 @@ import org.slf4j.LoggerFactory; public class GraphBuilderUtil { - public static final Logger LOG = LoggerFactory.getLogger(GraphBuilderUtil.class); + private static final Logger LOG = LoggerFactory.getLogger(GraphBuilderUtil.class); /** * optimistic method of comparing two trace for considering rebuilding of entire graph structure. diff --git a/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/StructuredTraceGraphBuilder.java b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/StructuredTraceGraphBuilder.java index f6e25e581..dd1c057fd 100644 --- a/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/StructuredTraceGraphBuilder.java +++ b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/StructuredTraceGraphBuilder.java @@ -9,7 +9,7 @@ import org.slf4j.LoggerFactory; public class StructuredTraceGraphBuilder { - public static final Logger LOG = LoggerFactory.getLogger(StructuredTraceGraphBuilder.class); + private static final Logger LOG = LoggerFactory.getLogger(StructuredTraceGraphBuilder.class); private static ThreadLocal cachedGraph = new ThreadLocal<>(); private static ThreadLocal cachedTrace = new ThreadLocal<>(); From d7a49b9f8f1c362b71e806deeeb8a3a8164bfe0c Mon Sep 17 00:00:00 2001 From: Ronak Date: Fri, 21 May 2021 13:59:32 +0530 Subject: [PATCH 3/6] chore: reverted the temporal change in test case --- .../enrichment/enrichers/ExitCallsEnricherTest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hypertrace-trace-enricher/hypertrace-trace-enricher-impl/src/test/java/org/hypertrace/traceenricher/enrichment/enrichers/ExitCallsEnricherTest.java b/hypertrace-trace-enricher/hypertrace-trace-enricher-impl/src/test/java/org/hypertrace/traceenricher/enrichment/enrichers/ExitCallsEnricherTest.java index 1366215fa..95d82a287 100644 --- a/hypertrace-trace-enricher/hypertrace-trace-enricher-impl/src/test/java/org/hypertrace/traceenricher/enrichment/enrichers/ExitCallsEnricherTest.java +++ b/hypertrace-trace-enricher/hypertrace-trace-enricher-impl/src/test/java/org/hypertrace/traceenricher/enrichment/enrichers/ExitCallsEnricherTest.java @@ -20,7 +20,6 @@ import org.hypertrace.traceenricher.enrichedspan.constants.utils.EnrichedSpanUtils; import org.hypertrace.traceenricher.enrichment.enrichers.ExitCallsEnricher.ApiExitCallInfo; import org.hypertrace.traceenricher.trace.util.ApiTraceGraph; -import org.hypertrace.traceenricher.trace.util.ApiTraceGraphBuilder; import org.junit.jupiter.api.Test; public class ExitCallsEnricherTest { @@ -43,7 +42,7 @@ public void testEnrichTrace_HotrodTrace() throws IOException { private void verifyComputeApiExitInfo_HotrodTrace( StructuredTrace trace, ExitCallsEnricher exitCallsEnricher) { - ApiTraceGraph apiTraceGraph = ApiTraceGraphBuilder.buildGraph(trace); + ApiTraceGraph apiTraceGraph = new ApiTraceGraph(trace); // this trace has 12 api nodes // api edges // 0 -> [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12] From 98a801d610a77ac30128d56d556070f903ac6234 Mon Sep 17 00:00:00 2001 From: Ronak Date: Mon, 24 May 2021 14:11:10 +0530 Subject: [PATCH 4/6] fix: adds unit tests for newly added class --- .../build.gradle.kts | 2 + .../trace/util/ApiTraceGraphBuilderTest.java | 62 +++++++++++++ .../trace/util/GraphBuilderUtilTest.java | 88 +++++++++++++++++++ .../util/StructuredTraceGraphBuilderTest.java | 67 ++++++++++++++ 4 files changed, 219 insertions(+) create mode 100644 hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/ApiTraceGraphBuilderTest.java create mode 100644 hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/GraphBuilderUtilTest.java create mode 100644 hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/StructuredTraceGraphBuilderTest.java diff --git a/hypertrace-trace-enricher/hypertrace-trace-enricher-api/build.gradle.kts b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/build.gradle.kts index 25528556f..6c4b22197 100644 --- a/hypertrace-trace-enricher/hypertrace-trace-enricher-api/build.gradle.kts +++ b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/build.gradle.kts @@ -14,4 +14,6 @@ dependencies { implementation("org.slf4j:slf4j-api:1.7.30") implementation("org.apache.commons:commons-lang3:3.12.0") testImplementation("org.junit.jupiter:junit-jupiter:5.7.1") + testImplementation("org.mockito:mockito-core:3.9.0") + testImplementation("org.mockito:mockito-inline:3.9.0") } diff --git a/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/ApiTraceGraphBuilderTest.java b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/ApiTraceGraphBuilderTest.java new file mode 100644 index 000000000..0827e609c --- /dev/null +++ b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/ApiTraceGraphBuilderTest.java @@ -0,0 +1,62 @@ +package org.hypertrace.traceenricher.trace.util; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockConstruction; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.when; + +import java.nio.ByteBuffer; +import java.util.List; +import org.hypertrace.core.datamodel.Edge; +import org.hypertrace.core.datamodel.Entity; +import org.hypertrace.core.datamodel.Event; +import org.hypertrace.core.datamodel.StructuredTrace; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.mockito.MockedConstruction; +import org.mockito.MockedStatic; + +public class ApiTraceGraphBuilderTest { + + @Test + void testBuildGraph() { + + Entity entity = mock(Entity.class); + Event parent = mock(Event.class); + Event child = mock(Event.class); + Edge eventEdge = mock(Edge.class); + + StructuredTrace underTestTrace = mock(StructuredTrace.class); + when(underTestTrace.getCustomerId()).thenReturn("__defaultTenant"); + when(underTestTrace.getTraceId()).thenReturn(ByteBuffer.wrap("2ebbc19b6428510f".getBytes())); + when(underTestTrace.getEntityList()).thenReturn(List.of(entity)); + when(underTestTrace.getEventList()).thenReturn(List.of(parent, child)); + when(underTestTrace.getEntityEdgeList()).thenReturn(List.of()); + when(underTestTrace.getEntityEventEdgeList()).thenReturn(List.of()); + when(underTestTrace.getEventEdgeList()).thenReturn(List.of(eventEdge)); + + // structure trace builder + try (MockedStatic builderMockedStatic = mockStatic(StructuredTrace.class)) { + StructuredTrace.Builder builder = mock(StructuredTrace.Builder.class); + when(builder.build()).thenReturn(underTestTrace); + + builderMockedStatic + .when(() -> StructuredTrace.newBuilder(underTestTrace)) + .thenReturn(builder); + + // check if we get a new + try (MockedConstruction mockedConstruction = + mockConstruction(ApiTraceGraph.class)) { + // first call + ApiTraceGraph actual = ApiTraceGraphBuilder.buildGraph(underTestTrace); + Assertions.assertNotNull(actual); + Assertions.assertEquals(1, mockedConstruction.constructed().size()); + + // second call + ApiTraceGraph second = ApiTraceGraphBuilder.buildGraph(underTestTrace); + Assertions.assertEquals(actual, second); + Assertions.assertEquals(1, mockedConstruction.constructed().size()); + } + } + } +} diff --git a/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/GraphBuilderUtilTest.java b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/GraphBuilderUtilTest.java new file mode 100644 index 000000000..2977dfe3e --- /dev/null +++ b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/GraphBuilderUtilTest.java @@ -0,0 +1,88 @@ +package org.hypertrace.traceenricher.trace.util; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.nio.ByteBuffer; +import java.util.List; +import org.hypertrace.core.datamodel.Edge; +import org.hypertrace.core.datamodel.Entity; +import org.hypertrace.core.datamodel.Event; +import org.hypertrace.core.datamodel.StructuredTrace; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +public class GraphBuilderUtilTest { + + @Test + public void testSameTraceForNullInputs() { + StructuredTrace cachedTrace = mock(StructuredTrace.class); + StructuredTrace underTestTrace = mock(StructuredTrace.class); + + boolean result = GraphBuilderUtil.isSameStructuredTrace(null, null); + Assertions.assertFalse(result); + + result = GraphBuilderUtil.isSameStructuredTrace(null, underTestTrace); + Assertions.assertFalse(result); + + result = GraphBuilderUtil.isSameStructuredTrace(cachedTrace, null); + Assertions.assertFalse(result); + } + + @Test + public void testSameTraceForTenantAndTraceCondition() { + // different tenant id + StructuredTrace cachedTrace = mock(StructuredTrace.class); + when(cachedTrace.getCustomerId()).thenReturn("__defaultTenant"); + when(cachedTrace.getTraceId()).thenReturn(ByteBuffer.wrap("2ebbc19b6428510f".getBytes())); + + StructuredTrace underTestTrace = mock(StructuredTrace.class); + when(underTestTrace.getCustomerId()).thenReturn("__defaultTenantUnderTest"); + when(underTestTrace.getTraceId()).thenReturn(ByteBuffer.wrap("2ebbc19b6428510f".getBytes())); + + boolean result = GraphBuilderUtil.isSameStructuredTrace(cachedTrace, underTestTrace); + Assertions.assertFalse(result); + + // different trace ids + cachedTrace = mock(StructuredTrace.class); + when(cachedTrace.getCustomerId()).thenReturn("__defaultTenant"); + when(cachedTrace.getTraceId()).thenReturn(ByteBuffer.wrap("2ebbc19b6428510f".getBytes())); + + underTestTrace = mock(StructuredTrace.class); + when(underTestTrace.getCustomerId()).thenReturn("__defaultTenantUnder"); + when(underTestTrace.getTraceId()).thenReturn(ByteBuffer.wrap("2ebbc19b6428511f".getBytes())); + + result = GraphBuilderUtil.isSameStructuredTrace(cachedTrace, underTestTrace); + Assertions.assertFalse(result); + } + + @Test + public void testSameTraceForSizeCondition() { + Entity entity = mock(Entity.class); + Event parent = mock(Event.class); + Event child = mock(Event.class); + Edge eventEdge = mock(Edge.class); + + // same size + StructuredTrace cachedTrace = mock(StructuredTrace.class); + when(cachedTrace.getCustomerId()).thenReturn("__defaultTenant"); + when(cachedTrace.getTraceId()).thenReturn(ByteBuffer.wrap("2ebbc19b6428510f".getBytes())); + when(cachedTrace.getEntityList()).thenReturn(List.of(entity)); + when(cachedTrace.getEventList()).thenReturn(List.of(parent, child)); + when(cachedTrace.getEntityEdgeList()).thenReturn(List.of()); + when(cachedTrace.getEntityEventEdgeList()).thenReturn(List.of()); + when(cachedTrace.getEventEdgeList()).thenReturn(List.of(eventEdge)); + + StructuredTrace underTestTrace = mock(StructuredTrace.class); + when(underTestTrace.getCustomerId()).thenReturn("__defaultTenant"); + when(underTestTrace.getTraceId()).thenReturn(ByteBuffer.wrap("2ebbc19b6428510f".getBytes())); + when(underTestTrace.getEntityList()).thenReturn(List.of(entity)); + when(underTestTrace.getEventList()).thenReturn(List.of(parent, child)); + when(underTestTrace.getEntityEdgeList()).thenReturn(List.of()); + when(underTestTrace.getEntityEventEdgeList()).thenReturn(List.of()); + when(underTestTrace.getEventEdgeList()).thenReturn(List.of(eventEdge)); + + boolean result = GraphBuilderUtil.isSameStructuredTrace(cachedTrace, underTestTrace); + Assertions.assertTrue(result); + } +} diff --git a/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/StructuredTraceGraphBuilderTest.java b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/StructuredTraceGraphBuilderTest.java new file mode 100644 index 000000000..8ea84dbe9 --- /dev/null +++ b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/StructuredTraceGraphBuilderTest.java @@ -0,0 +1,67 @@ +package org.hypertrace.traceenricher.trace.util; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.when; + +import java.nio.ByteBuffer; +import java.util.List; +import org.hypertrace.core.datamodel.Edge; +import org.hypertrace.core.datamodel.Entity; +import org.hypertrace.core.datamodel.Event; +import org.hypertrace.core.datamodel.StructuredTrace; +import org.hypertrace.core.datamodel.shared.StructuredTraceGraph; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.mockito.MockedStatic; + +public class StructuredTraceGraphBuilderTest { + + @Test + void testBuildGraph() { + + Entity entity = mock(Entity.class); + Event parent = mock(Event.class); + Event child = mock(Event.class); + Edge eventEdge = mock(Edge.class); + + StructuredTrace underTestTrace = mock(StructuredTrace.class); + when(underTestTrace.getCustomerId()).thenReturn("__defaultTenant"); + when(underTestTrace.getTraceId()).thenReturn(ByteBuffer.wrap("2ebbc19b6428510f".getBytes())); + when(underTestTrace.getEntityList()).thenReturn(List.of(entity)); + when(underTestTrace.getEventList()).thenReturn(List.of(parent, child)); + when(underTestTrace.getEntityEdgeList()).thenReturn(List.of()); + when(underTestTrace.getEntityEventEdgeList()).thenReturn(List.of()); + when(underTestTrace.getEventEdgeList()).thenReturn(List.of(eventEdge)); + + try (MockedStatic builderMockedStatic = mockStatic(StructuredTrace.class)) { + + StructuredTrace.Builder builder = mock(StructuredTrace.Builder.class); + when(builder.build()).thenReturn(underTestTrace); + + builderMockedStatic + .when(() -> StructuredTrace.newBuilder(underTestTrace)) + .thenReturn(builder); + + // mock createGraph method of structured trace graph + StructuredTraceGraph mockedStructuredTraceGraph = mock(StructuredTraceGraph.class); + + try (MockedStatic structuredTraceGraphMockedStatic = + mockStatic(StructuredTraceGraph.class)) { + structuredTraceGraphMockedStatic + .when(() -> StructuredTraceGraph.createGraph(underTestTrace)) + .thenReturn(mockedStructuredTraceGraph); + + // calls first time + StructuredTraceGraph actual = StructuredTraceGraphBuilder.buildGraph(underTestTrace); + Assertions.assertEquals(mockedStructuredTraceGraph, actual); + + // calls second time, this time it returns from cache + StructuredTraceGraphBuilder.buildGraph(underTestTrace); + structuredTraceGraphMockedStatic.verify( + () -> StructuredTraceGraph.createGraph(underTestTrace), times(1)); + } + } + } +} From 07d3e0ea27890c1e849de9fa719cd6e8c955ebe2 Mon Sep 17 00:00:00 2001 From: Ronak Date: Mon, 24 May 2021 14:15:32 +0530 Subject: [PATCH 5/6] chore: address few formatting --- .../traceenricher/trace/util/ApiTraceGraphBuilderTest.java | 2 +- .../traceenricher/trace/util/GraphBuilderUtilTest.java | 2 +- .../trace/util/StructuredTraceGraphBuilderTest.java | 3 +-- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/ApiTraceGraphBuilderTest.java b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/ApiTraceGraphBuilderTest.java index 0827e609c..283201f69 100644 --- a/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/ApiTraceGraphBuilderTest.java +++ b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/ApiTraceGraphBuilderTest.java @@ -44,7 +44,7 @@ void testBuildGraph() { .when(() -> StructuredTrace.newBuilder(underTestTrace)) .thenReturn(builder); - // check if we get a new + // make two calls, and check that first call create cache entries, and second call uses same try (MockedConstruction mockedConstruction = mockConstruction(ApiTraceGraph.class)) { // first call diff --git a/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/GraphBuilderUtilTest.java b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/GraphBuilderUtilTest.java index 2977dfe3e..f502a9b14 100644 --- a/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/GraphBuilderUtilTest.java +++ b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/GraphBuilderUtilTest.java @@ -49,7 +49,7 @@ public void testSameTraceForTenantAndTraceCondition() { when(cachedTrace.getTraceId()).thenReturn(ByteBuffer.wrap("2ebbc19b6428510f".getBytes())); underTestTrace = mock(StructuredTrace.class); - when(underTestTrace.getCustomerId()).thenReturn("__defaultTenantUnder"); + when(underTestTrace.getCustomerId()).thenReturn("__defaultTenant"); when(underTestTrace.getTraceId()).thenReturn(ByteBuffer.wrap("2ebbc19b6428511f".getBytes())); result = GraphBuilderUtil.isSameStructuredTrace(cachedTrace, underTestTrace); diff --git a/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/StructuredTraceGraphBuilderTest.java b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/StructuredTraceGraphBuilderTest.java index 8ea84dbe9..56aa66f72 100644 --- a/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/StructuredTraceGraphBuilderTest.java +++ b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/StructuredTraceGraphBuilderTest.java @@ -44,9 +44,8 @@ void testBuildGraph() { .when(() -> StructuredTrace.newBuilder(underTestTrace)) .thenReturn(builder); - // mock createGraph method of structured trace graph + // make two calls, and check that first call create cache entries, and second call uses same StructuredTraceGraph mockedStructuredTraceGraph = mock(StructuredTraceGraph.class); - try (MockedStatic structuredTraceGraphMockedStatic = mockStatic(StructuredTraceGraph.class)) { structuredTraceGraphMockedStatic From 3d9b21d010bcbda9ae0db47dc2d6701949bb4ac8 Mon Sep 17 00:00:00 2001 From: Ronak Date: Mon, 24 May 2021 16:05:41 +0530 Subject: [PATCH 6/6] address comments --- .../trace/util/StructuredTraceGraphBuilderTest.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/StructuredTraceGraphBuilderTest.java b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/StructuredTraceGraphBuilderTest.java index 56aa66f72..509b52b18 100644 --- a/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/StructuredTraceGraphBuilderTest.java +++ b/hypertrace-trace-enricher/hypertrace-trace-enricher-api/src/test/java/org/hypertrace/traceenricher/trace/util/StructuredTraceGraphBuilderTest.java @@ -55,6 +55,8 @@ void testBuildGraph() { // calls first time StructuredTraceGraph actual = StructuredTraceGraphBuilder.buildGraph(underTestTrace); Assertions.assertEquals(mockedStructuredTraceGraph, actual); + structuredTraceGraphMockedStatic.verify( + () -> StructuredTraceGraph.createGraph(underTestTrace), times(1)); // calls second time, this time it returns from cache StructuredTraceGraphBuilder.buildGraph(underTestTrace);