-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
with per_host in metrics, how about compatible with old version #6604
Comments
#6531) * Add per host config * Pass host label when option is enabled * Test per host enabled * metrics: scope metrics per loaded config * doc and linter Signed-off-by: Mohammed Al Sahaf <[email protected]> * inject the custom registry into the admin handler Co-Authored-By: Dave Henderson <[email protected]> * remove `TODO` comment * fixes Signed-off-by: Mohammed Al Sahaf <[email protected]> * refactor to delay metrics admin handler provision Signed-off-by: Mohammed Al Sahaf <[email protected]> --------- Signed-off-by: Mohammed Al Sahaf <[email protected]> Co-authored-by: Hussam Almarzooq <[email protected]> Co-authored-by: Dave Henderson <[email protected]>
I don't think that's related. Can you share your full config file? I need to replicate it, because I don't see the same issue on my side. |
Here is the configuration file that can be replicated.
|
Note I have the same panic while trying to test panic: duplicate metrics collector registration attempted
goroutine 1 [running]:
github.com/prometheus/client_golang/prometheus.(*Registry).MustRegister(0xc0003d39f0, {0xc0008d36f0?, 0x1b21117?, 0x4?})
github.com/prometheus/[email protected]/prometheus/registry.go:405 +0x66
github.com/prometheus/client_golang/prometheus/promauto.Factory.NewGaugeVec({{0x1ffd3c0?, 0xc0003d39f0?}}, {{0x1b229f7, 0x5}, {0x1b21117, 0x4}, {0x1b450a6, 0x12}, {0x1b9>
github.com/prometheus/[email protected]/prometheus/promauto/auto.go:308 +0x163
github.com/caddyserver/caddy/v2/modules/caddyhttp.initHTTPMetrics({{0x2001aa8, 0xc000643bc0}, 0xc0004e0810, 0xc000284840, {0xc00073c1f0, 0x1, 0x1}, {0x0, 0x0, 0x0}, ...}>
github.com/caddyserver/caddy/[email protected]/modules/caddyhttp/metrics.go:46 +0x130
github.com/caddyserver/caddy/v2/modules/caddyhttp.newMetricsInstrumentedHandler.func1()
github.com/caddyserver/caddy/[email protected]/modules/caddyhttp/metrics.go:121 +0x38
sync.(*Once).doSlow(0x1?, 0x3?)
sync/once.go:76 +0xb4
sync.(*Once).Do(...)
sync/once.go:67
github.com/caddyserver/caddy/v2/modules/caddyhttp.newMetricsInstrumentedHandler({{0x2001aa8, 0xc000643bc0}, 0xc0004e0810, 0xc000284840, {0xc00073c1f0, 0x1, 0x1}, {0x0, 0>
github.com/caddyserver/caddy/[email protected]/modules/caddyhttp/metrics.go:120 +0xb3
github.com/caddyserver/caddy/v2/modules/caddyhttp.wrapMiddleware({{0x2001aa8, 0xc000643bc0}, 0xc0004e0810, 0xc000284840, {0xc00073c1f0, 0x1, 0x1}, {0x0, 0x0, 0x0}, ...},>
github.com/caddyserver/caddy/[email protected]/modules/caddyhttp/routes.go:321 +0x99
github.com/caddyserver/caddy/v2/modules/caddyhttp.(*Route).ProvisionHandlers(0xc00062a288, {{0x2001aa8, 0xc000643bc0}, 0xc0004e0810, 0xc000284840, {0xc00073c1f0, 0x1, 0x>
...
github.com/spf13/cobra.(*Command).execute(0xc000980608, {0xc0008fee70, 0x3, 0x3})
github.com/spf13/[email protected]/command.go:983 +0xaaa
github.com/spf13/cobra.(*Command).ExecuteC(0xc000980008)
github.com/spf13/[email protected]/command.go:1115 +0x3ff
github.com/spf13/cobra.(*Command).Execute(...)
github.com/spf13/[email protected]/command.go:1039
github.com/caddyserver/caddy/v2/cmd.Main()
github.com/caddyserver/caddy/[email protected]/cmd/main.go:75 +0x1dd
main.main()
caddy/main.go:20 +0xf |
Okay, I figured the root cause. This is hairy. The shared I need to control the registration flow within the caddy context. I wonder if it's better to swap the control and have the context register the collectors instead of the registry being given to the HTTP app. |
This change moves the metrics configuration from per-server level to a single config knob within the `http` app. Enabling `metrics` in any of the configured servers inside `http` enables metrics for all servers. Fix #6604 Signed-off-by: Mohammed Al Sahaf <[email protected]>
@ta2013 @steffenbusch, can you test the CI artifacts of #6606 (here) to validate it works? I've tested it locally, and the panic is gone. |
Appreciate the swift fix; everything's running smoothly now. 👍 |
I have used |
I observed that r.Host="example.com" differs from r.Host="exAmple.com". Shouldn't the metrics for these labels be treated as identical? |
Yep. I've pushed a commit to normalize them. |
Thanks. It works! |
current caddyfile is using:
if i switch to caddy@master (with per_host support in metrics), then above config will cause panic!! because missing :port (ie servers :80, servers :443 )
the full log:
The text was updated successfully, but these errors were encountered: