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

Prometheus server panic on invalid instrument name #3180

Closed
MrAlias opened this issue Sep 16, 2022 · 1 comment · Fixed by #3182
Closed

Prometheus server panic on invalid instrument name #3180

MrAlias opened this issue Sep 16, 2022 · 1 comment · Fixed by #3182
Assignees
Labels
bug Something isn't working pkg:exporter:prometheus Related to the Prometheus exporter package

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Sep 16, 2022

Description

Running the prometheus exporter with an instrument that has an invalid name causes the HTTP server to panic.

Environment

  • Go Version: 1.19
  • opentelemetry-go version: e1a1f07

Steps To Reproduce

go.mod:

module go.opentelemetry.io/otel/example/prometheus

go 1.18

require (
	github.com/prometheus/client_golang v1.13.0
	go.opentelemetry.io/otel v1.10.0
	go.opentelemetry.io/otel/exporters/prometheus v0.31.0
	go.opentelemetry.io/otel/metric v0.31.0
	go.opentelemetry.io/otel/sdk/metric v0.31.0
)

require (
	github.com/beorn7/perks v1.0.1 // indirect
	github.com/cespare/xxhash/v2 v2.1.2 // indirect
	github.com/go-logr/logr v1.2.3 // indirect
	github.com/go-logr/stdr v1.2.2 // indirect
	github.com/golang/protobuf v1.5.2 // indirect
	github.com/matttproud/golang_protobuf_extensions v1.0.1 // indirect
	github.com/prometheus/client_model v0.2.0 // indirect
	github.com/prometheus/common v0.37.0 // indirect
	github.com/prometheus/procfs v0.8.0 // indirect
	go.opentelemetry.io/otel/sdk v1.10.0 // indirect
	go.opentelemetry.io/otel/trace v1.10.0 // indirect
	golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a // indirect
	google.golang.org/protobuf v1.28.1 // indirect
)

replace go.opentelemetry.io/otel => ../..

replace go.opentelemetry.io/otel/exporters/prometheus => ../../exporters/prometheus

replace go.opentelemetry.io/otel/sdk => ../../sdk

replace go.opentelemetry.io/otel/sdk/metric => ../../sdk/metric

replace go.opentelemetry.io/otel/metric => ../../metric

replace go.opentelemetry.io/otel/trace => ../../trace

main.go:

package main

import (
	"context"
	"log"
	"net/http"
	"os"
	"os/signal"

	"github.com/prometheus/client_golang/prometheus"
	"github.com/prometheus/client_golang/prometheus/promhttp"

	otelprom "go.opentelemetry.io/otel/exporters/prometheus"
	"go.opentelemetry.io/otel/metric/instrument"
	"go.opentelemetry.io/otel/sdk/metric"
)

func main() {
	ctx := context.Background()

	// The exporter embeds a default OpenTelemetry Reader and
	// implements prometheus.Collector, allowing it to be used as
	// both a Reader and Collector.
	exporter := otelprom.New()
	provider := metric.NewMeterProvider(metric.WithReader(exporter))
	meter := provider.Meter("github.com/open-telemetry/opentelemetry-go/example/prometheus")

	// Start the prometheus HTTP server and pass the exporter Collector to it
	registry := prometheus.NewRegistry()
	err := registry.Register(exporter.Collector)
	if err != nil {
		log.Fatal(err)
	}

	log.Printf("serving metrics at localhost:2222/metrics")
	http.Handle("/metrics", promhttp.HandlerFor(registry, promhttp.HandlerOpts{}))
	go http.ListenAndServe(":2222", nil)

	histogram, err := meter.SyncFloat64().Histogram(
		"invalid.name.latency",
		instrument.WithDescription("a histogram with an invalid name"),
	)
	if err != nil {
		log.Fatal(err)
	}
	histogram.Record(ctx, 23)

	ctx, _ = signal.NotifyContext(ctx, os.Interrupt)
	<-ctx.Done()
}
  1. Run the above code
    $ go run .
    2022/09/16 13:09:46 serving metrics at localhost:2222/metrics
    
  2. Poll the endoint: curl localhost:2222/metrics
  3. Get a panic

Output:

