open-telemetry / opentelemetry-go

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

otlpmetric not able to export delta histograms with metric SDK #2868

Closed pragmaticivan closed 1 year ago

pragmaticivan commented 2 years ago

Description

Currently trying to instrument a go app by having multiple data examples for metrics.

One of the examples is a histogram. The vendor (otlp receiver) only accepts deltas for histograms.

Environment

Steps To Reproduce

    pusher := controller.New(
        processor.NewFactory(
            simple.NewWithHistogramDistribution(
                histogram.WithExplicitBoundaries([]float64{0.001, 0.01, 0.1, 0.5, 1, 2, 5, 10}),
            ),
            aggregation.DeltaTemporalitySelector(),
        ),
        controller.WithExporter(metricExporter),
        controller.WithResource(c.Resource),
        controller.WithCollectPeriod(period),
    )

    if err = pusher.Start(context.Background()); err != nil {
        return nil, fmt.Errorf("failed to start controller: %v", err)
    }

    if err = runtimeMetrics.Start(runtimeMetrics.WithMeterProvider(pusher)); err != nil {
        return nil, fmt.Errorf("failed to start runtime metrics: %v", err)
    }

    if err = hostMetrics.Start(hostMetrics.WithMeterProvider(pusher)); err != nil {
        return nil, fmt.Errorf("failed to start host metrics: %v", err)
    }

    metricglobal.SetMeterProvider(pusher)

When using a histogram.

Error: 2022/04/27 17:32:24 error: cumulative to delta not implemented

Expected behavior

It should be able to send OTLP histogram to a remote OTLP API

Note: I've also tried that with aggregation.CumulativeTemporalitySelector but not data gets sent.

fatsheep9146 commented 2 years ago

Could you provide more information abount variable runtimeMetrics and hostMetrics?

For example, how are those two variables initialized ?

pragmaticivan commented 2 years ago

@fatsheep9146 you can actually ignore these 2.

Full code available here https://github.com/pragmaticivan/otel-pipeline-client-go/blob/main/pipelines/metrics.go

I ended up finding a workaround to use something else, but deltas are still not available, and that seems to be a known issue. Some folks created their own selectors or recommended using a collector to convert cumulative data. Which is weird, because a lot of players only accept deltas nowadays.

veqryn commented 2 years ago

I just ran into this too. New Relic doesn't support cumulative histograms. They only support delta histograms. (Also, they prefer delta temporality for counters (sum metrics) as well). As far as I know, delta temporality is more popular among metric storage services.

otResource, err := resource.New(ctx, resource.WithAttributes(keyValues...))

metricClient := otlpmetricgrpc.NewClient()
metricsExporter := otlpmetric.NewUnstarted(metricClient, otlpmetric.WithMetricAggregationTemporalitySelector(aggregation.DeltaTemporalitySelector()))

metricsController := controller.New(
    processor.NewFactory(
        simple.NewWithHistogramDistribution(),
        metricsExporter,
    ),
    controller.WithResource(otResource),
    controller.WithExporter(metricsExporter),
    controller.WithCollectPeriod(metricsCollectionPeriod),
)

global.SetMeterProvider(metricsController)
metricsExporter.Start(ctx)
metricsController.Start(ctx)

meter := global.Meter("mypackage")
thingHist, err := meter.SyncFloat64().Histogram("thething")

startT := time.Now()
thing()
thingHist.Record(ctx, time.Now().Sub(startT).Seconds())

Gets me lots of these errors: cumulative to delta not implemented

veqryn commented 2 years ago

@jmacd I saw you did PR #2350 , and I am wondering where it was decided that Open Telemetry histograms wouldn't support delta temporality, and why you made the decision to remove that from this repo describing it as 'no great loss'?

Per New Relic's docs, they don't support cumulative https://docs.newrelic.com/docs/more-integrations/open-source-telemetry-integrations/opentelemetry/best-practices/opentelemetry-best-practices-metrics

I've also tried switching to cumulative temporality, but my histogram metrics don't come through (new relic says there are errors), and my sum (counter) metrics all get turned into Gauges, which is not what I want at all.

veqryn commented 2 years ago

I'm not sure how I thought this was caused by histograms. Maybe it was the title of this ticket, after I google searched the cumulative to delta not implemented? Maybe it was because the last things I added to my repo was histograms. Anyway, the problem isn't histograms, the problem is CounterObserverInstrumentKind and UpDownCounterObserverInstrumentKind. Both of which don't need delta aggregations, and work with cumulative just fine for new relic at least.

type newRelicTemporalitySelector struct{}
func (s newRelicTemporalitySelector) TemporalityFor(desc *sdkapi.Descriptor, kind aggregation.Kind) aggregation.Temporality {
    if desc.InstrumentKind() == sdkapi.CounterInstrumentKind ||
        desc.InstrumentKind() == sdkapi.HistogramInstrumentKind {
        return aggregation.DeltaTemporality
    }
    return aggregation.CumulativeTemporality
}

Consider this resolved for me at least. Not sure about @pragmaticivan

TylerHelmuth commented 2 years ago

The SDK should support exporting delta CounterObserverInstrumentKind and UpDownCounterObserverInstrumentKind though. Let me know if I need to open a separate issue or if that capability is already being tracked somewhere.

MrAlias commented 1 year ago

Closing, stale. This should be resolved by the new SDK merged in https://github.com/open-telemetry/opentelemetry-go/pull/3175