Skip to content

allow setting labels common to all metrics#93

Closed
ghost wants to merge 1 commit into
prometheus:masterfrom
Showmax:master
Closed

allow setting labels common to all metrics#93
ghost wants to merge 1 commit into
prometheus:masterfrom
Showmax:master

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Oct 12, 2018

Hi @grobie,

I'd like to be able to have a set of labels common to all metrics. For example a Puma worker_id for all Puma services. And obviously I want to avoid setting those labels in constructor of every metric.

One thing I was wondering about was whether to make it a static hash or a proc, but I didn't come up with a use case for the proc, so it's a hash.

Thanks

Signed-off-by: Daniel Veleba <daniel.veleba@showmax.com>
@coveralls
Copy link
Copy Markdown

coveralls commented Oct 12, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 5871d99 on Showmax:master into 300de85 on prometheus:master.

@grobie
Copy link
Copy Markdown
Member

grobie commented Oct 12, 2018

Thank you for your contribution @daniel-veleba.

Exporting common labels to all metrics from clients is considered in anti-pattern in Prometheus. We call these target labels, as they are static per target. The Prometheus server is better suited to apply such labels, either via relabel rules extracted from meta labels or applied directory in static_configs.

@ghost
Copy link
Copy Markdown
Author

ghost commented Oct 15, 2018

Thank you for such a quick response!

Apologies for poor description. Two misunderstandings happened here:

  • I didn't mean really labels common to all metrics, rather common to a set of metrics. Eg. Puma specific, MQ specific etc. I understand this can be handled with static_config.
  • The values of the labels wouldn't be static. They wouldn't change throughout the runtime of the given service, but would change with service restarts. The puma worker_id is a good example here: we're running Puma in clustered mode, meaning it spawns several processes and distributes the HTTP requests among these processes. Thus metrics like HTTP request counts have different values per the process, so we need a label identifying the worker. Using pid doesn't seem a good idea, as it would have a huge cardinality. So we want to use the worker_id, an information internal to the service. And I don't see a way how to handle this via static_config.

Another example are service tags. We're tagging services to classify them. Tags look like ruby, mq, rails, puma etc. This is definitely something that can be stored in a hostname - [log_tags] map and thus solved with static_config. OTOH, that would mean creating the map in the first place, then making it available to the Prom server. Which seems extraneous, given the service already knows its tags. Thus the idea to apply such labels within the service.

Hope it makes more sense now.

@ghost
Copy link
Copy Markdown
Author

ghost commented Oct 17, 2018

ping @grobie ^^^

@dmagliola
Copy link
Copy Markdown
Collaborator

Hello Daniel!
Sorry for the late response!

As @grobie has mentioned, doing this goes against Prometheus Best Practices, and we'd like to keep the client as much in line with them as possible.
These are the best practices, for reference: https://prometheus.io/docs/instrumenting/writing_clientlibs/#labels

However, there's an easy way to implement this in your codebase without needing to modify the official client.
The version that is now in master, which is a release candidate that'll become 1.0 soon, allows you to specify preset_labels when initializing a metric. These will be merged with any labelset you specify for this metric.

One way of getting around specifying the same labels over and over is to have a central method you use to declare your metrics. We use this in GoCardless as a way of automating things that apply to all metrics, validate that the metric names meet our conventions, etc.

If you're ok with that, I'd like to close this PR.

@ghost
Copy link
Copy Markdown
Author

ghost commented Jun 26, 2019

Nice, thanks for your response Daniel. Closing

This pull request was closed.
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