Add enrichment for span.destination.service.resource #72
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -191,6 +191,9 @@ func (s *spanEnrichmentContext) enrichSpan( | |
| if cfg.ServiceTarget.Enabled { | ||
| s.setServiceTarget(span) | ||
| } | ||
| if cfg.DestinationService.Enabled { | ||
| s.setDestinationService(span) | ||
| } | ||
| } | ||
|
|
||
| // normalizeAttributes sets any dependent attributes that | ||
|
|
@@ -300,6 +303,40 @@ func (s *spanEnrichmentContext) setServiceTarget(span ptrace.Span) { | |
| } | ||
| } | ||
|
|
||
| func (s *spanEnrichmentContext) setDestinationService(span ptrace.Span) { | ||
| var destnResource string | ||
| if s.peerService != "" { | ||
|
Contributor
There was a problem hiding this comment. Hmm,
This is THE ultimate attribute describing a dependency 🎉 - if I understand the spec correctly, this literally gives us the service name of the callee. So the difference between taking this as the higher prio attribute for I suspect the thing here is the following: while this attribute in theory is very useful, in reality, that is probably never used. How could one populate this attribute? I can't imagine an agent is able to figure out the So I think the code path that uses If this would be a new mapping, I'd say that there should not be a difference here - if At the same time, I see there are lots of tests focusing on this - it's ok to have those, just keep in mind, if my theory about basically never having
Contributor
Author
There was a problem hiding this comment.
Yes, you are right. Thanks for the insight on the
Agreed 👍
Hmm, good point. I think I added |
||
| destnResource = s.peerService | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Couldn't this be just an if-else and then dropping the lots of Something like:
Member
There was a problem hiding this comment. Or separate the derivation from setting the attribute, so we can return early.
Contributor
Author
There was a problem hiding this comment. // For parity with apm-data, destn resource does not handle
// temporary destination flag. However, it is handled by
// service.target fields and we might want to do the same here.
if destnResource != "" && s.messagingDestinationName != "" {
destnResource += "/" + s.messagingDestinationName
}This is the problematic bit, the destination resource is updated based on the messagingDestinationName. I could make it return early with some conditions but, I think, that would make the code a bit harder to follow through and maintain. No strong opinions though but a simple if/else or a case in the switch statement itself for
Contributor
Author
There was a problem hiding this comment. In case it was not clear in the above statement, the messaging destination is appended to the destination resource if it is not empty. Atleast, this is what parity with apm-data would be. This would mean that if
Member
There was a problem hiding this comment. Ah I was thinking we could do something like this: if s.messagingSystem != "" {
destnResource := s.messagingSystem
if s.messagingDestinationName != "" {
destnResource += "/" + s.messagingDestinationName
}
return destnResource
}I realise now that's not quite equivalent to what you've implemented though, because as it's written we might append the
Contributor
Author
There was a problem hiding this comment.
I spent a bit of time reasoning about this when I was reading the apm-data code but I couldn't think of this NOT being valid so I carried over the logic. Technically, if we know the destination name of a topic and we have I am not too sure overall what to do here. I would suggest we do this separate to this PR, I have created a GH issue to discuss this and make any changes to this. Let me know if it sounds good. |
||
|
|
||
| switch { | ||
| case s.isDB: | ||
| if destnResource == "" && s.dbType != "" { | ||
| destnResource = s.dbType | ||
| } | ||
| case s.isMessaging: | ||
| if destnResource == "" && s.messagingSystem != "" { | ||
| destnResource = s.messagingSystem | ||
| } | ||
| // For parity with apm-data, destn resource does not handle | ||
| // temporary destination flag. However, it is handled by | ||
| // service.target fields and we might want to do the same here. | ||
| if destnResource != "" && s.messagingDestinationName != "" { | ||
| destnResource += "/" + s.messagingDestinationName | ||
| } | ||
|
Comment on lines
+321
to
+326
Contributor
Author
There was a problem hiding this comment. [For reviewers] Check the code comment. |
||
| case s.isRPC, s.isHTTP: | ||
| if destnResource == "" { | ||
| if res := getHostPort(s.urlFull, s.urlDomain, s.urlPort); res != "" { | ||
| destnResource = res | ||
| } | ||
axw marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
|
|
||
| if destnResource != "" { | ||
| span.Attributes().PutStr(AttributeSpanDestinationServiceResource, destnResource) | ||
| } | ||
| } | ||
|
|
||
| func isTraceRoot(span ptrace.Span) bool { | ||
| return span.ParentSpanID().IsEmpty() | ||
| } | ||
|
|
@@ -319,6 +356,9 @@ func isElasticTransaction(span ptrace.Span) bool { | |
| return false | ||
| } | ||
|
|
||
| // getHostPort derives the host:port value from url.* attributes. Unlike | ||
| // apm-data, the current code does NOT fallback to net.* or http.* | ||
| // attributes as most of these are now deprecated. | ||
| func getHostPort(urlFull *url.URL, urlDomain string, urlPort int64) string { | ||
| if urlFull != nil { | ||
| return urlFull.Host | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[For reviewers] There is quite a bit of discrepancy between the destination resource and the service target fields (
service.target.{name, type}) - you can check this by the test cases in the PR. The current code keeps the logic similar to what is present in apm-data for the most part -- any points of differences should be highlighted in the other comments.