[connector/servicegraphconnector] Adds a new config option allowing an extra label for virtual node identification #32826
Conversation
|
|
t00mas
force-pushed
the
extra-labels-for-service-graphs
branch
from
a881d28
to
4283b98
Compare
| if p.config.VirtualNodeExtraLabel { | ||
| dimensions = addExtraLabel(dimensions, virtualNodeLabel, string(e.VirtualNodeLabel)) | ||
| } |
There was a problem hiding this comment.
I understand we only want to add this label if the edge has a virtual node, right? We should check that
There was a problem hiding this comment.
Could be - it's an easy addition, we probably just change to:
if e.ConnectionType == store.VirtualNode && p.config.VirtualNodeExtraLabel {however, is it better to not have it, or to have virtual_node: with an empty value when this option is enabled?
I'm leaning towards having the label anyway with an empty value, but not 100% convinced of that.
There was a problem hiding this comment.
I think it can be confusing, but I don't have a strong opinion on it.
There was a problem hiding this comment.
OK then, I'll opt for leaving it with an empty value sometimes but always present. We can review this if it's an issue.
| @@ -215,6 +216,8 @@ func (p *serviceGraphConnector) metricFlushLoop(flushInterval time.Duration) { | |||
| } | |||
|
|
|||
| func (p *serviceGraphConnector) flushMetrics(ctx context.Context) error { | |||
| p.store.Expire() | |||
There was a problem hiding this comment.
Edges are already expired in a different goroutine, why force expiration here?
There was a problem hiding this comment.
That's what I saw, but without it (or because it's a different goroutine), the shutdown and metrics collection will happen before the edge expire&complete, so metrics from pending edges are not collected (they won't be added to .reqTotal for example).
Do you know if there's a better way to test this?
PS: This is unrelated to the addition of the new label, I realized when I was adding a test for connection_type: virtual_node.
There was a problem hiding this comment.
OK fixed, I overlooked a minor detail about the metric flush loop, so test has to allow it. No more double-expire here.
t00mas
force-pushed
the
extra-labels-for-service-graphs
branch
from
b878b35
to
1ad0509
Compare
Nope, just fixed a test signature after a rebase. |
| @@ -3,6 +3,8 @@ module github.com/open-telemetry/opentelemetry-collector-contrib/connector/servi | |||
| go 1.21.0 | |||
|
|
|||
| require ( | |||
| github.com/open-telemetry/opentelemetry-collector-contrib/pkg/golden v0.99.0 | |||
There was a problem hiding this comment.
sorry about this, but this needs a bump
There was a problem hiding this comment.
makes sense, updated
jpkrohling
force-pushed
the
extra-labels-for-service-graphs
branch
from
4c76439
to
58520f9
Compare
jpkrohling
force-pushed
the
extra-labels-for-service-graphs
branch
from
58520f9
to
cff100e
Compare
crobert-1
removed
cmd/telemetrygen
t00mas
force-pushed
the
extra-labels-for-service-graphs
branch
from
b2e8259
to
e03f97d
Compare
Description:
enable_virtual_node_labelis added. When enabled, it will add an extra label to the metrics,virtual_node, indicating if theclientor theserverwas the missing span.Link to tracking Issue: 31889
Testing:
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/golden.virtual_nodelabels to the metrics, as well asconnection_type: virtual_node(as I think it was untested before).Documentation:
Note to reviewers
I have a suspicion that
connection_type: virtual_nodewas broken. There was a race condition onstore.Expireso the expired edges were completing after the metrics were built. I have added ap.store.Expire()just at the start offlushMetricsto allow these edges to contribute to the metrics. Please advise if you see something wrong with this approach.