2022/09/16 13:05:27 "invalid.name.latency" is not a valid metric name
2022/09/16 13:05:27 http: panic serving [::1]:59118: runtime error: invalid memory address or nil pointer dereference
goroutine 5 [running]:
net/http.(*conn).serve.func1()
	/usr/lib/go/src/net/http/server.go:1850 +0xbf
panic({0x85d220, 0xc5fa70})
	/usr/lib/go/src/runtime/panic.go:890 +0x262
github.com/prometheus/client_golang/prometheus.processMetric({0x0, 0x0}, 0x0?, 0x0?, 0x0)
	/home/tyler/go/pkg/mod/github.com/prometheus/client_golang@v1.13.0/prometheus/registry.go:598 +0x4b
github.com/prometheus/client_golang/prometheus.(*Registry).Gather(0xc00012ee60)
	/home/tyler/go/pkg/mod/github.com/prometheus/client_golang@v1.13.0/prometheus/registry.go:536 +0x9ed
github.com/prometheus/client_golang/prometheus.(*noTransactionGatherer).Gather(0x8?)
	/home/tyler/go/pkg/mod/github.com/prometheus/client_golang@v1.13.0/prometheus/registry.go:1042 +0x22
github.com/prometheus/client_golang/prometheus/promhttp.HandlerForTransactional.func1({0x990028, 0xc0000c4000}, 0xc0000ae000)
	/home/tyler/go/pkg/mod/github.com/prometheus/client_golang@v1.13.0/prometheus/promhttp/http.go:135 +0xfe
net/http.HandlerFunc.ServeHTTP(0xc000137af0?, {0x990028?, 0xc0000c4000?}, 0x0?)
	/usr/lib/go/src/net/http/server.go:2109 +0x2f
net/http.(*ServeMux).ServeHTTP(0x0?, {0x990028, 0xc0000c4000}, 0xc0000ae000)
	/usr/lib/go/src/net/http/server.go:2487 +0x149
net/http.serverHandler.ServeHTTP({0xc000094240?}, {0x990028, 0xc0000c4000}, 0xc0000ae000)
	/usr/lib/go/src/net/http/server.go:2947 +0x30c
net/http.(*conn).serve(0xc0000000a0, {0x9909d8, 0xc000094150})
	/usr/lib/go/src/net/http/server.go:1991 +0x607
created by net/http.(*Server).Serve
	/usr/lib/go/src/net/http/server.go:3102 +0x4db

Expected behavior

No Panic

@MrAlias MrAlias added bug Something isn't working pkg:exporter:prometheus Related to the Prometheus exporter package labels Sep 16, 2022
@MrAlias
Copy link
Contributor Author

MrAlias commented Sep 16, 2022

Panic is originating from: https://github.com/prometheus/client_golang/blob/64435fc00ac419bb878a3f9c9658e8353c19a7cd/prometheus/registry.go#L598

We are sending metrics to the prom channel even if there is an error:

for _, metricData := range getMetricData(metrics) {
if metricData.valueType == prometheus.UntypedValue {
m, err := prometheus.NewConstHistogram(metricData.description, metricData.histogramCount, metricData.histogramSum, metricData.histogramBuckets, metricData.attributeValues...)
if err != nil {
otel.Handle(err)
}
ch <- m
} else {
m, err := prometheus.NewConstMetric(metricData.description, metricData.valueType, metricData.value, metricData.attributeValues...)
if err != nil {
otel.Handle(err)
}
ch <- m

The sent metrics are nil. We need to continue if there's an error. E.g.

	for _, metricData := range getMetricData(metrics) {
		if metricData.valueType == prometheus.UntypedValue {
			m, err := prometheus.NewConstHistogram(metricData.description, metricData.histogramCount, metricData.histogramSum, metricData.histogramBuckets, metricData.attributeValues...)
			if err != nil {
				otel.Handle(err)
				continue
			}
			ch <- m
		} else {
			m, err := prometheus.NewConstMetric(metricData.description, metricData.valueType, metricData.value, metricData.attributeValues...)
			if err != nil {
				otel.Handle(err)
				continue
			}
			ch <- m
		}
	}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pkg:exporter:prometheus Related to the Prometheus exporter package
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant