opentelemetry-go icon indicating copy to clipboard operation
opentelemetry-go copied to clipboard

Impossible to create Instrument with the same name from the different MeterProvider

Open morozovcookie opened this issue 3 years ago • 1 comments

Description

More that year have passed since I wrote about this in Slack and it is still here. I am trying to use Meter API, count some data and collect it with Prometheus. It is possible situation, when you need the same Instrument name with the different prefixes. I guess that Meter name is the prefix, but not. And If I will try to create two Meters and for each one create Instrument with the same name than I will get an error.

Environment

  • OS: OS X
  • Architecture: x86_64
  • Go Version: 1.19
  • opentelemetry-go version: v1.8.0

Steps To Reproduce

config := prometheus.Config{}
	ctrl := controller.New(
		processor.NewFactory(
			selector.NewWithHistogramDistribution(
				histogram.WithExplicitBoundaries(config.DefaultHistogramBoundaries),
			),
			aggregation.CumulativeTemporalitySelector(),
			processor.WithMemory(true),
		),
	)

	exporter, err := prometheus.New(config, ctrl)
	if err != nil {
		return fmt.Errorf("setup monitor routes: %w", err)
	}

	provider := exporter.MeterProvider()

	httpInstrumentProvider := provider.Meter("http").SyncInt64()
	httpErrorCounter, err := httpInstrumentProvider.Counter("error_count", instrument.WithUnit(unit.Dimensionless),
		instrument.WithDescription("count of http request errors"))
	if err != nil {
		return fmt.Errorf("setup monitor routes: %w", err)
	}

	httpErrorCounter.Add(ctx, 1)

	sqlInstrumentProvider := provider.Meter("sql").SyncInt64()
	sqlErrorCounter, err := sqlInstrumentProvider.Counter("error_count", instrument.WithUnit(unit.Dimensionless),
		instrument.WithDescription("count of sql query errors"))
	if err != nil {
		return fmt.Errorf("setup monitor routes: %w", err)
	}

	sqlErrorCounter.Add(ctx, 1)

	be.monitorRouter.HandleFunc("/metrics", exporter.ServeHTTP)

Expected behavior

I expect that I could create different Meters which isolate their Instruments (like namespace) and Instruments with the same name will not conflict between themselves in case if it was created with different Meters.

morozovcookie avatar Sep 09 '22 20:09 morozovcookie

Blocked by https://github.com/open-telemetry/opentelemetry-specification/pull/2703

jmacd avatar Sep 12 '22 22:09 jmacd

as discussed in https://github.com/open-telemetry/opentelemetry-go/pull/3357#pullrequestreview-1154026833

this issue is also blocked by https://github.com/open-telemetry/opentelemetry-specification/pull/2890

fatsheep9146 avatar Nov 02 '22 14:11 fatsheep9146

this issue is also blocked by open-telemetry/opentelemetry-specification#2890

@fatsheep9146 this issue should not be blocked. The referenced issue is merged in the specification and we should implement a solution that address it.

Let me know if you are still able to work on this.

MrAlias avatar Nov 03 '22 18:11 MrAlias

this issue is also blocked by open-telemetry/opentelemetry-specification#2890

@fatsheep9146 this issue should not be blocked. The referenced issue is merged in the specification and we should implement a solution that address it.

Let me know if you are still able to work on this.

Yes, I am working on this. =D @MrAlias

fatsheep9146 avatar Nov 03 '22 22:11 fatsheep9146

Where is exporter.ServeHTTP gone ?

How are we supposed to serve metrics to prometheus now ? Am I missing something ?

eddycharly avatar Nov 16 '22 21:11 eddycharly

Where is exporter.ServeHTTP gone ?

How are we supposed to serve metrics to prometheus now ? Am I missing something ?

I think you can see this example as reference. @eddycharly https://github.com/open-telemetry/opentelemetry-go/tree/main/example/prometheus

