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

Allow adding arbitrary labels to metrics when generating output #741

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

willnewton
Copy link

It's useful to be able to apply a label across all metrics, for
example when using a multi-processing framework to apply a
label for worker ID to all metrics (including from builtin
collector types). This change adds a new Registry type that
applies a dictionary of labels when metrics are collected.

Signed-off-by: Will Newton [email protected]

It's useful to be able to apply a label across all metrics, for
example when using a multi-processing framework to apply a
label for worker ID to all metrics (including from builtin
collector types). This change adds a new Registry type that
applies a dictionary of labels when metrics are collected.

Signed-off-by: Will Newton <[email protected]>
Copy link
Member

@csmarchbanks csmarchbanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so this is a bit backwards of what I was imagining, I apologize for not being clear in the other comment/misunderstanding your goal. The use case I would support is what is described in client_golang/prometheus#WrapRegistererWith. This allows systems such as distinct workers to register the same set of metrics with a distinct label, but would not apply to every metric being exported. For more details on why we don't encourage labels on every metric see https://prometheus.io/docs/instrumenting/writing_exporters/#target-labels-not-static-scraped-labels.

Does that make sense/would it help your use case?

samples with additional labels given in the dictionary.

Intended usage is:
generate_latest(REGISTRY.labelled_registry(['a_timeseries']))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example is incorrect as it should be passing a dict of constant labels.

@Sircular
Copy link

Sircular commented Oct 7, 2022

@csmarchbanks I'm a little confused by your comment, since the Python client doesn't seem to have the concept of a Registerer like the Golang client. Is the idea that we would create a wrapper class that contains a Registry which augments the labels of a collector when .register() is called? Then we could make multiple wrappers that add different labels, and they could all reference the same registry.

@csmarchbanks
Copy link
Member

csmarchbanks commented Oct 7, 2022

I can imagine a few implementations that would be acceptable, but my guess is that we will have a new class that inherits from CollectorRegistry but overrides collect() to add the labels to each metric as they are yielded rather than adding labels to the collectors. You could then do something like:

REGISTRY.register(labeled_registry)

to add all of the labeled metrics to the default registry or any other registry. Does that make sense?

I might even be ok with just adding a const_labels variable when initializing a CollectorRegistry, but not sure if that would be best compared to a new class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants