Add lifecycle endpoint to clear metrics - #714
Conversation
Signed-off-by: YuMin Kim <gimyumin40@gmail.com>
362b2e4 to
b4405a9
Compare
pedro-stanaka
left a comment
pedro-stanaka
left a comment
There was a problem hiding this comment.
Need some changes to fix potential bugs and breakage of interface for library users.
| } | ||
|
|
||
| // ClearMetrics clears all dynamically registered StatsD time series. | ||
| func (b *Exporter) ClearMetrics() int { |
There was a problem hiding this comment.
This synchronously sends on an unbuffered channel and waits for a reply that is only processed inside Listen(). If called before Listen() starts, after it exits, or while it is stuck, callers can block indefinitely. This is especially risky now that ClearMetrics is public and callable by library users outside the binary's lifecycle flow.
Suggestion: Consider guarding send/receive with lifecycle state. A robust pattern is a done channel closed when Listen exits, then using select in ClearMetrics() for both send and receive paths (including case <-done:) so calls fail fast instead of hanging indefinitely.
There was a problem hiding this comment.
Thanks, good catch. I updated ClearMetrics to take a context.Context and return (int, error) so callers do not block indefinitely when the exporter listener is not running.
It now tracks the Listen lifecycle, returns ErrExporterNotRunning before/after the event loop, and still serializes successful clear requests through the exporter loop. The HTTP handler now returns 503 if clearing cannot be performed.
| GetHistogram(metricName string, labels prometheus.Labels, help string, mapping *mapper.MetricMapping, metricsCount *prometheus.GaugeVec) (prometheus.Observer, error) | ||
| GetSummary(metricName string, labels prometheus.Labels, help string, mapping *mapper.MetricMapping, metricsCount *prometheus.GaugeVec) (prometheus.Observer, error) | ||
| RemoveStaleMetrics() | ||
| ClearMetrics() int |
There was a problem hiding this comment.
Adding ClearMetrics() int to the exported exporter.Registry interface is source-incompatible for downstream users that provide custom registry implementations or mocks. Because this repository is also consumed as a library, this change can cause immediate compile failures on upgrade even when runtime behavior is otherwise correct.
Suggestion: Consider keeping the exported Registry contract unchanged and introducing an internal optional capability interface (for example, type clearableRegistry interface { ClearMetrics() int }) behind a type assertion where clearing is needed. This preserves compatibility for existing library consumers.
There was a problem hiding this comment.
Done. I restored the exported Registry interface so downstream custom implementations and mocks do not need to add ClearMetrics.
Clearing is now handled through an internal optional clearableRegistry capability interface, with ErrClearMetricsUnsupported returned if the configured registry does not support it. I also added a test registry without ClearMetrics to cover this compatibility case.
|
|
||
| The `statsd_exporter` has an optional lifecycle API (disabled by default) that can be used to reload or quit the exporter | ||
| by sending a `PUT` or `POST` request to the `/-/reload` or `/-/quit` endpoints. | ||
| The `statsd_exporter` has an optional lifecycle API (disabled by default) that can be used to reload, quit, or clear the exporter |
There was a problem hiding this comment.
| The `statsd_exporter` has an optional lifecycle API (disabled by default) that can be used to reload, quit, or clear the exporter | |
| The `statsd_exporter` has an optional lifecycle API (disabled by default) that can be used to reload, quit, or clear dynamically registered StatsD metric series |
Signed-off-by: YuMin Kim <gimyumin40@gmail.com>
af36ce0 to
7631062
Compare
pedro-stanaka
left a comment
pedro-stanaka
left a comment
There was a problem hiding this comment.
Just a few more comments and it should be good to go, also sign the DCO please in your commits.
| return b.listenDone, b.listening | ||
| } | ||
|
|
||
| func (b *Exporter) clearRegistryMetrics() clearMetricsResult { |
There was a problem hiding this comment.
I wonder if we should instrument this, for large metric sets, we could block for a while and cause the whole exporter to get "stuck" here. Maybe lets add at least a doc block saying that this function may cause "hangs" when working with larger sets.
|
|
||
| func clearMetricsHandler(clearer metricsClearer, logger *slog.Logger) http.HandlerFunc { | ||
| return func(w http.ResponseWriter, r *http.Request) { | ||
| if r.Method == http.MethodPut || r.Method == http.MethodPost { |
There was a problem hiding this comment.
Lets return 405 Method Not Allowed when the method is not POST|PUT.
Allow: PUT, POST, and test both supported methods and at least one unsupported method.
2 participants
Fixes #637.
Summary
This adds a
/-/clearlifecycle endpoint, available only when--web.enable-lifecycleis enabled, alongside the existing/-/reloadand/-/quitendpoints.The endpoint clears dynamically registered StatsD time series without restarting the exporter. Internally, the clear request is handled by the exporter event loop, so it is serialized with normal event ingestion and stale metric cleanup. The registry clear path deletes registered label sets in the same way TTL expiration does, while keeping the existing vector/type metadata available for future samples.
cc @pedro-stanaka from the issue discussion.
Verification
go test ./pkg/registry ./pkg/exporter -run 'TestClearMetrics|TestTtlExpiration|TestHashLabelNames' -count=1go test . -run TestClearMetricsHandler -count=1go test ./...go test -race ./pkg/exporter -run TestClearMetrics -count=1go test -race . -run TestClearMetricsHandler -count=1git diff --check