Add ability to detect virtual nodes in the servicegraph processor - #2365
Conversation
mdisibio
left a comment
mdisibio
left a comment
There was a problem hiding this comment.
LGTM - A few small q's and comments.
| func (p *Processor) upsertPeerNode(e *store.Edge, spanAttr []*v1_common.KeyValue) { | ||
| for _, peerKey := range p.Cfg.PeerAttributes { | ||
| if v, ok := processor_util.FindAttributeValue(peerKey, spanAttr); ok { | ||
| e.PeerNode = v |
There was a problem hiding this comment.
Looking at this logic - is it correct to say that the order of PeerAttributes is their priority? If net.sock.peer.addr exists it will always be used first. If so let's add a note to the documentation snippet in service_graphs.md
There was a problem hiding this comment.
If the order of peerAttributes is the priority, I would suggest that names be preferred over addresses in the default list. Also, does the code look at peer.service anywhere? There is an otel way to map host name or ip address to peer.service: https://opentelemetry.io/docs/instrumentation/java/automatic/agent-config/#peer-service-name and see https://opentelemetry.io/docs/reference/specification/trace/semantic_conventions/span-general/#general-remote-service-attributes as well.
There was a problem hiding this comment.
Nice suggestions. Applied both.
| func (p *Processor) onExpire(e *store.Edge) { | ||
| p.metricExpiredEdges.Inc() | ||
|
|
||
| e.ConnectionType = store.VirtualNode |
There was a problem hiding this comment.
I think this logic is detecting an unmatched edge and registering the virtual node if conditions are right. Could we add a comment? Should e.ConnectionType only be set if one of the conditions is true? I think always setting it (even if not needed) is a little unclear.
There was a problem hiding this comment.
Added comments to clarify what this is doing.
Should e.ConnectionType only be set if one of the conditions is true?
Doesn't really matter, as if it doesn't match one of the two conditions to be considered a virtual node, it won't be collected.
knylander-grafana
left a comment
knylander-grafana
left a comment
There was a problem hiding this comment.
Doc changes look good. Thank you for updating the docs.
Co-authored-by: Kim Nylander <104772500+knylander-grafana@users.noreply.github.com>
… peer-node-service-graph
|
Hi. Can i know please when this feature will be released? |
|
This will be in Tempo 2.2. There is no set release date, but due to our normal cadence it will likely be cut late July/early August |
…afana#2365) * Add ability to detect virtual nodes in the servicegraph processor * Docs and some fixes * Add config to overrides * More docs * Fix mock overrides * Apply suggestions from code review Co-authored-by: Kim Nylander <104772500+knylander-grafana@users.noreply.github.com> * Address comments * Update docs --------- Co-authored-by: Kim Nylander <104772500+knylander-grafana@users.noreply.github.com>
6 participants
What this PR does:
Adds the ability to detect virtual nodes in the servicegraph processor.
Virtual nodes are nodes that form part of the lifecycle of a trace, but spans for them are not being collected because they're outside of the user's reach (eg. an external service for payment processing) or are not instrumented (eg. a frontend application).
Virtual nodes can be detected in two different ways
span.kindset toserver. This indicates that the request has initiated by an external system that's not instrumented, like a frontend application or an engineer viacurlclientspan does not have its matchingserverspan, but has a peer attribute present. In this case we make the assumption that a call was made to an external service, for which Tempo won't receive spans.peer.service,net.peer.name,net.sock.peer.name,rpc.service.key,net.sock.peer.addr,http.url,http.target.PRs in the OTel collector implementation
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]