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

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

Closed
pragmaticivan opened this issue Apr 27, 2022 · 7 comments
Closed

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

pragmaticivan opened this issue Apr 27, 2022 · 7 comments
Labels
bug Something isn't working

Comments

@pragmaticivan
Copy link

pragmaticivan commented Apr 27, 2022

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

  • OS: Alpine (docker container)
  • Architecture: x86
  • Go Version: 1.18
  • opentelemetry-go version: v1.6.3

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.

@pragmaticivan pragmaticivan added the bug Something isn't working label Apr 27, 2022
@fatsheep9146
Copy link
Contributor

Could you provide more information abount variable runtimeMetrics and hostMetrics?

For example, how are those two variables initialized ?

@pragmaticivan
Copy link
Author

@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
Copy link

veqryn commented Jun 16, 2022

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
Copy link

veqryn commented Jun 16, 2022

@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
Copy link

veqryn commented Jun 16, 2022

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
Copy link
Member

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
Copy link
Contributor

MrAlias commented Oct 12, 2022

Closing, stale. This should be resolved by the new SDK merged in #3175

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants