Skip to content

Conversation

chalabi2
Copy link

@chalabi2 chalabi2 commented Jun 4, 2025

This PR adds rate limit metrics requested in #79 to Caddy's internal metrics registry

@chalabi2 chalabi2 changed the title add metrics feat: add metrics Jun 4, 2025
@chalabi2 chalabi2 closed this Jun 4, 2025
@chalabi2 chalabi2 reopened this Jun 4, 2025
@chalabi2 chalabi2 marked this pull request as draft June 4, 2025 22:34
@chalabi2 chalabi2 marked this pull request as ready for review June 4, 2025 22:41
metrics.go Outdated
Comment on lines 96 to 98
metricsOnce.Do(func() {
globalMetrics = initializeMetrics(reg)
})
Copy link
Contributor

Choose a reason for hiding this comment

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

This will stop working on config reload, and no metrics will be registered or emitted. Unfortunately, the only way to manage it for now is to check for duplicate registration error, and ignore it.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the review! I updated it as per your request.

Copy link
Contributor

Choose a reason for hiding this comment

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

Now it will panic if the rate_limiter is used multiple times in Caddy because internally it calls MustRegister, and prometheus will see an existing registration

Copy link
Author

Choose a reason for hiding this comment

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

Thanks!

  • Removed sync.Once to allow re-registration on config reloads.
  • Now directly call initializeMetrics(reg) in registerMetrics.
  • globalMetrics is only set if nil, keeping metrics functional across reloads.
  • Duplicate registration errors are now ignored (returning nil as suggested).

This way, metrics continue working after reloads without panics, and Prometheus should handle any duplicate registrations gracefully.

@chalabi2 chalabi2 requested a review from mohammed90 June 12, 2025 22:27
@mohammed90
Copy link
Contributor

I'm trying to address the issue of duplicate registration in caddyserver/caddy#7051. Do you have any feedback to give on that there?

@chalabi2
Copy link
Author

I'm trying to address the issue of duplicate registration in caddyserver/caddy#7051. Do you have any feedback to give on that there?

Hey sorry for the delay.

I dont have any explicit feedback on your recommended solution in the PR you linked. Sorry this is my first foray into using caddy in production and just noticed some things were missing for my specific use case.

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.

2 participants