Surface new labels for uninstrumented services and systems #3543

t00mas · 2024-04-05T15:49:28Z

What this PR does:

For the servicegraphs processor:

Gates this feature behind the EnableExtraUninstrumentedServicesLabels flag in the Config
If enabled, adds new labels to traces_service_graph_request_*:
- virtual_node with a value of client or server, so the un-instrumented service's node can be identified.
- client|server_db|messaging_system with the corresponding resource attribute value, to surface and identify the service and the node.

Which issue(s) this PR fixes:
Fixes #3461

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

CLAassistant · 2024-04-05T15:49:34Z

All committers have signed the CLA.

modules/generator/processor/servicegraphs/servicegraphs_test.go

domasx2 · 2024-04-09T09:24:35Z

modules/generator/processor/servicegraphs/servicegraphs.go

@@ -190,6 +190,17 @@ func (p *Processor) consume(resourceSpans []*v1_trace.ResourceSpans) (err error)
 							e.ServerService = dbName
 							e.ServerLatencySec = spanDurationSec(span)
 						}
+
+						if p.Cfg.EnableExtraUninstrumentedServicesLabels {
+							switch e.ConnectionType {


this is mutually exclusive, but is it possible that a service can be both database and messaging system? eg Redis

it is - because at this point it has been decided what the service is acting as, depending on the presence/absence of the db.name resource attribute

joe-elliott

a few initial questions. other thoughts:

what occurs if there are conflicts between these new values and configured span dimensions?
what do you think about pooling edges in the store?

modules/generator/processor/servicegraphs/store/edge.go

modules/generator/processor/servicegraphs/config.go

t00mas · 2024-04-12T14:40:38Z

a few initial questions. other thoughts:

what occurs if there are conflicts between these new values and configured span dimensions?

what do you think about pooling edges in the store?

Thanks for reviewing Joe.

I've added some testing to cover "conflicts" with manually added Dimensions.

Could you be more specific about the edge pooling? Happy to address that if you think there's an issue there. (Sorry, I'm just new to Tempo in general)

mapno

I wonder if we should make this a per-tenant override instead. If this feature is behind a global config parameter, it makes me think that we should just add it to the default generator without a flag. What's the use case for enabling or disabling it globally?

t00mas · 2024-04-18T13:09:23Z

I wonder if we should make this a per-tenant override instead. If this feature is behind a global config parameter, it makes me think that we should just add it to the default generator without a flag. What's the use case for enabling or disabling it globally?

It actually makes more sense as a per-tenant override instead, yes.

I've also been discussing with Joe and I will:

Scope-down the PR just to add the new virtual_node label (the others can be added with a combination of Dimensions and Prefixes)
Pool edges in the store for performance reasons

docs/sources/tempo/metrics-generator/service_graphs/_index.md

knylander-grafana

Doc changes look good. Thank you for adding docs!

t00mas · 2024-04-22T13:41:58Z

These are some benchmarks about Edge reuse with pooling:

goos: darwin
goarch: arm64
pkg: github.com/grafana/tempo/modules/generator/processor/servicegraphs

	20_runs_bench.txt	20_runs_bench_with_pooling.txt
	sec/op	sec/op vs base
ServiceGraphs-11	21.33µ ± 0%	21.63µ ± 2% +1.41% (p=0.006 n=20)
	B/op	B/op vs base
ServiceGraphs-11	24.89Ki ± 0%	24.06Ki ± 0% -3.35% (p=0.000 n=20)
	allocs/op	allocs/op vs base
ServiceGraphs-11	450.0 ± 0%	443.0 ± 0% -1.56% (p=0.000 n=20)

There's probably better numbers to be had at scale, since these are limited to the testdata size.

Co-authored-by: Kim Nylander <104772500+knylander-grafana@users.noreply.github.com>

joe-elliott · 2024-04-22T19:18:44Z

modules/generator/processor/servicegraphs/servicegraphs.go

@@ -86,8 +86,20 @@ type Processor struct {

 func New(cfg Config, tenant string, registry registry.Registry, logger log.Logger) gen.Processor {
 	labels := []string{"client", "server", "connection_type"}
+
+	if cfg.EnableVirtualNodeLabel {


why don't we add this to the list of labels above? this will prevent us from having to add lines 96-102 which adds special handling for this value.

then we could add a new param to onComplete where we pass the string from onExpire

definitely doable - however what you save in these lines to avoid the prefixes, is added back to the onComplete logic for slice allocation and label value assignment.

also, it'll make this new label behave as half-dimension half-edge-property. do you think that'd be ok?

can you also explain further what's the modification of the onComplete signature for?

yup, i see. i was trying to remove some of the state wrangling in the consume() method but you've already completely removed it.

agree that the tradeoffs mentioned:

what you save in this lines to avoid the prefixes, is added back to the onComplete logic for slice allocation and label value assignment.

are not worth it. let's keep it as implemented now.

also, it'll make this new label behave as half-dimension half-edge-property. do you think that'd be ok?

i'm not seeing an issue with that. do you have some concerns in mind?

ok agree, leaving it behaving as before, as a dimension.

I think it's better to "treat it" as if it's fully one thing or the other. What I mean by that, from a future reader standpoint, it'll be easier to follow logic if it's treated as a dimension or as a struct property (first-class label?)

modules/generator/processor/servicegraphs/servicegraphs.go

modules/generator/processor/servicegraphs/store/edge.go

modules/generator/processor/servicegraphs/servicegraphs.go

joe-elliott

Docs update, feature addition, a perf improvement. Love it. Thank you!

modules/generator/processor/servicegraphs/config.go

docs/sources/tempo/configuration/_index.md

mapno · 2024-04-24T10:59:16Z

modules/generator/processor/servicegraphs/servicegraphs.go

@@ -52,6 +52,8 @@ const (
 	metricRequestClientSeconds = "traces_service_graph_request_client_seconds"
 )

+const virtualNodeLabel = "__virtual_node"


Why the __ prefix?

I asked for this. Should we drop it? Just trying to avoid conflicts with actual span attributes.

I'll be OK with and without the __ prefix

In spanmetrics we do the opposite, if there's a collision with an instrinsic dimension we add __ (code). I don't feel too strongly about this. I prefer it that way, butI'm OK with what you decide.

OK - then to not raise any questions to future readers, and because it seems a collision is unlikely, I'll leave it as without the __ prefix.

It'd be good in the future to identify and sanitize in the same way as in spanmetrics, happy to work on that small improvement in a future PR if you need me.

modules/generator/processor/servicegraphs/store/edge.go

modules/generator/processor/servicegraphs/store/interface.go

Co-authored-by: Mario <mariorvinas@gmail.com>

mapno

I'm good with the PR, except that one comment. Nice work!

modules/generator/processor/servicegraphs/store/store.go

The edge is not expired here, so it shouldn't be returned to the pool. Co-authored-by: Mario <mariorvinas@gmail.com>

t00mas requested review from joe-elliott, annanay25, mdisibio, mapno, kvrhdn, zalegrala, electron0zero, ie-pham and stoewer as code owners April 5, 2024 15:49

mar4uk reviewed Apr 8, 2024

View reviewed changes

modules/generator/processor/servicegraphs/servicegraphs_test.go Outdated Show resolved Hide resolved

domasx2 reviewed Apr 9, 2024

View reviewed changes

joe-elliott reviewed Apr 9, 2024

View reviewed changes

modules/generator/processor/servicegraphs/store/edge.go Outdated Show resolved Hide resolved

modules/generator/processor/servicegraphs/config.go Outdated Show resolved Hide resolved

t00mas force-pushed the service-labels branch from cc7f069 to 2bdc44b Compare April 12, 2024 14:09

t00mas requested a review from knylander-grafana as a code owner April 12, 2024 14:27

mapno reviewed Apr 18, 2024

View reviewed changes

knylander-grafana reviewed Apr 19, 2024

View reviewed changes

docs/sources/tempo/metrics-generator/service_graphs/_index.md Outdated Show resolved Hide resolved

knylander-grafana reviewed Apr 19, 2024

View reviewed changes

t00mas and others added 9 commits April 22, 2024 15:45

Surface new labels for uninstrumented services and systems

3521126

Update CHANGELOG.md

f8c7d0b

remove unnecessary Println in test

9514aee

Reuse dimensions and prefixes for this use-case

fb7a380

Add docs

944e490

keep only virtual_node behind the new feature

4a8c03b

add overrides

bdb56ee

Update docs/sources/tempo/metrics-generator/service_graphs/_index.md

154bdf3

Co-authored-by: Kim Nylander <104772500+knylander-grafana@users.noreply.github.com>

forgot this test (fixed)

3c640cc

t00mas added 3 commits April 22, 2024 15:46

add benchmarks

1ee1d01

add edge pooling/reuse

cc8d136

update flag

0921aee

t00mas force-pushed the service-labels branch from 6e315ab to 0921aee Compare April 22, 2024 13:46

joe-elliott reviewed Apr 22, 2024

View reviewed changes

t00mas added 2 commits April 23, 2024 12:20

move label out of dimensions to avoid prefix logic

0b8afcb

lint

a9ed1cc

joe-elliott approved these changes Apr 23, 2024

View reviewed changes

mapno reviewed Apr 24, 2024

View reviewed changes

t00mas and others added 4 commits April 24, 2024 14:57

Update modules/generator/processor/servicegraphs/config.go

7903e7f

Co-authored-by: Mario <mariorvinas@gmail.com>

minor amends to names and docs

75896ce

leave the new virtual_node label as an extra dimension

ab706d0

keep edge sync.Pool ops inside store

5642615

mapno requested changes Apr 25, 2024

View reviewed changes

modules/generator/processor/servicegraphs/store/store.go Outdated Show resolved Hide resolved

t00mas and others added 2 commits April 25, 2024 10:11

Update modules/generator/processor/servicegraphs/store/store.go

bf5f1c6

The edge is not expired here, so it shouldn't be returned to the pool. Co-authored-by: Mario <mariorvinas@gmail.com>

leave the new label un-prefixed

153640b

mapno approved these changes Apr 25, 2024

View reviewed changes

joe-elliott merged commit 251bf5a into grafana:main Apr 30, 2024
15 checks passed

t00mas deleted the service-labels branch May 15, 2024 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Surface new labels for uninstrumented services and systems #3543

Surface new labels for uninstrumented services and systems #3543

t00mas commented Apr 5, 2024 •

edited

Loading

CLAassistant commented Apr 5, 2024 •

edited

Loading

domasx2 Apr 9, 2024

t00mas Apr 9, 2024

joe-elliott left a comment

t00mas commented Apr 12, 2024

mapno left a comment

t00mas commented Apr 18, 2024 •

edited

Loading

knylander-grafana left a comment

t00mas commented Apr 22, 2024

joe-elliott Apr 22, 2024

t00mas Apr 23, 2024 •

edited

Loading

joe-elliott Apr 23, 2024

t00mas Apr 24, 2024

joe-elliott left a comment

mapno Apr 24, 2024

joe-elliott Apr 24, 2024

t00mas Apr 24, 2024

mapno Apr 25, 2024

t00mas Apr 25, 2024

mapno left a comment

Surface new labels for uninstrumented services and systems #3543

Surface new labels for uninstrumented services and systems #3543

Conversation

t00mas commented Apr 5, 2024 • edited Loading

CLAassistant commented Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joe-elliott left a comment

Choose a reason for hiding this comment

t00mas commented Apr 12, 2024

mapno left a comment

Choose a reason for hiding this comment

t00mas commented Apr 18, 2024 • edited Loading

knylander-grafana left a comment

Choose a reason for hiding this comment

t00mas commented Apr 22, 2024

Choose a reason for hiding this comment

t00mas Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joe-elliott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapno left a comment

Choose a reason for hiding this comment

t00mas commented Apr 5, 2024 •

edited

Loading

CLAassistant commented Apr 5, 2024 •

edited

Loading

t00mas commented Apr 18, 2024 •

edited

Loading

t00mas Apr 23, 2024 •

edited

Loading