Use prometheus client and labels #48
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the metrics package to use the official Prometheus Go client library instead of a custom implementation. The main changes include using Prometheus CounterVec and GaugeVec types with labels for tool names, session types, and other metadata instead of generating custom metric names.
Key changes:
- Replaces custom metric string concatenation with Prometheus labels for tool calls and errors
- Migrates from internal counter storage to Prometheus registry and collectors
- Updates tests to verify metrics through HTTP handler output instead of internal counter values
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| mcp/common.go | Updates tool tracking to use labels instead of concatenated metric names |
| kc/manager.go | Adds new method to support labeled metrics |
| go.mod | Adds Prometheus client library dependency |
| app/metrics/metrics.go | Complete refactor to use Prometheus client with registry and vectorized metrics |
| app/metrics/metrics_test.go | Updates tests to verify metrics through HTTP output instead of internal counters |
| app/metrics/daily_metrics_test.go | Simplifies tests and removes internal metric parsing logic |
Comments suppressed due to low confidence (1)
go.mod:3
- Go version 1.24.2 does not exist. The latest stable Go version as of my knowledge cutoff is 1.21.x. Please verify and use a valid Go version.
go 1.24.2
|
|
||
| if counter, ok := counterInterface.(prometheus.Counter); ok { | ||
| // Try to register the counter (ignore already registered errors) | ||
| m.registry.Register(counter) //nolint:all |
There was a problem hiding this comment.
Silently ignoring all errors with //nolint:all can hide important registration failures. Consider handling specific errors like prometheus.AlreadyRegisteredError explicitly.
| m.registry.Register(counter) //nolint:all | |
| if err := m.registry.Register(counter); err != nil { | |
| if _, already := err.(prometheus.AlreadyRegisteredError); !already { | |
| log.Printf("error registering prometheus counter %q: %v", key, err) | |
| } | |
| } |
There was a problem hiding this comment.
Should I make this change? Should be debug log level by default
|
|
||
| if counter, ok := counterInterface.(prometheus.Counter); ok { | ||
| // Try to register the counter (ignore already registered errors) | ||
| m.registry.Register(counter) //nolint:all |
There was a problem hiding this comment.
Silently ignoring all errors with //nolint:all can hide important registration failures. Consider handling specific errors like prometheus.AlreadyRegisteredError explicitly.
| m.registry.Register(counter) //nolint:all | |
| if err := m.registry.Register(counter); err != nil { | |
| // Ignore AlreadyRegisteredError, log others | |
| if _, ok := err.(prometheus.AlreadyRegisteredError); !ok { | |
| fmt.Printf("Failed to register Prometheus counter for key %s: %v\n", key, err) | |
| } | |
| } |
| // Get or create Prometheus counter for this key | ||
| counterInterface, _ := m.genericCounters.LoadOrStore(key, prometheus.NewCounter( | ||
| prometheus.CounterOpts{ | ||
| Name: strings.ReplaceAll(key, "-", "_"), |
There was a problem hiding this comment.
The metric name transformation only handles hyphens. Consider using a more comprehensive sanitization function to handle other invalid Prometheus metric name characters.
| Name: strings.ReplaceAll(key, "-", "_"), | |
| Name: sanitizeMetricName(key), |
Refactors the metrics package to use the official prometheus golang client. Tool names are now added as labels instead of custom metrics to simplify aggregate queries. PR #48
Refactors the metrics package to use the official prometheus golang client. Tool names are now added as labels instead of custom metrics to simplify aggregate queries. PR #48 Co-authored-by: C V Hariharan <[email protected]>
1 participant
This PR refactors the metrics package to use the official prometheus golang client. The tool names are now added as labels instead of custom metrics to simplify aggregate queries.