fatsheep9146 avatar Nov 17 '22 02:11 fatsheep9146

@fatsheep9146 thanks, I did the same as in https://github.com/open-telemetry/opentelemetry-go/tree/main/example/prometheus.

I now see crazy metrics like kyverno_policy_changes_total_total_total_total_total_total_total_total_total_total. Any idea what's going on here ?

eddycharly avatar Nov 17 '22 10:11 eddycharly

@fatsheep9146 thanks, I did the same as in https://github.com/open-telemetry/opentelemetry-go/tree/main/example/prometheus.

I now see crazy metrics like kyverno_policy_changes_total_total_total_total_total_total_total_total_total_total. Any idea what's going on here ?

Could you paste your code? @eddycharly

fatsheep9146 avatar Nov 17 '22 10:11 fatsheep9146

Basically the same as in the example, my metric is created like this:

	m.policyChangesMetric, err = meter.SyncInt64().Counter("kyverno_policy_changes_total", instrument.WithDescription("can be used to track all the changes associated with the Kyverno policies present on the cluster such as creation, updates and deletions"))
	if err != nil {
		m.Log.Error(err, "Failed to create instrument, kyverno_policy_changes_total")
		return nil, err
	}

eddycharly avatar Nov 17 '22 13:11 eddycharly

The code initialising otel is here https://github.com/eddycharly/kyverno/blob/a8a496c8b231e1587797dc4af464ce747a524384/pkg/metrics/metrics.go#L169-L203

eddycharly avatar Nov 17 '22 13:11 eddycharly

kyverno_policy_changes_total_total_total_total_total_total_total_total_total_total

@eddycharly

First of all, the best practice to create a counter which is monotonically is not include the "total " suffix when you call Counter. The suffix will be supplied automatically by sdk itself.

So I suggest you change your code into

	m.policyChangesMetric, err = meter.SyncInt64().Counter("kyverno_policy_changes", instrument.WithDescription("can be used to track all the changes associated with the Kyverno policies present on the cluster such as creation, updates and deletions"))
	if err != nil {
		m.Log.Error(err, "Failed to create instrument, kyverno_policy_changes_total")
		return nil, err
	}

Then finally you can get the metric name kyverno_policy_changes_total

fatsheep9146 avatar Nov 21 '22 04:11 fatsheep9146

@fatsheep9146 the suggested changes didn't fix the issue, I still observe metrics like this:

# TYPE kyverno_policy_changes_total_total_total_total_total_total_total_total_total_total_total_total_total counter            

kyverno_policy_changes_total_total_total_total_total_total_total_total_total_total_total_total_total{policy_background_mode="true",policy_change_type="deleted",policy_name="tasks-no-extractor",policy_namespace="-",policy_type="cluster",policy_validation_mode="enforce"} 1

eddycharly avatar Nov 21 '22 23:11 eddycharly

@fatsheep9146 it looks like it's fixed on main but not yet in v1.11.1

eddycharly avatar Nov 22 '22 15:11 eddycharly

Offending code (should have been out of the for loop) https://github.com/open-telemetry/opentelemetry-go/blob/2fe8861a24e20088c065b116089862caf9e3cd8b/exporters/prometheus/exporter.go#L207-L210

eddycharly avatar Nov 22 '22 15:11 eddycharly

@eddycharly Yes, I think this bug maybe fixed by #3369

fatsheep9146 avatar Nov 22 '22 15:11 fatsheep9146

Any idea when the next release containing the fix might come out ?

eddycharly avatar Nov 22 '22 15:11 eddycharly

Any idea when the next release containing the fix might come out ?

I think the milestone is tracked here. https://github.com/open-telemetry/opentelemetry-go/milestone/34

@eddycharly

fatsheep9146 avatar Nov 22 '22 15:11 fatsheep9146

Thank you so much 🙏

eddycharly avatar Nov 22 '22 15:11 eddycharly