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

Add a WithNamespace option to the Prometheus Exporter #3437

Closed
paivagustavo opened this issue Nov 2, 2022 · 15 comments · Fixed by #3970
Closed

Add a WithNamespace option to the Prometheus Exporter #3437

paivagustavo opened this issue Nov 2, 2022 · 15 comments · Fixed by #3970
Labels
enhancement New feature or request pkg:exporter:prometheus Related to the Prometheus exporter package proposal

Comments

@paivagustavo
Copy link
Member

paivagustavo commented Nov 2, 2022

Problem Statement

As an user of the Prometheus Exporter, I would like to be able to set a namespace prefix for my exported metrics.

Proposed Solution

A new prometheus.WithNamespace(prefix) option that would add the given prefix to all metrics except meta metrics, i.e., target_info metrics shouldn't be prefixed since they have some special behavior.

Alternatives

One could use a prometheus wrapped registerer that is passed for the Prometheus Exporter, e.g, prometheus.WrapRegistererWithPrefix(namespace, registry), but that adds the namespace prefix to the target_info metrics as well.

Prior Art

This is a common option existing on various prometheus exporter including Otel Collector's Contrib Prometheus Remote Write Exporter and Prometheus Exporter, OpenCensus and other prometheus libraries as well.

@paivagustavo paivagustavo added the enhancement New feature or request label Nov 2, 2022
@paivagustavo paivagustavo changed the title Add a namespace option to the Prometheus Exporter Add a WithNamespace option to the Prometheus Exporter Nov 2, 2022
@MrAlias MrAlias added the pkg:exporter:prometheus Related to the Prometheus exporter package label Nov 2, 2022
@paivagustavo
Copy link
Member Author

btw, If there is no objection, I can work on adding it.

@MrAlias
Copy link
Contributor

MrAlias commented Nov 2, 2022

Is this something other OTel language implementations have?

@MrAlias
Copy link
Contributor

MrAlias commented Nov 2, 2022

This could also be resolved via view right?

@paivagustavo
Copy link
Member Author

Is this something other OTel language implementations have?

Didn't do a full search, but found that otel python implements it.

This could also be resolved via view right?

If I could specify a view that matched multiple instruments, probably? Something like a regex matcher, that make the groups available during the stream creation.

@MrAlias
Copy link
Contributor

MrAlias commented Nov 2, 2022

If I could specify a view that matched multiple instruments, probably? Something like a regex matcher, that make the groups available during the stream creation.

Hopefully we are headed there, but even then it looks like a common task others have solved at the exporter level.

I'm in favor of the option.

@MrAlias
Copy link
Contributor

MrAlias commented Nov 2, 2022

The full definition would be something like this, right?

func WithNamespace(prefix string) Option

@MrAlias
Copy link
Contributor

MrAlias commented Nov 3, 2022

@dashpole mentioned in today's SIG meeting that it is going to be uncommon for users to want to prefix multiple instrumentation libraries with the same prefix.

This view refactor proposal may help to address this. If accepted a user could define a name replacement view based on the existing instrument name and if it matches a scope.

Additionally, if we wanted to, we could provide something like:

func NamespaceView(prefix string, scope instrumentation.Scope) View

@MrAlias
Copy link
Contributor

MrAlias commented Nov 3, 2022

We could also try to solve the scope->prefix issue with just an option for the exporter. E.g.

func WithNamespace(prefix string, scope instrumentation.Scope) Option

We will need to recreate match criteria for the scope matching here.

@paivagustavo
Copy link
Member Author

@dashpole mentioned in today's SIG meeting that it is going to be uncommon for users to want to prefix multiple instrumentation libraries with the same prefix.

I understand and agree that this it not the ideal for most of the cases and we wouldn't recommend users to do it. Although I do think this is fairly common for existing codes that uses OpenCensus and other instrumentation libraries. Which can make the transition to Otel painful.

I would still advocate for having a WithNamespace just as other prometheus libraries already exposes. And having the NamespaceView as a recommendation for future use cases that wants to prefix their scope metrics.

@MrAlias
Copy link
Contributor

MrAlias commented Nov 3, 2022

I understand and agree that this it not the ideal for most of the cases and we wouldn't recommend users to do it.

I'm less excited to add this if we know it is not ideal. If there is a better solution I would be in favor of that instead of carrying forward non-ideal behavior from prior systems.

It seems to me that both alternative approaches outlined above could be used by OpenCensus users migrating to OTel, right?

@paivagustavo
Copy link
Member Author

It seems to me that both alternative approaches outlined above could be used by OpenCensus users migrating to OTel, right?

I guess if we could match all scopes with those views then yes. Since I don't think users would be migrating to Otel under a single scope.

@MrAlias
Copy link
Contributor

MrAlias commented Nov 3, 2022

It seems to me that both alternative approaches outlined above could be used by OpenCensus users migrating to OTel, right?

I guess if we could match all scopes with those views then yes. Since I don't think users would be migrating to Otel under a single scope.

Seems like it would be up to us to define the match criteria. If you define the function to consider the scope parameters as a "match any" for zero-value fields, than an empty scope would mean match all.

@MrAlias
Copy link
Contributor

MrAlias commented Nov 3, 2022

You could also look for a "wildcard" of say Name: "*" that would match all, but I think that might also give the impression all wildcards are supported. Probably not ideal.

@MrAlias
Copy link
Contributor

MrAlias commented Dec 9, 2022

Moving to the post-GA project. This seems like something we can add in a backwards compatible manner and isn't required for a GA.

@paivagustavo
Copy link
Member Author

paivagustavo commented Dec 18, 2022

Example of users that are affected because we don't have such option:

#3405 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request pkg:exporter:prometheus Related to the Prometheus exporter package proposal
Projects
Development

Successfully merging a pull request may close this issue.

2 participants