feat(metrics)!: add server label to per-worker series#2396
feat(metrics)!: add server label to per-worker series#2396nicolas-grekas wants to merge 1 commit intophp:mainfrom
Conversation
Workers declared in distinct php_server blocks today share a single "worker" Prometheus label dimension, so metric series collide. This adds a sibling "server" label so each (server, worker) pair stays on its own series. Plumbing: - New Scope opaque type + NextScope() in scope.go: a per-php_server identifier the embedder allocates once per declarative block. - ScopeLabel(s) / SetScopeLabel(s, label): registry mapping Scope to a human-readable label, falling back to the numeric id when none is set. Empty labels are ignored. - WithWorkerScope(Scope) tags a declared worker; worker.scope copies it through newWorker. - Caddy module: each FrankenPHPModule provisions its own scope and on the first ServeHTTP resolves a label via the cascade (route host matcher -> user-set caddy server name -> first listen addr) and registers it via SetScopeLabel. API change: - BREAKING: every Metrics interface method that took (name string) now takes (server, name string). Embedders implementing frankenphp.Metrics need to widen their signatures; PrometheusMetrics and the null impl are updated in-tree. Mirrors the shape of php#1376 (which introduced the "worker" label the same way). - basicLabels becomes {"server", "worker"}. The scope mechanism is independently reusable for other per-server features (future per-server isolation, dispatching, etc.).
|
I could also move caddy/scopelabel.go et al from #2393 here. For now, I organized these PRs so that they can be merged independently. |
henderkes
left a comment
henderkes
left a comment
There was a problem hiding this comment.
Do we really need this? Worker names are already unique so where's the win here? If a user sets multiple workers per domain? But this is per-php_server so even for that I'm not sure.
If it's just to better distinguish between different workers without a specific name set, why not change the automatically generated prefix instead?
| // isAutoServerName reports whether name is one of Caddy's auto-assigned | ||
| // server names (srv0, srv1, ...). Anything else is treated as user-set. | ||
| func isAutoServerName(name string) bool { | ||
| if !strings.HasPrefix(name, "srv") || len(name) <= 3 { |
There was a problem hiding this comment.
this seems like a very common thing to prefix servers with, likely not the best check
I'm going to go this way, this should work: from the php side, bgworkers are scoped and from the metrics side they are prefixed. For the prefix, I'll reuse what's currently in caddy/scopelabel.go, aka prefix = explicit servers.name or (first)domain or listen address in this order. |
|
Let me close here, thanks for the direction. |
2 participants
Summary
Today's per-worker Prometheus metrics carry only a
workerlabel. Twophp_serverblocks declaring workers with the same name therefore collide on the same metric series. This PR adds a siblingserverlabel so each(server, worker)pair stays on its own series.What changes
scope.go):Scopeopaque type,NextScope()per-block allocator, and aScopeLabel/SetScopeLabelregistry that maps a Scope to a human-readable label.FrankenPHPModuleprovisions its own scope and, on the firstServeHTTP, resolves a label via cascade — first host of the route's host matcher → user-set Caddy server name → first listener address → numeric id fallback. Registered once viasync.Once.(name string)now takes(server, name string). Mirrors the shape of introduces worker name option, use label on worker metrics instead #1376 which introduced theworkerlabel the same way.PrometheusMetricsandnullMetricsare updated in-tree; thebasicLabelsbecomes{\"server\", \"worker\"}.docs/metrics.mdreflects the new label set + cascade.Breaking change
Embedders implementing
frankenphp.Metricsneed to widen their method signatures. No runtime behaviour change for users who don't implement that interface.Test plan