Configure service telemetry #5

ptodev · 2023-06-20T10:29:49Z

Grafana Agent uses code from the Collector. However, in order to integrate with it we need features which are not available in the Collector yet, hence the need for this forked repo. So far we dealt with this by exporting all Collector internals. This PR is an attempt at a more lightweight approach, where we only modify the minimum bits of functionality that we need.

There is also an upstream PR to merge this functionality into the Collector itself, or at least to get some feedback on how appropriate this approach is.

rfratto

Left some feedback, I know this is in our fork but I'd like @jpkrohling to sign off on whether this is going roughly in the right direction.

We should aim to upstream this as soon as we can so we don't have to keep using the fork, and the current state makes me a little nervous that keeping it in sync will be a bit of a pain.

rfratto · 2023-06-20T10:31:58Z

otelcol/configprovider.go

@@ -84,7 +84,7 @@ func (cm *configProvider) Get(ctx context.Context, factories Factories) (*Config
 	}

 	var cfg *configSettings
-	if cfg, err = unmarshal(conf, factories); err != nil {
+	if cfg, err = Unmarshal(conf, factories); err != nil {


Do we have to expose this method? It seems like downstream we could use NewConfigProvider.Get instead to do the same thing.

ConfigProvider can only get config from URIs. In our case the Agent parses its own config file, and we can't give the whole file to ConfigProviderbecause it contains non-Otel config. We could theoretically add new functionality to ConfigProviderbut I don't think it'll be accepted upstream since what we do here is not really what it is meant to do.

I'm not sure I have the full context, but you could have another config provider specific to agent. We have a few config providers already, like the file provider, HTTP provider, and so on. Look at confmap/provider for example of implementations.

rfratto · 2023-06-20T10:32:10Z

otelcol/unmarshaler.go

@@ -29,7 +29,7 @@ type configSettings struct {

 // unmarshal the configSettings from a confmap.Conf.
 // After the config is unmarshalled, `Validate()` must be called to validate.
-func unmarshal(v *confmap.Conf, factories Factories) (*configSettings, error) {
+func Unmarshal(v *confmap.Conf, factories Factories) (*configSettings, error) {


See above, I'm not sure if we need to expose this.

rfratto · 2023-06-20T10:34:37Z

service/telemetry.go

@@ -91,8 +91,8 @@ func newColTelemetry(useOtel bool, disableHighCardinality bool, extendedConfig b
 	}
 }

-func (tel *telemetryInitializer) init(res *resource.Resource, settings component.TelemetrySettings, cfg telemetry.Config, asyncErrorChannel chan error) error {


The diff in this file is pretty significant. Is this the absolute smallest unit of change we could be doing?

I'm worried about the overhead of maintaining the fork with the number of merge conflicts this is susceptible to.

rfratto · 2023-06-20T10:37:08Z

service/telemetry/telemetry.go

@@ -39,18 +43,29 @@ type Settings struct {
 }

 // New creates a new Telemetry from Config.
-func New(_ context.Context, set Settings, cfg Config) (*Telemetry, error) {
+func New(_ context.Context, set Settings, cfg Config, tracerProvider trace.TracerProvider) (*Telemetry, error) {


Have you discussed these changes with @jpkrohling?

Something feels a bit off to me here with how the service-level providers are being injected down through the call stack; it doesn't feel like it's on the right trajectory for code that could eventually be merged.

I have not discussed this particular change with @jpkrohling. I only discussed with him the changes in the upstream PR.

I know that passing a TracerProvider is a bit hacky. It is not something I expect to be merged upstream. The reason I'm doing it here is because I noticed the Agent used to use a noop TracerProvider before, and I wanted to match this post-upgrade because I don't think we need it. As far as I can tell Otel uses it for the zpagesextension which we don't use in the Agent.

We have an effort upstream to instrument the collector, including for tracing. I'm quite confident the solution upstream is going to be quite different than what is here, but again, @mar4uk has a better context when it comes to this effort.

There is a proposal to make the collector observable open-telemetry#7532 upstream.
But PRs are not moving for quite some time: open-telemetry#7644
So it is difficult to say when this proposal will be implemented and go live. Probably not very soon

ptodev · 2023-06-20T14:25:02Z

We should aim to upstream this as soon as we can so we don't have to keep using the fork, and the current state makes me a little nervous that keeping it in sync will be a bit of a pain.

@rfratto Honestly I doubt that the upstream PR will get merged, for reasons already mentioned in its description. On Slack it was mentioned that most likely the way forward is going to be in not using the service package at all, and instead the graph (previously pipelines) package will be made public so that people could build their own services.

The one change which maybe has a chance of being upstreamed is the one for disabling Collector's /metrics endpoint. But IMO it'd need to be implemented differently so that it's configured through yaml.

jpkrohling · 2023-06-21T19:30:41Z

otelcol/configprovider.go

@@ -84,7 +84,7 @@ func (cm *configProvider) Get(ctx context.Context, factories Factories) (*Config
 	}

 	var cfg *configSettings
-	if cfg, err = unmarshal(conf, factories); err != nil {
+	if cfg, err = Unmarshal(conf, factories); err != nil {


I'm not sure I have the full context, but you could have another config provider specific to agent. We have a few config providers already, like the file provider, HTTP provider, and so on. Look at confmap/provider for example of implementations.

jpkrohling · 2023-06-21T19:31:42Z

service/service.go

@@ -57,6 +59,14 @@ type Settings struct {
 	// LoggingOptions provides a way to change behavior of zap logging.
 	LoggingOptions []zap.Option

+	OtelMetricViews []sdkmetric.View


I know @mar4uk has been following the discussions around instrumenting the collector itself. Perhaps she could provide some feedback to this part of the PR?

jpkrohling · 2023-06-21T19:33:34Z

service/telemetry/telemetry.go

@@ -39,18 +43,29 @@ type Settings struct {
 }

 // New creates a new Telemetry from Config.
-func New(_ context.Context, set Settings, cfg Config) (*Telemetry, error) {
+func New(_ context.Context, set Settings, cfg Config, tracerProvider trace.TracerProvider) (*Telemetry, error) {


We have an effort upstream to instrument the collector, including for tracing. I'm quite confident the solution upstream is going to be quite different than what is here, but again, @mar4uk has a better context when it comes to this effort.

rfratto

Alright, I don't want to block this, and we anticipate it to be temporary anyway (especially since Flow doesn't need it). Let's get it merged for now; we can always revisit in future updates if there's a different approach we could take if we're having a hard time maintaining this.

To resolve the govulncheck reports: ``` Vulnerability #1: GO-2023-1987 Large RSA keys can cause high CPU usage in crypto/tls More info: https://pkg.go.dev/vuln/GO-2023-1987 Standard library Found in: crypto/[email protected] Fixed in: crypto/[email protected] Example traces found: Error: #1: service/internal/proctelemetry/config.go:299:27: proctelemetry.initOTLPgRPCExporter calls otlpmetricgrpc.New, which eventually calls tls.Conn.Handshake Error: #2: service/internal/proctelemetry/config.go:156:39: proctelemetry.InitPrometheusServer calls http.Server.ListenAndServe, which eventually calls tls.Conn.HandshakeContext Error: #3: service/service.go:251:36: service.buildResource calls uuid.NewRandom, which eventually calls tls.Conn.Read Error: #4: service/config.go:35:13: service.Config.Validate calls fmt.Printf, which eventually calls tls.Conn.Write Error: #5: service/telemetry/telemetry.go:32:28: telemetry.Telemetry.Shutdown calls trace.TracerProvider.Shutdown, which eventually calls tls.Dialer.DialContext ``` https://github.com/open-telemetry/opentelemetry-collector/actions/runs/5753675727/job/15597394973?pr=8144

ptodev added 4 commits June 20, 2023 11:14

Export otelcol.Unmarshal

d624388

Pass a metric reader and metric views

7d0efc0

Support external metric servers

f0b9d0a

Override tracer provider

b88df00

ptodev requested a review from rfratto June 20, 2023 10:29

ptodev mentioned this pull request Jun 20, 2023

Upgrade OpenTelemetry Collector dependency grafana/agent#3858

Merged

rfratto reviewed Jun 20, 2023

View reviewed changes

jpkrohling reviewed Jun 21, 2023

View reviewed changes

rfratto approved these changes Jun 22, 2023

View reviewed changes

ptodev merged commit 1841af9 into grafana:0.79-grafana Jun 30, 2023

ptodev mentioned this pull request Jun 30, 2023

Configure service telemetry on 0.80 #6

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configure service telemetry #5

Configure service telemetry #5

ptodev commented Jun 20, 2023

rfratto left a comment

rfratto Jun 20, 2023

ptodev Jun 20, 2023

jpkrohling Jun 21, 2023

rfratto Jun 20, 2023

rfratto Jun 20, 2023

rfratto Jun 20, 2023

ptodev Jun 20, 2023

jpkrohling Jun 21, 2023

mar4uk Jun 22, 2023

ptodev commented Jun 20, 2023

jpkrohling Jun 21, 2023

jpkrohling Jun 21, 2023

jpkrohling Jun 21, 2023

rfratto left a comment •

edited

Loading

Configure service telemetry #5

Configure service telemetry #5

Conversation

ptodev commented Jun 20, 2023

rfratto 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

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

ptodev commented Jun 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfratto left a comment • edited Loading

Choose a reason for hiding this comment

rfratto left a comment •

edited

Loading