Allow non-default registry to be used for metrics of SD components by ptodev · Pull Request #13023 · prometheus/prometheus

ptodev · 2023-10-23T21:15:00Z

This PR makes it possible to register metrics of SD components using a registry other than the global one. This is going to be very useful for applications such as Grafana Agent which import Prometheus SDs from the Prometheus repo and instantiate more than one SD per process.

This PR is a follow-up from #12565. This PR does for SD mechanisms what #12958 does for the "scrape" package.

Note that the Kubernetes SD mechanism registers metrics from the Go k8s client. Those metrics are per process and not per SD instance, so I moved them outside of the SD mechanism. They are now registered when the SD Manager metrics are registered, once per process.

Also note that since we register the metrics during the Run() function, we are not able to use the metrics during the SD creation because the Run() function hasn't ran yet. I believe this will not be a problem.

I successfully tested the change locally:

I used file_sd_configs and kubernetes_sd_configs. I see all the k8s metrics and file SD metrics whenever those SDs are enabled in the config.
I tested Prometheus with both the --enable-feature=new-service-discovery-manager cmd option and without. In both cases I get the same metrics.
I tried reloading the Prometheus config using a pkill -SIGHUP -i prometheus command. This worked ok, Prometheus reloaded the config and kept running, and the same metrics were visible.

I did not test promtool.

beorn7 · 2023-10-24T16:41:37Z

I'll have a look ASAP. Could you fix the DCO and the lint issues in the meantime?

ptodev · 2023-10-24T21:24:11Z

I'll have a look ASAP. Could you fix the DCO and the lint issues in the meantime?

Thank you! The CI is green now.

beorn7

Epic work. Thank you very much. I think you can avoid having the separate registration of "shared" metrics by using ConstLabels or WrapRegistererWith. Feel free to ask if you would like to discuss this approach more. Let's try this out before I do the detailed review.

cmd/prometheus/main.go

beorn7 · 2023-11-29T19:38:01Z

Thank you very much. I hope I'll get to this tomorrow.

beorn7 · 2023-12-05T15:06:14Z

I'm still trying very hard to get to this. In the meantime, could you address the lint issues? They seem easy to fix.

ptodev · 2023-12-06T09:57:36Z

I'm still trying very hard to get to this. In the meantime, could you address the lint issues? They seem easy to fix.

Yes, thank you, this is done now.

discovery/kubernetes/endpoints.go

beorn7

Very nice. I have just one concern about the "late WithLabelValues calls inside the anonymous functions in the K8s SD. (I think I sent the first comment directly and not part of this review.) If you have done this on purpose and already know that the performance impact doesn't matter, then fine. Otherwise, I would recommend to do the WithLabelValues call once "outside the loop".

And then there is a merge conflict.

But once those two things are addressed, we can finally merge. Thank you very much.

discovery/kubernetes/endpoints.go

Signed-off-by: Paulin Todev <paulin.todev@gmail.com>

This was flagged by the linter. Signed-off-by: Paulin Todev <paulin.todev@gmail.com>

Signed-off-by: Paulin Todev <paulin.todev@gmail.com>

ptodev · 2023-12-12T10:40:43Z

@beorn7 Thank you for the review! I addressed the issues which you mentioned. Also, the branch is rebased.

ptodev · 2023-12-12T12:54:18Z

discovery/kubernetes/endpoints.go

+
+	svcAddCount := eventCount.WithLabelValues(RoleService.String(), MetricLabelRoleAdd)
+	svcUpdateCount := eventCount.WithLabelValues(RoleService.String(), MetricLabelRoleUpdate)
+	svcDeleteCount := eventCount.WithLabelValues(RoleService.String(), MetricLabelRoleDelete)


@beorn7 I thought it'd be nice to define string constants such as "endpoints", "endpointslice", "add", "update", and "delete" only once and use them as constants instead of rewriting the string. I hope you don't mind.

There are different camps in Prometheus land. Some people argue, if you see a label value in your metrics, you should be able to find it directly in the code with grep. I don't feel strongly, and I understand that string constants are matching most developers' exceptions much better.

beorn7

Thank you very much. Epic work.

bwplotka · 2023-12-19T17:16:46Z

NOTE: It seems this PR might have broke something: #13312

…-register-metrics-globally-sd" This reverts commit 9825a80, reversing changes made to 669cad6.

ptodev requested review from brancz and dgl as code owners October 23, 2023 21:15

