Skip to content

Mark span as APIBoundary if service name differs from parent service … #221

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Jul 15, 2021
Merged

Mark span as APIBoundary if service name differs from parent service … #221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
625a7e7
Mark span as APIBoundary if service name differs from parent service …
pavan-traceable Jul 11, 2021
57016e7
Updated documentation
pavan-traceable Jul 11, 2021
c0eed4a
Updated doc
pavan-traceable Jul 11, 2021
8d949f5
Merge branch 'main' into api-boundary-fix
pavan-traceable Jul 15, 2021
ebcb730
Add edge between entry services
pavan-traceable Jul 15, 2021
060f5eb
Updated doc
pavan-traceable Jul 15, 2021
034ffff
Fix logging
pavan-traceable Jul 15, 2021
28b5880
Moved utility to EnrichedSpanUtils
pavan-traceable Jul 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions ...ain/java/org/hypertrace/traceenricher/enrichedspan/constants/utils/EnrichedSpanUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import java.util.Map;
import java.util.Optional;
import org.apache.avro.reflect.Nullable;
import org.apache.commons.codec.binary.StringUtils;
import org.hypertrace.core.datamodel.AttributeValue;
import org.hypertrace.core.datamodel.Event;
import org.hypertrace.core.datamodel.shared.SpanAttributeUtils;
Expand Down Expand Up @@ -368,4 +369,12 @@ public static List<String> getSpaceIds(Event event) {
.map(AttributeValue::getValueList)
.orElseGet(Collections::emptyList);
}

/** Check whether these spans belongs to different services. */
public static boolean areBothSpansFromDifferentService(Event event, Event parentEvent) {
if (event.getServiceName() == null || parentEvent.getServiceName() == null) {
return false;
}
return !StringUtils.equals(event.getServiceName(), parentEvent.getServiceName());
}
}
43 changes: 39 additions & 4 deletions ...ace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/ApiTraceGraph.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,12 +272,11 @@ private ApiNode<Event> buildApiNode(StructuredTraceGraph graph, Event rootEvent)

private void buildApiNodeEdges(StructuredTraceGraph graph) {
// 1. get all the exit boundary events from an api node
// 2. exit boundary events are the only ones which can call a different api node
// 3. find all the children of exit boundary events, which will be entry boundary nodes of
// 2. find all the children of exit boundary events, which will be entry boundary nodes of
Copy link
Contributor

@avinashkolluru avinashkolluru Jul 15, 2021

Choose a reason for hiding this comment

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

nit: find all the children of exit boundary events is not accurate since we check the children of entry boundary events too now

Copy link
Contributor Author

@pavan-traceable pavan-traceable Jul 15, 2021

Choose a reason for hiding this comment

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

Other case updated later in the method, this is the first case.

// different api nodes
// 4. find all the api nodes based on children of exit boundary events from
// 3. find all the api nodes based on children of exit boundary events from
// `entryBoundaryToApiNode`
// 5. connect the exit boundary and entry boundary of different api node with an edge
// 4. connect the exit boundary and entry boundary of different api node with an edge
for (ApiNode<Event> apiNode : apiNodeList) {
// exit boundary events of api node
List<Event> exitBoundaryEvents = apiNode.getExitApiBoundaryEvents();
Expand Down Expand Up @@ -325,6 +324,42 @@ private void buildApiNodeEdges(StructuredTraceGraph graph) {
}
}
}
// Sometimes an exit span might be missing for services like Istio, Kong.
// Only Entry spans will be populated for these services,
// an edge must be created between these services as well.
// 1. get entry boundary event from an api node.
// 2. find all the children of entry boundary event, which will be entry boundary nodes of
// different api nodes
// 3. Check both parent span and child belong to different service as well.
// 4. connect the entry boundary of different api nodes with an edge.
Optional<Event> entryBoundaryEvent = apiNode.getEntryApiBoundaryEvent();
if (entryBoundaryEvent.isPresent()) {
List<Event> children = graph.getChildrenEvents(entryBoundaryEvent.get());
if (children != null) {
for (Event child : children) {
// if the child of an entry boundary event is an entry api boundary type,
// which can happen if exit span missing and both belongs to different services.
if (EnrichedSpanUtils.isEntryApiBoundary(child)
Copy link
Contributor

@findingrish findingrish Jul 15, 2021

Choose a reason for hiding this comment

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

if the exit span is missing how will these two span have parent and child relationship?

Copy link
Contributor

@findingrish findingrish Jul 15, 2021

Choose a reason for hiding this comment

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

Ignore this, I was assuming that an actual span is missing here which is not the case

&& EnrichedSpanUtils.areBothSpansFromDifferentService(
child, entryBoundaryEvent.get())) {
ApiNode<Event> destinationApiNode = entryApiBoundaryEventIdToApiNode.get(child);
if (LOGGER.isDebugEnabled()) {
LOGGER.debug(
"Edge between entry boundaries servicename: {} span: {} to servicename: {} span: {} of trace {}",
entryBoundaryEvent.get().getServiceName(),
HexUtils.getHex(entryBoundaryEvent.get().getEventId()),
child.getServiceName(),
HexUtils.getHex(child.getEventId()),
HexUtils.getHex(trace.getTraceId()));
}
Optional<ApiNodeEventEdge> edgeBetweenApiNodes =
createEdgeBetweenApiNodes(
apiNode, destinationApiNode, entryBoundaryEvent.get(), child);
edgeBetweenApiNodes.ifPresent(apiNodeEventEdgeList::add);
}
}
}
}
}
}

