Improve servicegraph handling of newer OTEL semantic conventions #3711
Conversation
zalegrala
requested review from
knylander-grafana,
joe-elliott,
annanay25,
mdisibio,
mapno,
yvrhdn,
electron0zero,
ie-pham and
stoewer
as code owners
| ) | ||
|
|
||
| // Check for db.name first. The dbName is set initially to maintain backwards compatbility. | ||
| if name, ok := processor_util.FindAttributeValue(string(semconv.DBNameKey), resourceAttr, span.Attributes); ok { |
There was a problem hiding this comment.
The pattern here loops over the span and resource attributes to find specific keys. This means that if you use the preferred attributes in the spans, then the generator has to do less work. I'd considered finding all available keys in one pass, but following the existing pattern elsewhere feels simpler.
| // Check for network.peer.address and network.peer.port. Use port if it is present. | ||
| if host, ok := processor_util.FindAttributeValue(string(semconv.NetworkPeerAddressKey), resourceAttr, span.Attributes); ok { | ||
| if port, ok := processor_util.FindAttributeValue(string(semconv.NetworkPeerPortKey), resourceAttr, span.Attributes); ok { | ||
| e.ServerService = fmt.Sprintf("%s:%s", host, port) |
There was a problem hiding this comment.
There was a problem hiding this comment.
I thought about the carnality here too, but I think because this is server side information, we should be okay since I expect server name/port combos to be significantly less than the ephemeral ports in use by the client. Good thing to watch out for when we roll for sure.
CHANGELOG.md
Outdated
| @@ -49,6 +49,7 @@ | |||
| * [ENHANCEMENT] TraceQL - Add support for scoped intrinsics using `:` [#3629](https://github.com/grafana/tempo/pull/3629) (@ie-pham) | |||
| available scoped intrinsics: trace:duration, trace:rootName, trace:rootService, span:duration, span:kind, span:name, span:status, span:statusMessage | |||
| * [ENHANCEMENT] Performance improvements on TraceQL and tag value search. [#3650](https://github.com/grafana/tempo/pull/3650),[#3667](https://github.com/grafana/tempo/pull/3667) (@joe-elliott) | |||
| * [ENHANCEMENT] Improve use of OTEL semantic conventions on the service graph [#3711](https://github.com/grafana/tempo/pull/3711) (@zalegrala) | |||
There was a problem hiding this comment.
Might have to move this back up the changelog, unless we backport this PR
There was a problem hiding this comment.
Good catch, thank you.
| // if we have a server.address, use it as the database ServerService | ||
| // if we have a network.peer.address, use it as the database ServerService. Include :port if network.peer.port is present | ||
| // if we have a db.name, use it as the database ServerService, which is the backwards-compatible behavior | ||
| func (p *Processor) upsertDatabaseRequest(e *store.Edge, resourceAttr []*v1_common.KeyValue, span *v1_trace.Span) { |
There was a problem hiding this comment.
Good idea to break this out in a separate function 👍
zalegrala
force-pushed
the
otelDatabaseSemconv
branch
from
6211030 to
ca5189a
Compare
What this PR does:
Currently, the
db.nameattribute is used as the node identity on the service graph. This leads to situations where the name of the database may be reused between services, leading to a single node on the graph where more than one actual service is in use.This PR changes the node identity when newer OTEL semantic conventions are available as attributes on the span. The following attributes are considered in order:
peer.serviceserver.addressnetwork.peer.address-- additionallynetwork.peer.portwill be used if availabledb.nameWhich issue(s) this PR fixes:
Fixes #3010
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]