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

Address exporter/prometheus package name collision

Open MrAlias opened this issue 3 years ago • 3 comments
trafficstars

Originally posted by @dashpole in https://github.com/open-telemetry/opentelemetry-go/pull/3168#discussion_r970987617

Currently, the prometheus exporter package name is guaranteed to conflict with an import of "github.com/prometheus/client_golang/prometheus". This second import is required now because the prometheus exporter only returns a prometheus.Collector that needs to be registered with that package. This is not ideal for users as they will always need to override a package name when they setup the exporter.

Possible solutions

Rename the exporter

go.opentelemetry.io/otel/exporters/otelprometheus has been proposed.

Provide a registration method

The exporter could have a registration method that returns an http.Handler.

Add a Must function

Add a MustRegister function that returns the Exporter after registering it with a passed prom registerer or the global one

MrAlias avatar Sep 14 '22 19:09 MrAlias

The previous version of the prometheus exporter did not need this because of how it was structured. It was possible to pass a prometheus Registry (WithRegistry) and if it wasn't supplied it would create one, and then the exporter would expose the http.Handler (ServeHTTP()). This means clients would only need to provide a registry, and thus import prometheus, if they needed it for something other then initialization.

I think this is something that can be added to the new version.

MadVikingGod avatar Sep 15 '22 14:09 MadVikingGod

The previous version of the prometheus exporter did not need this because of how it was structured. It was possible to pass a prometheus Registry (WithRegistry) and if it wasn't supplied it would create one, and then the exporter would expose the http.Handler (ServeHTTP()). This means clients would only need to provide a registry, and thus import prometheus, if they needed it for something other then initialization.

I think this is something that can be added to the new version.

It was pointed out in #3135 that this approach would require the New function to return an error as well as an Exporter.

MrAlias avatar Sep 15 '22 14:09 MrAlias

Documenting a few experiments:

  • Adding WithRegisterer seems about the same in terms of ease-of-use as what we have now.
  • Making the exporter also an http handler does remove the need to import upstream prometheus (if I can get it to not panic?): https://github.com/open-telemetry/opentelemetry-go/compare/new_sdk/main...dashpole:opentelemetry-go:http_experiment. But we would still want to expose the underlying registry somehow, and allow configuration of the promhttp opts.

dashpole avatar Sep 15 '22 20:09 dashpole