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

Address exporter/prometheus package name collision #3172

Closed
MrAlias opened this issue Sep 14, 2022 · 3 comments · Fixed by #3239
Closed

Address exporter/prometheus package name collision #3172

MrAlias opened this issue Sep 14, 2022 · 3 comments · Fixed by #3239
Labels
area:metrics Part of OpenTelemetry Metrics pkg:exporter:prometheus Related to the Prometheus exporter package

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Sep 14, 2022

Originally posted by @dashpole in #3168 (comment)

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 MrAlias added this to the Metric SDK: Beta milestone Sep 14, 2022
@MrAlias MrAlias added area:metrics Part of OpenTelemetry Metrics pkg:exporter:prometheus Related to the Prometheus exporter package labels Sep 14, 2022
@MadVikingGod
Copy link
Contributor

MadVikingGod commented Sep 15, 2022

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.

@MrAlias
Copy link
Contributor Author

MrAlias commented Sep 15, 2022

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.

@dashpole
Copy link
Contributor

dashpole commented Sep 15, 2022

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?): 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Part of OpenTelemetry Metrics pkg:exporter:prometheus Related to the Prometheus exporter package
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants