Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move telemetry initialization logic to service/telemetry #8170

Open
mx-psi opened this issue Aug 3, 2023 · 1 comment
Open

Move telemetry initialization logic to service/telemetry #8170

mx-psi opened this issue Aug 3, 2023 · 1 comment
Labels
area:service collector-telemetry healthchecker and other telemetry collection issues enhancement New feature or request priority:p2 Medium

Comments

@mx-psi
Copy link
Member

mx-psi commented Aug 3, 2023

Telemetry initialization is currently split between the service/telemetry package and the service package in the telemetry.go package. This causes some issues:

  1. The telemetry.TracerProvider method returns a tracer provider that is never used in practice. It is used here
    TracerProvider: srv.telemetry.TracerProvider(),
    and then immediately overridden by the telemetry initializer one.
    srv.telemetrySettings.TracerProvider = srv.telemetryInitializer.tp
    The telemetry initializer tracer provider is built from scratch
    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)
    }
    without using telemetry.TracerProvider. I believe because of this the sampling configuration set on service/telemetry is ignored.
    tp := sdktrace.NewTracerProvider(
    // needed for supporting the zpages extension
    sdktrace.WithSampler(alwaysRecord()),
    )
  2. Parts of telemetry are initialized in service/telemetry, while others are initialized in service. In particular, the logger is initialized in service/telemetry, the tracer provider can be initialized in both but only the service one is used, and the metrics provider is only initialized in service.

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

// 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

@mx-psi mx-psi added enhancement New feature or request priority:p2 Medium area:service collector-telemetry healthchecker and other telemetry collection issues labels Aug 3, 2023
@atoulme atoulme added the release:required-for-ga Must be resolved before GA release label Dec 19, 2023
codeboten pushed a commit that referenced this issue Jan 30, 2024
…kage (#9384)

Second redo of
#8171 that
does not depend on
#9131

Link to tracking Issue: Updates
#8170

---------

Co-authored-by: Alex Boten <[email protected]>
@mx-psi mx-psi moved this to Todo in Collector: v1 Apr 18, 2024
@mx-psi mx-psi removed this from Collector: v1 Jul 31, 2024
@mx-psi
Copy link
Member Author

mx-psi commented Jul 31, 2024

Removed this from Collector v1 project board based on #10643

@mx-psi mx-psi removed the release:required-for-ga Must be resolved before GA release label Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:service collector-telemetry healthchecker and other telemetry collection issues enhancement New feature or request priority:p2 Medium
Projects
None yet
Development

No branches or pull requests

3 participants