port servicegraph processor code to connector #26092

rlankfo · 2023-08-24T05:15:38Z

Description:
This PR ports the deprecated servicegraph processor code to continue support on the connector.

Link to tracking Issue:
#26091

Testing:
make test

Documentation:
TODO: Documentation should be updated to include the new names of the featuregates which are changed from processor.servicegraph.* to connector.servicegraph.*

jpkrohling

I have a question about the metrics consumer when creating a new connector, but other than that, LGTM. I verified that the business of this connector matches the one from the processor.

jpkrohling · 2023-08-31T08:35:23Z

connector/servicegraphconnector/config.go

+type Config struct {
+
+	// MetricsExporter is the name of the metrics exporter to use to ship metrics.
+	MetricsExporter string `mapstructure:"metrics_exporter"`


Do we still need this as a connector? Won't the connector get a metrics consumer on its TracesToMetrics constructor?

We don't need this anymore. Good catch!

jpkrohling · 2023-08-31T08:36:30Z

connector/servicegraphconnector/connector.go

+func (p *serviceGraphConnector) Start(_ context.Context, host component.Host) error {
+	p.store = store.NewStore(p.config.Store.TTL, p.config.Store.MaxItems, p.onComplete, p.onExpire)
+
+	if p.metricsConsumer == nil {


This looks odd: as a traces to metrics connector, I'd expect the metrics consumer to always be set.

This should always be set -- this is also left over from the processor logic. I removed these nil checks in another place but missed this one.

connector/servicegraphconnector/factory.go

connector/servicegraphconnector/connector.go

rlankfo

Thanks for the review @jpkrohling

The leftover MetricsExporter on the config and the nil checks on the metricsConsumer were leftover behavior from the processor. I removed some of this behavior initially but missed these spots you called out.

I also removed the legacy metric name feature gate.

rlankfo · 2023-09-08T06:00:24Z

connector/servicegraphconnector/config.go

+type Config struct {
+
+	// MetricsExporter is the name of the metrics exporter to use to ship metrics.
+	MetricsExporter string `mapstructure:"metrics_exporter"`


We don't need this anymore. Good catch!

rlankfo · 2023-09-08T06:03:59Z

connector/servicegraphconnector/connector.go

+func (p *serviceGraphConnector) Start(_ context.Context, host component.Host) error {
+	p.store = store.NewStore(p.config.Store.TTL, p.config.Store.MaxItems, p.onComplete, p.onExpire)
+
+	if p.metricsConsumer == nil {


This should always be set -- this is also left over from the processor logic. I removed these nil checks in another place but missed this one.

mapno

LGTM.

When is the processor removed?

mapno · 2023-09-11T08:07:39Z

connector/servicegraphconnector/factory.go

+	virtualNodeFeatureGate = featuregate.GlobalRegistry().MustRegister(
+		virtualNodeFeatureGateID,
+		featuregate.StageAlpha,
+		featuregate.WithRegisterDescription("When enabled, when the edge expires, connector 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.WithRegisterReferenceURL("https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/17196"),
+	)


IMO, it's a good opportunity to remove this gate and instead default to a smaller number of attributes or none

thanks @mapno that sounds good to me. I will make these changes and request another review.

jpkrohling

Other than the feature gate, this looks good. I have a question about the future of this component though: should we strive to combine this with the span metrics connector? If this is something to consider for the future, do we need this connector here as an intermediary step between the processor and the span metrics connector?

connector/servicegraphconnector/config.go

Co-authored-by: Juraci Paixão Kröhling <[email protected]>

rlankfo · 2023-09-11T19:18:03Z

When is the processor removed?

I didn't have a plan to remove the processor yet, but once this is merged I suppose there's no reason not to remove it? cc @jpkrohling

rlankfo · 2023-09-11T19:22:19Z

I have a question about the future of this component though: should we strive to combine this with the span metrics connector? If this is something to consider for the future, do we need this connector here as an intermediary step between the processor and the span metrics connector?

@jpkrohling I'd like to see this functionality combined with the span metrics connector. I think this code change probably makes sense still given that the connector already exists and the processor is deprecated. What do you think?

jpkrohling · 2023-09-12T08:39:47Z

I'd like to see this functionality combined with the span metrics connector.

Let's get it there then. The current connector is in alpha state, we can still easily deprecate/remove it. I'll open an issue to discuss that with the current code owner (@albertteoh).

jpkrohling · 2023-09-12T08:46:36Z

Opened #26648 to discuss this.

github-actions · 2023-09-27T05:19:55Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions bot added the connector/servicegraph label Aug 24, 2023

github-actions bot requested review from jpkrohling and mapno August 24, 2023 05:15

rlankfo force-pushed the rlankfo/servicegraphconnector branch 2 times, most recently from 9d867c6 to 4d7c3fc Compare August 24, 2023 05:31

jpkrohling reviewed Aug 31, 2023

View reviewed changes

rlankfo commented Sep 8, 2023

View reviewed changes

rlankfo force-pushed the rlankfo/servicegraphconnector branch from 3997a00 to 6d81c76 Compare September 8, 2023 06:28

rlankfo marked this pull request as ready for review September 8, 2023 06:28

rlankfo requested a review from a team September 8, 2023 06:28

github-actions bot assigned dashpole Sep 8, 2023

rlankfo requested a review from jpkrohling September 8, 2023 06:33

rlankfo added 3 commits September 8, 2023 09:17

port servicegraphprocessor code to servicegraphconnector

6608b06

remove leftover processor logic and MetricsExporter config

e92b298

fix lint

4a9d497

rlankfo force-pushed the rlankfo/servicegraphconnector branch from 8279de5 to 4a9d497 Compare September 8, 2023 16:17

mapno reviewed Sep 11, 2023

View reviewed changes

jpkrohling reviewed Sep 11, 2023

View reviewed changes

connector/servicegraphconnector/config.go Outdated Show resolved Hide resolved

Update connector/servicegraphconnector/config.go

557b6c6

Co-authored-by: Juraci Paixão Kröhling <[email protected]>

github-actions bot added the Stale label Sep 27, 2023

rlankfo closed this Oct 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

port servicegraph processor code to connector #26092

port servicegraph processor code to connector #26092

rlankfo commented Aug 24, 2023

jpkrohling left a comment

jpkrohling Aug 31, 2023

rlankfo Sep 8, 2023

jpkrohling Aug 31, 2023

rlankfo Sep 8, 2023

rlankfo left a comment •

edited

Loading

rlankfo Sep 8, 2023

rlankfo Sep 8, 2023

mapno left a comment

mapno Sep 11, 2023

rlankfo Sep 11, 2023

jpkrohling left a comment

rlankfo commented Sep 11, 2023

rlankfo commented Sep 11, 2023

jpkrohling commented Sep 12, 2023

jpkrohling commented Sep 12, 2023

github-actions bot commented Sep 27, 2023

port servicegraph processor code to connector #26092

port servicegraph processor code to connector #26092

Conversation

rlankfo commented Aug 24, 2023

jpkrohling 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

rlankfo left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

rlankfo commented Sep 11, 2023

rlankfo commented Sep 11, 2023

jpkrohling commented Sep 12, 2023

jpkrohling commented Sep 12, 2023

github-actions bot commented Sep 27, 2023

rlankfo left a comment •

edited

Loading