Skip to content

Add an easy way to define preset labels globally#174

Closed
stefansundin wants to merge 1 commit intoprometheus:masterfrom
stefansundin:add-global-preset-labels
Closed

Add an easy way to define preset labels globally#174
stefansundin wants to merge 1 commit intoprometheus:masterfrom
stefansundin:add-global-preset-labels

Conversation

@stefansundin
Copy link
Contributor

Hi again,

With this change, I will be able to do the following:

prometheus = Prometheus::Client.registry(labels: %i[app_version], preset_labels: { app_version: ENV["HEROKU_RELEASE_VERSION"] })

And easily see the version that my metrics were emitted by. I can now clearly see if behavior changed when the version changed, and the code will be pretty clean too.

I wish that I wouldn't need to set both :labels and :preset_labels.. It feels overly verbose. I almost think preset_labels.keys should be merged into labels automatically, but that would be a breaking change, so I didn't try to include it here.

I don't know if there are any philosophical objections to something like this, but I figured that I should throw it out there. Thanks!

Signed-off-by: Stefan Sundin <stefan@stefansundin.com>
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 619e533 on stefansundin:add-global-preset-labels into 7129453 on prometheus:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 619e533 on stefansundin:add-global-preset-labels into 7129453 on prometheus:master.

@brian-brazil
Copy link

I don't know if there are any philosophical objections to something like this, but I figured that I should throw it out there. Thanks!

This should be done as target labels on the Prometheus side, an application doesn't know how a scraper views it.

@lawrencejones
Copy link
Contributor

lawrencejones commented Dec 28, 2019

To expand on Brian's comment, it's usually advised that metrics exported from an application are label-less unless absolutely necessary.

As I'm not sure how familiar you are with Prometheus, it's worth saying that the time series you view in prometheus will have both the labels exported in your application, plus what is applied during the service discovery relabelling that you configure in prometheus.

An example up metric from one of our apps may look like this when you curl the metrics endpoint:

up 1

But have the following labels once ingested by prometheus:

up{app="appetiser",chart="gc-app-1.0.0",heritage="Tiller",instance="appetiser-worker-default-585cb8f8ff-fqfxk",job="kubernetes-pods",namespace="staging",node="gke-prd-cluster-primary-07-8e13a6ea-4xbw",pod_template_hash="585cb8f8ff",port="8081",release="appetiser",role="worker",version="9449ec8aa941afda33d8368cefce3c9a8eba50e2",worker="default"} 1

It's often a better idea to have your labels be assigned during the relabelling than use 'default' labels that application itself configures. Adding a feature like this PR would suggest that the latter method is a good idea, when we're keen to discourage that pattern.

Let me know if any of this doesn't make sense.

@genebean
Copy link

FWIW, this setting would simplify some code I am currently adding to an application.

@lawrencejones
Copy link
Contributor

Hey @genebean, can you help us understand what you're trying to solve so we can develop a use case for this? Right now we want people to be using service discovery relabelling to apply these types of metric label, so unless we find a circumstance that can't be solved with that method, we'll be unable to consider merging this.

@dmagliola
Copy link
Collaborator

I agree with the views of @lawrencejones and @brian-brazil. We are trying to actively discourage this usage, as the preferred approach is to use service discovery relabelling.
Could you tell us more about your use case where that doesn't solve the issue?

@dmagliola
Copy link
Collaborator

I'm closing this PR since the consensus seems to be against doing this.
If we've got a clear use case, we can reopen the discussion.

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.

6 participants