open-telemetry / opentelemetry-go

OpenTelemetry Go API and SDK
https://opentelemetry.io/docs/languages/go
Apache License 2.0
5.3k stars 1.08k forks source link

view: observable instrument not registered for callback #4666

Closed utezduyar closed 10 months ago

utezduyar commented 1 year ago

Following runnable code with conditional view is not working and it prints observable instrument not registered for callbackmessage.

package main

import (
    "encoding/json"
    "log"
    "math/rand"
    "os"
    "time"

    "go.opentelemetry.io/contrib/instrumentation/runtime"
    "go.opentelemetry.io/otel/exporters/stdout/stdoutmetric"
    "go.opentelemetry.io/otel/sdk/metric"
    metricsdk "go.opentelemetry.io/otel/sdk/metric"

    "go.opentelemetry.io/otel"
)

func main() {

    // Print with a JSON encoder that indents with two spaces.
    enc := json.NewEncoder(os.Stdout)
    enc.SetIndent("", "  ")
    exporter, err := stdoutmetric.New(
        stdoutmetric.WithEncoder(enc),
        stdoutmetric.WithoutTimestamps(),
    )
    if err != nil {
        panic(err)
    }

    reader := metricsdk.NewPeriodicReader(exporter,
        metricsdk.WithInterval(5*time.Second))

    var drop metric.View = func(i metric.Instrument) (metric.Stream, bool) {
        if i.Name != "process.runtime.go.mem.heap_alloc" {
            return metricsdk.Stream{Aggregation: metricsdk.AggregationDrop{}}, true
        }
        return metricsdk.Stream{}, false
    }

    meterProvider := metricsdk.NewMeterProvider(
        metricsdk.WithReader(reader),
        metricsdk.WithView(drop),
    )
    otel.SetMeterProvider(meterProvider)

    log.Print("Starting runtime instrumentation:")
    err = runtime.Start(runtime.WithMinimumReadMemStatsInterval(time.Second))
    if err != nil {
        panic(err)
    }

    time.Sleep(99 * time.Minute)

}

If I remove the condition from the view and drop the aggregation, it works.

    var drop metric.View = func(i metric.Instrument) (metric.Stream, bool) {
        return metricsdk.Stream{Aggregation: metricsdk.AggregationDrop{}}, true
    }
utezduyar commented 1 year ago

As I wrote on the linked issue: I started to wonder if this is a problem specific to the instrumentation/runtime package. Same way of dropping metrics with a custom View works for other scope like: go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp Either way depending on the outcome, maybe there needs to be a documentation update on the View API therefore I think the issue is relevant.

pellared commented 1 year ago

When I quickly checked the issue it looks as it is a problem related with observable instruments.

I think this error is produced for any observable instrument with drop aggregation during collection.

utezduyar commented 1 year ago

I am pretty sure I have tested it with one of the meters on go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp and didn't see the problem. Should I double check my test?

utezduyar commented 1 year ago

Oh I missed what you wrote. Seems like go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp is not observable.

utezduyar commented 11 months ago

Isn't this a regression on drop aggregation? Is there any other way of dropping some of the instruments from the libraries?

pellared commented 11 months ago

Isn't this a regression on drop aggregation?

I do not think it is a regression. It might be a bug that was always there.

Is there any other way of dropping some of the instruments from the libraries?

Nothing comes to my mind apart from creating some metric reader/exporter decorator which would exclude some metric data to be exported... 😬

scorpionknifes commented 11 months ago

I've attempted to draft out a simple fix, not sure if it's good, appreciate any feedback :pray: https://github.com/open-telemetry/opentelemetry-go/pull/4772

Basically, it's correctly dropping the observable instrument resulting it to be unregistered causing it to fail the registered check