Mark span as APIBoundary if service name differs from parent service … #221
Conversation
pavan-traceable
requested review from
a team,
aaron-steinfeld,
avinashkolluru,
kotharironak and
surajpuvvada
Codecov Report
@@ Coverage Diff @@
## main #221 +/- ##
============================================
- Coverage 80.75% 80.70% -0.06%
- Complexity 1127 1128 +1
============================================
Files 101 101
Lines 4345 4349 +4
Branches 398 400 +2
============================================
+ Hits 3509 3510 +1
- Misses 650 653 +3
Partials 186 186
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.
|
|
||
| Event parentEvent = graph.getParentEvent(event); | ||
| if (!EnrichedSpanUtils.isEntrySpan(parentEvent)) { | ||
| if (!EnrichedSpanUtils.isEntrySpan(parentEvent) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
Do we have any attributes from Kong, Istio that indicate that span is proxySpan - e.g tracer.type?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Discussed offline. I withdraw my concerns. I'm ok with multiple API boundaries if the service name changes.
There was a problem hiding this comment.
@kotharironak Had offline discussion with Dave and Tim, they have agreed with this change.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
...-trace-enricher-api/src/main/java/org/hypertrace/traceenricher/trace/util/ApiTraceGraph.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
| && GraphBuilderUtil.areBothSpansFromDifferentService( | ||
| child, entryBoundaryEvent.get())) { | ||
| ApiNode<Event> destinationApiNode = entryApiBoundaryEventIdToApiNode.get(child); | ||
| LOGGER.debug( |
There was a problem hiding this comment.
Wrap this within if (LOGGER.isDebugEnabled()) {
HexUtils.getHex( could be expensive and shouldn't be done if debug logs are not enabled.
| // 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 |
There was a problem hiding this comment.
nit: find all the children of exit boundary events is not accurate since we check the children of entry boundary events too now
There was a problem hiding this comment.
Other case updated later in the method, this is the first case.
| return false; | ||
| } | ||
|
|
||
| /** Check whether these spans belongs to different services. */ |
There was a problem hiding this comment.
I think this method should be in ApiBoundaryTypeAttributeEnricher itself, this class is housing utilities related rebuilding of ApiTraceGraph and StructuredTraceGraph
There was a problem hiding this comment.
This is used in ApiTraceGraph as well.
There was a problem hiding this comment.
I see its used in ApiTraceGraph as well, then the right place for it is utility for span/enriched span ig
There was a problem hiding this comment.
I didn't get it, which class is ideal for this ? In GraphQLBuilder util most of the methods are related to what i have added now, although name(GraphQLBuilderUtil) is not looking right
There was a problem hiding this comment.
I didn't get it, which class is ideal for this
EnrichedSpanUtils (most of the method for checking some property on span is present here)
In GraphBuilder util most of the methods are related to what
most of the method here is to check the existing event/trace has changed or not and is meant to support Graph caching logic, whereas the newly added method seems unrelated
yeah this class needs renaming
There was a problem hiding this comment.
Yes, in fact i realised while doing changes in Traceable, moved to EnrichedSpanUtils.
This comment has been minimized.
This comment has been minimized.
| 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) |
There was a problem hiding this comment.
if the exit span is missing how will these two span have parent and child relationship?
There was a problem hiding this comment.
Ignore this, I was assuming that an actual span is missing here which is not the case
This comment has been minimized.
This comment has been minimized.
Mark span as APIBoundary if it is entry span and service name differs from parent span service name.
Description
Added one more condition to eligible span as APIBoundary type span if it is entry span and service name differs from Parent span.
Testing
Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.
Checklist:
Documentation
Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.