Merge span metrics and service graph into span metrics connector

[jpkrohling]

Component(s)

connector/servicegraph, connector/spanmetrics, processor/servicegraph, processor/spanmetrics

Describe the issue you're reporting

I would like to combine those four components into one, given that they, at their cores, they are about converting spans to metrics in a way or another:

• span metrics processor
• service graph processor
• span metrics connector
• service graph connector

Besides, I would like the result to use the semantic conventions as much as possible. Given that the current span metrics connector is still alpha, we should probably be fine in determining the metrics to be deprecated in the next version already, changing them to their new names in N+2 already (0.87.0, if everything goes as planned).

For work to start on this, I need buy-in from the current span metrics processor/connector code owner, @albertteoh. Given that @mapno and @rlankfo worked on service graph processor and will work on the connector, it would also make sense to make them code owners.

@albertteoh, are you on board with the proposed changes?

[github-actions]

Pinging code owners:

• connector/spanmetrics: @albertteoh
• processor/spanmetrics: @albertteoh

See Adding Labels via Comments if you do not have permissions to add labels yourself.

[albertteoh]

Thanks for opening this discussion, @jpkrohling.

I would like to combine those four components into one, given that they, at their cores, they are about converting spans to metrics in a way or another

What are the primary motivations for consolidation?

I can definitely see benefits of less maintenance overhead by decommissioning the deprecated processor variants, assuming this is what you mean by combining.

I'm just a little less clear on the benefits of consolidating the service graph and spanmetrics connectors, though I agree, they both aggregate spans to metrics, but they serve different use cases AFAICT.

determining the metrics to be deprecated in the next version already, changing them to their new names in N+2 already (0.87.0, if everything goes as planned).

Are you able to please elaborate more on this or point me to an issue that details the proposed name changes and rationale?

[jpkrohling]

What are the primary motivations for consolidation?

It really is about having two similar components being consolidated. The use-cases are somewhat different, but we've seen that they are mostly used together.

Are you able to please elaborate more on this or point me to an issue that details the proposed name changes and rationale?

The idea is to use the semantic conventions as much as possible. I haven't done a summary of changes yet, but I assume there might be things that do not conform with the conventions or things that didn't exist when the component was first created. If you are OK with the general idea, we'd move forward and compile a list of changes, which shouldn't be many.

[albertteoh]

I'm okay with the general idea, and I'm also in favour of having more owners to the connector, which will benefit users because we'll have more resources available to handle feature/bug requests and questions.

Let's move forward on this.

[github-actions]

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

• processor/spanmetrics: @albertteoh
• processor/servicegraph: @jpkrohling @mapno
• connector/servicegraph: @jpkrohling @mapno
• connector/spanmetrics: @albertteoh

See Adding Labels via Comments if you do not have permissions to add labels yourself.

[JaredTan95]

Good idea. Currently spanmetrics and servicegraph processor have similar configurations and functions in many places.
Like the recent ongoing requirement for servicegraph: #29628 #27881. If the two processors can be maintained together. I think it's more reasonable.

[mapno]

My concern with merging these two components is that they have different requirements. Servicegraph needs for all spans of a trace to be processed together in order to extract connections (HTTP requests, db calls, etc), whereas spanmetrics can process spans independently of each other. By merging, we'd be introducing this limitation to the spanmetrics connector/processor.

[JaredTan95]

My concern with merging these two components is that they have different requirements. Servicegraph needs for all spans of a trace to be processed together in order to extract connections (HTTP requests, db calls, etc), whereas spanmetrics can process spans independently of each other. By merging, we'd be introducing this limitation to the spanmetrics connector/processor.

BTW,I have a question.For now,we can using otel col LB to route span data from the same traceid to the same otel col instance. Can we use an external caching component(For example, extension the store with external cache component,maybe like redis) to make the graph processor stateless so I don't have to use LB?

[fstab]

spanmetrics can process spans independently of each other.

The most complex scenario here is: Tail sampling and exemplars. If spanmetrics process spans independent of each other, how to make sure that traces containing an exemplar are sampled?

[mapno]

an we use an external caching component(For example, extension the store with external cache component,maybe like redis) to make the graph processor stateless so I don't have to use LB?

Actually, yes. It's not a bad idea. What we'd need is an implementation of the store pkg that works with an external db/cache. It could even be shared by multiple collector instances if it can be made safe with transactions or similar techniques.

[JaredTan95]

an we use an external caching component(For example, extension the store with external cache component,maybe like redis) to make the graph processor stateless so I don't have to use LB?

Actually, yes. It's not a bad idea. What we'd need is an implementation of the store pkg that works with an external db/cache. It could even be shared by multiple collector instances if it can be made safe with transactions or similar techniques.

Yes, we've recently tried to implement this internally, and if it goes well, we'll contribute upstream