Prefix service graph extra dimensions

[domasx2]

What this PR does:
In the service graph processor, when adding custom dimensions to metrics, values can be overridden if parent and children span have the same attribute but with different values. The attributes of the last span to be processed are the ones selected for the metrics. This results in inconsistent behaviour, and does not match what otel collector service graph connector does.

One use case is to add service.namespace dimension, in case you want services to be identified by both name and namespace. You have to have a label for both client and server.

This PR prefixes values coming from client span with client_ and values coming from server span with server_.

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

Checklist

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

[CLAassistant]

All committers have signed the CLA.

[joe-elliott]

This PR will impact cloud customers and cannot be merged until we understand and communicate that. It is a breaking change to metrics as well as a bump in metrics cardinality.

[Jxlam-pdx]

@cyrille-leclerc @gouthamve can you drive understanding and communicating the impact with teams involved? It's a breaking change and potentially have cost implication with Cardinality increase. Thanks!

[domasx2]

@joe-elliott thanks, that makes sense. Any thoughts on introducing this in a non breaking way? For example, adding syntax to be explicit about desired behaviour, like client:service.namespace to generate client_service_namespace taken only from client span. Or a config opt-in flag.

[domasx2]

OTEL service graph processor does prefix: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/processor/servicegraphprocessor/processor.go#L293

[knylander-grafana]

If this is a breaking change, how will this change impact how our customers use things? What will they need to change or update to correct for this breaking change?

[cyrille-leclerc]

@joe-elliott @Jxlam-pdx point taken. Sorry for the delay, @gouthamve is busy at KubeCon this week.
Our team will clarify the change, document it, and evaluate the impact on the cardinality.

[zalegrala]

Have we started the review on current customers? Is there any update to that end?

[domasx2]

As per discussion in #1860, made this opt-in via enable_client_server_prefix configuration option

[ie-pham]

If we are intending on adding enable_client_server_prefix as a per tenant config, this needs to also be in the overrides section. You can follow what I did for enable_target_info in my PR 😅 but it would just be labeled

you should touch at least these files:
modules/generator/config.go
modules/generator/overrides.go
modules/generator/overrides_test.go
modules/overrides/limits.go
modules/overrides/overrides.go

[domasx2]

@ie-pham thanks! I did so, but now am completely stumped as to why CI is failing :( everything works locally and the error+ location in CI doesn't make any sense... Maybe you have a clue? 🙏

[ie-pham]

@ie-pham thanks! I did so, but now am completely stumped as to why CI is failing :( everything works locally and the error+ location in CI doesn't make any sense... Maybe you have a clue? 🙏

@domasx2
I think you need to rebase because someone had refactored that whole section and renamed a few things

1. add an entry to this section https://github.com/grafana/tempo/blob/main/modules/overrides/interface.go#L54
2. in overrides/overides.go change o *Overrides => o *overrides

[domasx2]

Thanks @ie-pham , resolved :)

[ie-pham]

This LGTM

[domasx2]

@joe-elliott can you take a look again? Feature is now opt-in, no longer a breaking change

[domasx2]

hold on with merging this for a bit, still some ongoing discussions on app o11y side

[domasx2]

good to merge 👍