Enrich root spans that represent a dependency #125
Conversation
| if hasType { | ||
| transactionType := spanTypeAttr.Str() | ||
| if spanSubtypeAttr, hasSubType := span.Attributes().Get(AttributeSpanSubtype); hasSubType { | ||
| transactionType += "." + spanSubtypeAttr.Str() |
There was a problem hiding this comment.
I'd be happy to hear opinion on this one.
The APM UI groups transactions based on transaction type. The idea here is that for such exit&root spans we'd want to use a different transaction type so they don't end up in the same group with incoming transactions.
E.g. the default transaction type for incoming HTTP is request - with this, for outgoing we'd use external.http. It can be that a service sends GET both as outgoing root span and as an incoming transaction. This way those 2 transactions will have different types.
Alternatively to . we could also have / - e.g. external/http.
There was a problem hiding this comment.
will that impact other things like service map or so?
There was a problem hiding this comment.
So far, I haven't seen any impact. Service map is mostly driven by the target.* and destination.* fields from what I know.
The dependency list has icons - that is based on this span, but that is not driven by transaction.type either, so that's not impacted either.
| @@ -189,6 +189,7 @@ func (s *spanEnrichmentContext) Enrich(span ptrace.Span, cfg config.Config) { | |||
| func (s *spanEnrichmentContext) enrich(span ptrace.Span, cfg config.Config) { | |||
| if s.isTransaction { | |||
| s.enrichTransaction(span, cfg.Transaction) | |||
| s.enrichExitSpanTransaction(span, cfg) | |||
There was a problem hiding this comment.
Feel free to ignore, maybe this is personal preference. But the code may be a bit easier to follow when pulling in the if condition to this method, so that it's clear that only transactions that are of type client or producer get enriched.
There was a problem hiding this comment.
I'm wondering whether we need a dedicated method for this at all.
Couldn't we just have some heuristic method in addition that just identifies whether a span is an exist span, use that in an additional condition here, but then just calling the existing enrichSpan() method.
There was a problem hiding this comment.
I still needed to extend enrichSpan() a bit, but now I got rid of enrichExitSpanTransaction - so there is only enrichSpan and enrichTransaction as we had it originally.
The 2 main differences in enrichExitSpanTransaction where:
- Not setting
processor.eventtospan - Setting
transaction.type
We handle these now with a condition.
I also moved the check up to this line.
| span ptrace.Span, | ||
| cfg config.Config, | ||
| ) { | ||
| if span.Kind() == ptrace.SpanKindClient || span.Kind() == ptrace.SpanKindProducer { |
There was a problem hiding this comment.
I'm not sure we can rely on that condition, tbh.
Since that entire situation we try to handle here is an instrumentation bug in itself (IN THEORY: there never should be a single span that is entry and exit at the same time, but IN PRACTICE that happens in some situations), I don't think we should rely on SpanKind at all, tbh.
For example, what if the SpanKind == SpanKindServer, but the span still represents entry and exit at the same time?
How about, we use heuristics similar to how we do it for deriving span.type to identify whether a span is an exit span or not. For example, checking for certain attributes that would only make sense on exit spans but not on entry spans.
There was a problem hiding this comment.
This is a valid point and was thinking about this as well. My idea was to be on the safe side and only enrich real exit spans with the necessary attributes.
I understand we have this issue with nginx proxy spans that are a single span for both incoming and outgoing calls. I don't know what's the span type there, but if it's SpanKindServer then we would not enrich that indeed.
But that use-case is already a bug and I'm not sure how much we want to unwind such bugs.
For example, what if the SpanKind == SpanKindServer, but the span still represents entry and exit at the same time?
I think that's bug in the instrumentation. SemConv says :
For an HTTP client span, SpanKind MUST be Client.
and
For an HTTP server span, SpanKind MUST be Server.
So for HTTP that goes against the spec and I'm not sure how much we should fight these bugs.
How about, we use heuristics similar to how we do it for deriving span.type to identify whether a span is an exit span or not. For example, checking for certain attributes that would only make sense on exit spans but not on entry spans.
We could explore that, but I'm not sure how reliable it is. The check if something is an exit span is a bit different - there is no differentiation in that check between incoming and outgoing spans, because incomings are already assumed to be transactions, so all those cases are already filtered out at that point by only doing the check for spans.
I just quickly looked at HTTP spans, what I see, the server side only has url.path which is only required in server spans, and is not present in client spans. And then url.full is only on theclient-side. But if spankind is already invalid, I'm not sure how much we can rely on attributes. E.g. what if both are present?
Overall I feel we'd end up with a messy heuristic which may not work in all cases and may not even unwind these bugs in a way we'd want to.
If we relax this check here, I think the risk we run here is that we may categorize some incoming calls as exit spans and we start calculating dependencies for those. That'd be very bad. If we can come up with something to avoid that, then I'd feel more confident to relax this.
| if cfg.Span.TypeSubtype.Enabled { | ||
| s.setSpanTypeSubtype(span) | ||
| } | ||
| if cfg.Span.ServiceTarget.Enabled { |
There was a problem hiding this comment.
@gregkalapos What about the thing we discussed about having two ProcessorEvent values in this case?
There was a problem hiding this comment.
I tested it and that triggers an error when the water-flow chart is generated. These transactions will be also returned for all queries that look for spans and there were some missing span related fields there.
@axw had a related idea: we could just remove the filter on processor.event from the relevant queries in Kibana. I'd rather explore that option and potentially get rid of processor.event in those queries.
There was a problem hiding this comment.
Discussed this with @AlexanderWert and we said, the correct way would be to still set both span and transaction and it'd be nice to not have to update Kibana.
In 3def749 I changed this.
Now that we share the enrichSpan() method, there are no missing field errors when the waterfall chart is generated. With above commit, this seems working:
However, order matters here. This is the code in Kibana that builds up the chart - and if it's [span transaction], then it's still categorized as a span and not as a transaction leading to an empty waterfall chart.
So, it seems we can set both span and transaction on such root exit spans, but I'd say mid/long term we should still update Kibana to make it more robust.
| // In OTel, a local root span can represent an outgoing call or a producer span. | ||
| // In such cases, the span is still mapped into a transaction, but enriched | ||
| // with additional attributes that are specific to the outgoing call or producer span. | ||
| isExitRootSpan := s.isTransaction && span.Kind() == ptrace.SpanKindClient || span.Kind() == ptrace.SpanKindProducer |
There was a problem hiding this comment.
This check is still up for discussion: #125 (comment)
In the OTel data model it's allowed (and not very uncommon) to have a root span which in itself represents a dependency. Such spans are root and exit span at the same time.
Examples can be seen in the tests (e.g. outgoing HTTP call, just a span without root that represents a db call or produces a message). Such spans are mapped into a transaction, because in the Elastic data model we always need a transaction as root.
This PR adds logic to enrich such transactions with the necessary fields to calculate dependencies and draw a service map.
Follow up PR in Kibana will still need to be done to query such transactions - currently only
processor.event: spanis used for dependencies. This PR adds all the necessary fields, so then those queries can be updated and filtering onprocessor.eventcan be removed (or explicitly filter to both spans and transaction in case removing is not needed).