From 1259b63a81364e245e0f0486e3e8b95922024ceb Mon Sep 17 00:00:00 2001 From: Olivier Giniaux Date: Mon, 15 Apr 2024 00:38:30 +0200 Subject: [PATCH] [connector/servicegraph] Update virtual node defaults and change feature gate to beta (#31735) **Description:** - Change default values to peer.service, db.name and db.system (in this order). These are compliant with conventions, and the same as the tempo service graph uses. - Switch the feature from "alpha" to "beta". Virtual nodes are working well on the tempo side, and feature could be disabled anyway by using an empty list if needed. - Reword documentation for improved clarity - Fix "processor" that should be "connector" instead since recent commit **Link to tracking Issue:** https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/31734 **Testing:** No functional change requiring additional tests **Documentation:** Updated documentation --- .../servicegraphconnector-virtualnodes.yaml | 20 +++++++++++++++++++ connector/servicegraphconnector/README.md | 6 +++--- connector/servicegraphconnector/connector.go | 4 ++-- connector/servicegraphconnector/factory.go | 8 ++++---- 4 files changed, 29 insertions(+), 9 deletions(-) create mode 100644 .chloggen/servicegraphconnector-virtualnodes.yaml diff --git a/.chloggen/servicegraphconnector-virtualnodes.yaml b/.chloggen/servicegraphconnector-virtualnodes.yaml new file mode 100644 index 000000000000..d213656cfef0 --- /dev/null +++ b/.chloggen/servicegraphconnector-virtualnodes.yaml @@ -0,0 +1,20 @@ +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: breaking + +# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) +component: connector/servicegraphconnector + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Change `connector.servicegraph.virtualNode` feature gate from Alpha to Beta (now enabled by default) and change `virtual_node_peer_attributes` default values. + +# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. +issues: [31734] + +# If your change doesn't affect end users or the exported elements of any package, +# you should instead start your pull request title with [chore] or use the "Skip Changelog" label. +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [user] diff --git a/connector/servicegraphconnector/README.md b/connector/servicegraphconnector/README.md index 3c163a88977b..3d089cd18394 100644 --- a/connector/servicegraphconnector/README.md +++ b/connector/servicegraphconnector/README.md @@ -118,7 +118,7 @@ datasources: The following settings are required: -- `metrics_exporter`: the name of the exporter that this processor will write metrics to. This exporter **must** be present in a pipeline. +- `metrics_exporter`: the name of the exporter that this connector will write metrics to. This exporter **must** be present in a pipeline. - `latency_histogram_buckets`: the list of durations defining the latency histogram buckets. - Default: `[2ms, 4ms, 6ms, 8ms, 10ms, 50ms, 100ms, 200ms, 400ms, 800ms, 1s, 1400ms, 2s, 5s, 10s, 15s]` - `dimensions`: the list of dimensions to add together with the default dimensions defined above. @@ -134,8 +134,8 @@ The following settings can be optionally configured: - Default: `1m` - `store_expiration_loop`: the time to expire old entries from the store periodically. - Default: `2s` -- `virtual_node_peer_attributes`: the list of attributes need to match for building virtual server node, the higher the front, the higher the priority. - - Default: `[db.name, net.sock.peer.addr, net.peer.name, rpc.service, net.sock.peer.name, net.peer.name, http.url, http.target]` +- `virtual_node_peer_attributes`: the list of attributes, ordered by priority, whose presence in a client span will result in the creation of a virtual server node. An empty list disables virtual node creation. + - Default: `[peer.service, db.name, db.system]` - `metrics_flush_interval`: the interval at which metrics are flushed to the exporter. - Default: Metrics are flushed on every received batch of traces. - `database_name_attribute`: the attribute name used to identify the database name from span attributes. diff --git a/connector/servicegraphconnector/connector.go b/connector/servicegraphconnector/connector.go index bd8a22a431be..fe275acbac0f 100644 --- a/connector/servicegraphconnector/connector.go +++ b/connector/servicegraphconnector/connector.go @@ -42,7 +42,7 @@ var ( } defaultPeerAttributes = []string{ - semconv.AttributeDBName, semconv.AttributeNetSockPeerAddr, semconv.AttributeNetPeerName, semconv.AttributeRPCService, semconv.AttributeNetSockPeerName, semconv.AttributeNetPeerName, semconv.AttributeHTTPURL, semconv.AttributeHTTPTarget, + semconv.AttributePeerService, semconv.AttributeDBName, semconv.AttributeDBSystem, } defaultDatabaseNameAttribute = semconv.AttributeDBName @@ -380,7 +380,7 @@ func (p *serviceGraphConnector) onExpire(e *store.Edge) { p.statExpiredEdges.Add(context.Background(), 1) - if virtualNodeFeatureGate.IsEnabled() { + if virtualNodeFeatureGate.IsEnabled() && len(p.config.VirtualNodePeerAttributes) > 0 { e.ConnectionType = store.VirtualNode if len(e.ClientService) == 0 && e.Key.SpanIDIsEmpty() { e.ClientService = "user" diff --git a/connector/servicegraphconnector/factory.go b/connector/servicegraphconnector/factory.go index daec2316c71c..4d533a00d79f 100644 --- a/connector/servicegraphconnector/factory.go +++ b/connector/servicegraphconnector/factory.go @@ -30,21 +30,21 @@ var virtualNodeFeatureGate, legacyMetricNamesFeatureGate, legacyLatencyUnitMsFea func init() { virtualNodeFeatureGate = featuregate.GlobalRegistry().MustRegister( virtualNodeFeatureGateID, - featuregate.StageAlpha, - featuregate.WithRegisterDescription("When enabled, when the edge expires, processor checks if it has peer attributes(`db.name, net.sock.peer.addr, net.peer.name, rpc.service, http.url, http.target`), and then aggregate the metrics with virtual node."), + featuregate.StageBeta, + featuregate.WithRegisterDescription("When enabled and setting `virtual_node_peer_attributes` is not empty, the connector looks for the presence of these attributes in span to create virtual server nodes."), featuregate.WithRegisterReferenceURL("https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/17196"), ) // TODO: Remove this feature gate when the legacy metric names are removed. legacyMetricNamesFeatureGate = featuregate.GlobalRegistry().MustRegister( legacyLatencyMetricNamesFeatureGateID, featuregate.StageAlpha, // Alpha because we want it disabled by default. - featuregate.WithRegisterDescription("When enabled, processor uses legacy latency metric names."), + featuregate.WithRegisterDescription("When enabled, connector uses legacy latency metric names."), featuregate.WithRegisterReferenceURL("https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/18743,https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/16578"), ) legacyLatencyUnitMsFeatureGate = featuregate.GlobalRegistry().MustRegister( legacyLatencyUnitMs, featuregate.StageAlpha, // Alpha because we want it disabled by default. - featuregate.WithRegisterDescription("When enabled, processor reports latency in milliseconds, instead of seconds."), + featuregate.WithRegisterDescription("When enabled, connector reports latency in milliseconds, instead of seconds."), featuregate.WithRegisterReferenceURL("https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/27488"), ) }