ptodev mentioned this pull request Oct 23, 2023

Metrics in the "scrape" package can now be registered with a non-default registry #12958

Merged

beorn7 self-assigned this Oct 24, 2023

beorn7 self-requested a review October 24, 2023 16:38

beorn7 removed their assignment Oct 24, 2023

ptodev force-pushed the prefer-to-not-register-metrics-globally-sd branch from f2c25df to bfeac67 Compare October 24, 2023 19:03

beorn7 requested changes Nov 9, 2023

View reviewed changes

cmd/prometheus/main.go Outdated Show resolved Hide resolved

ptodev force-pushed the prefer-to-not-register-metrics-globally-sd branch 2 times, most recently from 0c04f6a to 8d8894b Compare November 15, 2023 17:14

ptodev requested a review from beorn7 November 15, 2023 17:17

ptodev force-pushed the prefer-to-not-register-metrics-globally-sd branch 2 times, most recently from f801b57 to f50b965 Compare November 23, 2023 10:04

ptodev force-pushed the prefer-to-not-register-metrics-globally-sd branch from f50b965 to 7f71a10 Compare November 29, 2023 23:29

ptodev force-pushed the prefer-to-not-register-metrics-globally-sd branch 3 times, most recently from 2e5c9f8 to 1d48f6a Compare December 6, 2023 09:25

beorn7 reviewed Dec 7, 2023

View reviewed changes

discovery/kubernetes/endpoints.go Outdated Show resolved Hide resolved

beorn7 requested changes Dec 7, 2023

View reviewed changes

discovery/kubernetes/endpoints.go Outdated Show resolved Hide resolved

ptodev added 3 commits December 11, 2023 11:14

Allow non-default registry to be used for metrics of SD components

6de80d7

Signed-off-by: Paulin Todev <paulin.todev@gmail.com>

Use const labels for Discovery Manager metrics.

6a5306a

Signed-off-by: Paulin Todev <paulin.todev@gmail.com>

Remove unnecessary else.

6279497

This was flagged by the linter. Signed-off-by: Paulin Todev <paulin.todev@gmail.com>

ptodev force-pushed the prefer-to-not-register-metrics-globally-sd branch from 1d48f6a to 5be085c Compare December 11, 2023 13:19

ptodev added 2 commits December 11, 2023 13:39

Set up labels for counters in advance

108a749

Signed-off-by: Paulin Todev <paulin.todev@gmail.com>

Define metric label values in one place

27bb57a

Signed-off-by: Paulin Todev <paulin.todev@gmail.com>

ptodev force-pushed the prefer-to-not-register-metrics-globally-sd branch from 5be085c to 27bb57a Compare December 11, 2023 13:39

Fix linter issues

d2e9970

Signed-off-by: Paulin Todev <paulin.todev@gmail.com>

ptodev force-pushed the prefer-to-not-register-metrics-globally-sd branch from 3d2e1fc to d2e9970 Compare December 11, 2023 14:28

ptodev requested a review from beorn7 December 12, 2023 10:39

ptodev commented Dec 12, 2023

View reviewed changes

beorn7 approved these changes Dec 12, 2023

View reviewed changes

beorn7 merged commit 9825a80 into prometheus:main Dec 12, 2023

ptodev mentioned this pull request Dec 20, 2023

scraping/alerting: Fix issues with duplicate metric registration #13316

Closed

marctc added a commit to grafana/prometheus that referenced this pull request Jan 4, 2024

Revert "Merge pull request prometheus#13023 from ptodev/prefer-to-not…

92240af

…-register-metrics-globally-sd" This reverts commit 9825a80, reversing changes made to 669cad6.

chenrui333 mentioned this pull request Feb 22, 2024

prometheus 2.50.0 Homebrew/homebrew-core#163774

Merged

Conversation

ptodev commented Oct 23, 2023

Uh oh!

beorn7 commented Oct 24, 2023

Uh oh!

ptodev commented Oct 24, 2023

Uh oh!

beorn7 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

beorn7 commented Nov 29, 2023

Uh oh!

beorn7 commented Dec 5, 2023

Uh oh!

ptodev commented Dec 6, 2023

Uh oh!

Uh oh!

beorn7 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ptodev commented Dec 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ptodev Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

beorn7 Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

beorn7 left a comment

Choose a reason for hiding this comment

Uh oh!

bwplotka commented Dec 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ptodev commented Dec 12, 2023 •

edited

Loading