-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Description
Telemetry initialization is currently split between the service/telemetry
package and the service
package in the telemetry.go
package. This causes some issues:
- The
telemetry.TracerProvider
method returns a tracer provider that is never used in practice. It is used hereopentelemetry-collector/service/service.go
Line 103 in 3089ea8
TracerProvider: srv.telemetry.TracerProvider(), opentelemetry-collector/service/service.go
Line 115 in 3089ea8
srv.telemetrySettings.TracerProvider = srv.telemetryInitializer.tp opentelemetry-collector/service/telemetry.go
Lines 101 to 111 in 3089ea8
func (tel *telemetryInitializer) initTraces(res *resource.Resource, cfg telemetry.Config) (trace.TracerProvider, error) { opts := []sdktrace.TracerProviderOption{} for _, processor := range cfg.Traces.Processors { sp, err := proctelemetry.InitSpanProcessor(context.Background(), processor) if err != nil { return nil, err } opts = append(opts, sdktrace.WithSpanProcessor(sp)) } return proctelemetry.InitTracerProvider(res, opts) } telemetry.TracerProvider
. I believe because of this the sampling configuration set onservice/telemetry
is ignored.opentelemetry-collector/service/telemetry/telemetry.go
Lines 47 to 50 in 3089ea8
tp := sdktrace.NewTracerProvider( // needed for supporting the zpages extension sdktrace.WithSampler(alwaysRecord()), ) - Parts of telemetry are initialized in
service/telemetry
, while others are initialized inservice
. In particular, the logger is initialized inservice/telemetry
, the tracer provider can be initialized in both but only theservice
one is used, and the metrics provider is only initialized inservice
.
I think we should consolidate telemetry initialization to the service/telemetry
package and have methods to generate the telemetry providers from config as part of the public API. This would mean adding a new method to telemetry.Telemetry
to handle registration of process metrics, currently done in the service via the telemetry initializer
opentelemetry-collector/service/service.go
Lines 210 to 211 in 3089ea8
// The process telemetry initialization requires the ballast size, which is available after the extensions are initialized. | |
if err = proctelemetry.RegisterProcessMetrics(srv.telemetryInitializer.ocRegistry, srv.telemetryInitializer.mp, obsreportconfig.UseOtelForInternalMetricsfeatureGate.IsEnabled(), getBallastSize(srv.host)); err != nil { |
One challenge here is that proctelemetry
depends on service/telemetry
for the generated configuration; if we don't want to expose the OpenCensus registry we need to refactor the current package structure (e.g. by having a new service/telemetry/config
package).
cc @codeboten since this may overlap with the work on #7532