Expand Down
5 changes: 3 additions & 2 deletions ...a/org/hypertrace/traceenricher/enrichment/enrichers/ApiBoundaryTypeAttributeEnricher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,12 @@ public void enrichEvent(StructuredTrace trace, Event event) {
/*
Determines if this is entry span is an entry to api
1. If the event is an ENTRY span, and
2. If the direct parent span is either an EXIT Span or Null, then this is the entry point.
2. If the direct parent span is either an EXIT Span or Null, or direct parent span service name differs from current span service name.
*/

Event parentEvent = graph.getParentEvent(event);
if (!EnrichedSpanUtils.isEntrySpan(parentEvent)) {
if (!EnrichedSpanUtils.isEntrySpan(parentEvent)
Copy link
Contributor

@tim-mwangi tim-mwangi Jul 11, 2021

Choose a reason for hiding this comment

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

Here's a hypothetical: in scenarios where an envoy proxy is injected, usually there will be 2 entry spans back to back and for exits, 2 exit spans back to back. What happens then when the proxy labels the service as "test-service" and the in-app agent labels it with "test-service-foo". Technically, all these spans are in one service, given that a pod is considered as the service but now there will be 2 API boundaries instead of just one.

Copy link
Contributor Author

@pavan-traceable pavan-traceable Jul 12, 2021

Choose a reason for hiding this comment

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

Thanks @tim-mwangi. Is it not possible to fix this ? This change mainly because of multiple gateway spans like Kong, Istio etc which we don't have any control. @davexroth what should we do ?

Copy link
Contributor

@kotharironak kotharironak Jul 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Do we have any attributes from Kong, Istio that indicate that span is proxySpan - e.g tracer.type?

Copy link
Contributor

@kotharironak kotharironak Jul 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

As the boundary between two services (which will have different service labels respectively) are based on (entry, exit) pair, is it safe to assume that two consecutive (entry, entry) or (exit, exit) pairs showing different service labels can possible due to proxy?

If, yes, we can consider the below logic for determining entry/exit proxySpan if tracer.type attribute (or some other attribute indicating proxySpan) is not available:

isEntryProxySpan(currentSpan):
    isEntrySpan(currentSpan AND (foreach child of (currentSpan) -> isEntrySpan(child.span) && child.service() != currentSpan.service()) 

isExitProxySpan():
    isExistSpan() AND (isParentSpanExitSpan() && parent.serviceName != this.span.serviceName())

Should we decorate at proxySpan level or inner service span level for entry API boundary?

  • Currently, on exit span, we are decorating at the proxy span level. So, for entry span, we can do the same, decorating at proxy span level.

we will need merging of spans (proxy + service span) as dependent enrichers eventually if we go with the above logic.

Copy link
Contributor

@tim-mwangi tim-mwangi Jul 13, 2021

Choose a reason for hiding this comment

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

Discussed offline. I withdraw my concerns. I'm ok with multiple API boundaries if the service name changes.

Copy link
Contributor Author

@pavan-traceable pavan-traceable Jul 15, 2021

Choose a reason for hiding this comment

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

@kotharironak Had offline discussion with Dave and Tim, they have agreed with this change.

|| EnrichedSpanUtils.areBothSpansFromDifferentService(event, parentEvent)) {
addEnrichedAttribute(
event, API_BOUNDARY_TYPE_ATTR_NAME, AttributeValueCreator.create(ENTRY_BOUNDARY_TYPE));

Expand Down
1 change: 1 addition & 0 deletions ...java/org/hypertrace/traceenricher/enrichment/enrichers/AbstractAttributeEnricherTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ Event createMockEvent() {
lenient()
.when(e.getMetrics())
.thenReturn(Metrics.newBuilder().setMetricMap(new HashMap<>()).build());
when(e.getServiceName()).thenReturn("service");
return e;
}

Expand Down
18 changes: 18 additions & 0 deletions ...g/hypertrace/traceenricher/enrichment/enrichers/ApiBoundaryTypeAttributeEnricherTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,24 @@ public void test_enrichEvent_doubleEntryExitSpan_markTheOuterOneAsApiBoundary()
Constants.getEnrichedSpanConstant(EXIT));
}

@Test
public void test_enrichEvent_doubleEntrySpan_differentServiceName() {
// OuterEntry -> inner Entry -> inner Exit -> outer Exit
mockDoubleEntryStructuredGraph();
target.enrichEvent(trace, outerEntrySpan);
Assertions.assertEquals(
EnrichedSpanUtils.getApiBoundaryType(outerEntrySpan),
Constants.getEnrichedSpanConstant(ENTRY));
target.enrichEvent(trace, innerEntrySpan);
Assertions.assertNull(EnrichedSpanUtils.getApiBoundaryType(innerEntrySpan));
mockDoubleEntryStructuredGraph();
when(innerEntrySpan.getServiceName()).thenReturn("service2");
target.enrichEvent(trace, innerEntrySpan);
Assertions.assertEquals(
EnrichedSpanUtils.getApiBoundaryType(innerEntrySpan),
Constants.getEnrichedSpanConstant(ENTRY));
}

@Test
public void test_enrichEvent_loopToItsOwnService_findTwoAPIEntries() {
// Entry to API 1 -> Exit From API 1 & Re-entry -> Entry to API 1 -> Exit to API 2
Expand Down