feat(monitor): add Prometheus metrics - #4077
Open
wzxjohn wants to merge 2 commits into
Open
Conversation
Contributor
WalkthroughAdds a Prometheus-based monitoring package and instruments HTTP middleware, GORM DB callbacks and pool, Redis hooks, relay/token flows, and startup metrics server; promotes Prometheus client to a direct dependency and wires metric recording into controller and service logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Gin as "Gin Server\n(HTTP + Middleware)"
participant Handler as "App Handler\n(controller/relay.go)"
participant DB as "GORM / SQL DB"
participant Redis as "Redis Client"
participant Monitor as "monitor package"
participant Metrics as "Prometheus\n/metrics HTTP"
Client->>Gin: HTTP request
Gin->>Monitor: PrometheusMiddleware (start)
Gin->>Handler: invoke handler
Handler->>DB: run queries
DB-->>Monitor: GORM callbacks record DB metrics
Handler->>Redis: execute commands
Redis-->>Monitor: Redis hook records command metrics
Handler->>Monitor: RecordRelayRequest / RecordRelayRetry / RecordRelayTokens
Monitor-->>Metrics: expose metrics via /metrics endpoint
Handler-->>Gin: response
Gin->>Monitor: PrometheusMiddleware (observe)
Gin-->>Client: HTTP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
monitor/metrics.go (2)
11-14: Potential data race onenabledflag.The
enabledvariable is written insideinitOnce.Do()and read byEnabled()without synchronization. Whilesync.Onceguarantees the write happens-before any subsequentDo()call returns, concurrent readers callingEnabled()beforeInitMetrics()completes could observe a stalefalsevalue, which is likely the intended behavior. However, ifEnabled()is called concurrently duringInitMetrics()execution, there's a potential race.Consider using
sync/atomicfor the flag to ensure memory visibility:🔧 Proposed fix using atomic
import ( "sync" + "sync/atomic" "github.com/prometheus/client_golang/prometheus" ) const namespace = "newapi" var ( initOnce sync.Once - enabled bool + enabled atomic.Bool )Then update
Enabled():func Enabled() bool { - return enabled + return enabled.Load() }And in
InitMetrics():- enabled = true + enabled.Store(true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor/metrics.go` around lines 11 - 14, The package has a potential data race on the package-level enabled flag: change the `enabled` variable to an atomic type (e.g., uint32) and use atomic operations so reads/writes are synchronized; update `Enabled()` to return atomic.LoadUint32(&enabled) == 1 and set the flag in `InitMetrics()` with atomic.StoreUint32(&enabled, 1) (or 0 on failure) instead of unsynchronized reads/writes, leaving `initOnce` usage intact to ensure one-time initialization.
158-185: High cardinality metrics may impact Prometheus performance.Tier 3 metrics use
user_idandtoken_idlabels, which can create unbounded cardinality (O(users × tokens × models)). While the design decision to use only counters (no histograms) is appropriate, consider:
- Adding documentation about expected cardinality and Prometheus resource requirements
- Providing a configuration option to disable Tier 3 metrics independently
- Implementing metric expiration/cleanup for inactive users/tokens
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor/metrics.go` around lines 158 - 185, Tier 3 counters (TokenRequestsTotal, TokenTokensUsedTotal, TokenQuotaConsumedTotal) create potentially unbounded label cardinality (user_id, token_id, model) which can hurt Prometheus; add a runtime toggle to enable/disable these Tier 3 metrics (e.g., a config flag or env var read during metrics init), document expected cardinality and resource guidance for running with this flag enabled, and implement a cleanup/expiration strategy for these CounterVecs (e.g., periodically calling DeleteLabelValues or using a short-lived registry) when tokens/users become inactive so labels are pruned; update the metrics initialization code to respect the new config flag and add brief docs mentioning Prometheus requirements and recommended limits.monitor/db.go (2)
78-87: Background goroutine lacks shutdown mechanism.The
StartDBPoolCollectorgoroutine runs indefinitely with no way to stop it. This can cause issues during graceful shutdown or in tests. Consider accepting a context:🔧 Proposed enhancement for graceful shutdown
-func StartDBPoolCollector(sqlDB *sql.DB) { +func StartDBPoolCollector(ctx context.Context, sqlDB *sql.DB) { if sqlDB == nil || !Enabled() { return } go func() { ticker := time.NewTicker(15 * time.Second) defer ticker.Stop() - for range ticker.C { + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + } stats := sqlDB.Stats() DBOpenConnections.WithLabelValues("open").Set(float64(stats.OpenConnections)) DBOpenConnections.WithLabelValues("in_use").Set(float64(stats.InUse)) DBOpenConnections.WithLabelValues("idle").Set(float64(stats.Idle)) } }() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor/db.go` around lines 78 - 87, The background goroutine that polls sqlDB.Stats() (currently started in StartDBPoolCollector) runs forever and needs a shutdown mechanism; modify StartDBPoolCollector to accept a context.Context, replace the for range ticker.C loop with a select that listens for ctx.Done() and the ticker.C case, and on ctx cancellation stop the ticker and return; update references to sqlDB.Stats(), DBOpenConnections and the ticker usage within that function accordingly and ensure callers pass a cancellable or app context so the collector can be cleanly stopped.
36-52: Callback registration errors are silently ignored.The
db.Callback().*.Register()calls return errors that are discarded with_. If registration fails (e.g., duplicate callback name), metrics will silently stop working. Consider logging registration failures:🔧 Proposed fix to log registration errors
+import "log" func registerCallback(db *gorm.DB, operation string) { beforeName := "prometheus:before_" + operation afterName := "prometheus:after_" + operation // ... callback definitions ... switch operation { case "create": - _ = db.Callback().Create().Before("*").Register(beforeName, before) - _ = db.Callback().Create().After("*").Register(afterName, after) + if err := db.Callback().Create().Before("*").Register(beforeName, before); err != nil { + log.Printf("Failed to register DB callback %s: %v", beforeName, err) + } + if err := db.Callback().Create().After("*").Register(afterName, after); err != nil { + log.Printf("Failed to register DB callback %s: %v", afterName, err) + } // ... similar for other cases ... } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor/db.go` around lines 36 - 52, The callback Register calls currently discard returned errors; update each registration (the db.Callback().Create()/Query()/Update()/Delete()/Raw().Before("*").Register(beforeName, before) and .After("*").Register(afterName, after) calls) to capture the error into a variable and log it if non-nil (include operation, beforeName/afterName and the error in the log message) instead of using `_ =`. Apply this change for both the Before and After registrations in all switch cases so failures (e.g., duplicate callback names) are surfaced.monitor/server.go (1)
32-34: Consider adding graceful shutdown support.The server currently has no shutdown mechanism. If the main application shuts down, the metrics server goroutine will be abandoned. Consider accepting a context for graceful shutdown:
🔧 Proposed enhancement for graceful shutdown
+import "context" -func StartMetricsServer() { +func StartMetricsServer(ctx context.Context) { if !Enabled() { return } // ... port setup ... server := &http.Server{ Addr: addr, Handler: mux, } + go func() { + <-ctx.Done() + server.Shutdown(context.Background()) + }() + log.Printf("Prometheus metrics server listening on %s", addr) - if err := http.ListenAndServe(addr, mux); err != nil { + if err := server.ListenAndServe(); err != nil && err != http.ErrServerClosed { log.Printf("Prometheus metrics server error: %v", err) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor/server.go` around lines 32 - 34, Replace the direct call to http.ListenAndServe with an http.Server instance and add graceful shutdown using a context: create server := &http.Server{Addr: addr, Handler: mux}, run server.ListenAndServe in a goroutine, and in the main routine wait for the provided context to be cancelled (ctx.Done()) then call server.Shutdown(shutdownCtx) with a short timeout context to allow in-flight requests to finish; update any function signature that currently starts the server to accept a context.Context so you can invoke server.Shutdown when the app is stopping (referencing http.ListenAndServe, server.ListenAndServe, and server.Shutdown).model/main.go (1)
199-201: Consider instrumenting LOG_DB for consistency.The main DB is instrumented with Prometheus metrics, but
InitLogDB()(lines 218-253) does not register callbacks or pool collectors forLOG_DBwhen it uses a separate database connection. If observability into log database operations is desired, consider adding similar instrumentation there.The current placement after connection pool configuration is appropriate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/main.go` around lines 199 - 201, InitLogDB currently creates a separate log GORM connection but doesn't register Prometheus instrumentation; after you configure the log DB's connection pool in InitLogDB(), call monitor.RegisterDBCallbacks(logDB) and monitor.StartDBPoolCollector(logSqlDB) (where logDB is the GORM *gorm.DB returned/assigned in InitLogDB and logSqlDB is the underlying *sql.DB obtained via logDB.DB()) so the LOG_DB is instrumented the same way as the main DB.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controller/relay.go`:
- Line 235: The Tier 3 metric calls are creating unbounded cardinality via
RecordTokenRequest(relayInfo.UserId, relayInfo.TokenId) (and related metrics
TokenRequestsTotal, TokenTokensUsedTotal, TokenQuotaConsumedTotal) — add a
runtime config flag (e.g., EnableTier3Metrics or DisableHighCardinalityMetrics)
in the monitor or config package and check it before invoking
monitor.RecordTokenRequest and the other Tier 3 metric emitters so these metrics
can be disabled per deployment; alternatively implement low-cardinality
bucketing/sampling inside RecordTokenRequest (e.g., hash the userId/tokenId and
map to fixed N buckets or sample 1/N requests) and update the metric
names/labels accordingly to avoid user_id/token_id labels being emitted for
every unique id.
In `@monitor/metrics.go`:
- Around line 87-100: DBWaitTotal and DBWaitDuration are never updated; modify
StartDBPoolCollector to read stats := sqlDB.Stats() each tick and compute deltas
against stored previous values (e.g., lastWaitCount int64 and lastWaitDuration
time.Duration) and call DBWaitTotal.Add(float64(deltaWaitCount)) and
DBWaitDuration.Add(deltaWaitDuration.Seconds()) only when the current values
exceed the previous ones, then update the stored previous variables so counters
reflect incremental WaitCount and WaitDuration from sql.DBStats rather than
repeatedly adding the cumulative totals.
In `@monitor/middleware.go`:
- Around line 18-23: HTTPActiveConnections may not be decremented if a
downstream handler panics because Dec() is called after c.Next(); fix by making
the decrement run in a defer immediately after increment so it always executes:
add defer HTTPActiveConnections.Dec() right after HTTPActiveConnections.Inc(),
and if you compute request duration using the start variable, move that
duration/reporting logic into a deferred closure that captures start (or perform
duration calculation inside the same defer) so c.Next() can still run and any
panics won't leave the gauge inflated; reference HTTPActiveConnections,
c.Next(), and the start variable.
---
Nitpick comments:
In `@model/main.go`:
- Around line 199-201: InitLogDB currently creates a separate log GORM
connection but doesn't register Prometheus instrumentation; after you configure
the log DB's connection pool in InitLogDB(), call
monitor.RegisterDBCallbacks(logDB) and monitor.StartDBPoolCollector(logSqlDB)
(where logDB is the GORM *gorm.DB returned/assigned in InitLogDB and logSqlDB is
the underlying *sql.DB obtained via logDB.DB()) so the LOG_DB is instrumented
the same way as the main DB.
In `@monitor/db.go`:
- Around line 78-87: The background goroutine that polls sqlDB.Stats()
(currently started in StartDBPoolCollector) runs forever and needs a shutdown
mechanism; modify StartDBPoolCollector to accept a context.Context, replace the
for range ticker.C loop with a select that listens for ctx.Done() and the
ticker.C case, and on ctx cancellation stop the ticker and return; update
references to sqlDB.Stats(), DBOpenConnections and the ticker usage within that
function accordingly and ensure callers pass a cancellable or app context so the
collector can be cleanly stopped.
- Around line 36-52: The callback Register calls currently discard returned
errors; update each registration (the
db.Callback().Create()/Query()/Update()/Delete()/Raw().Before("*").Register(beforeName,
before) and .After("*").Register(afterName, after) calls) to capture the error
into a variable and log it if non-nil (include operation, beforeName/afterName
and the error in the log message) instead of using `_ =`. Apply this change for
both the Before and After registrations in all switch cases so failures (e.g.,
duplicate callback names) are surfaced.
In `@monitor/metrics.go`:
- Around line 11-14: The package has a potential data race on the package-level
enabled flag: change the `enabled` variable to an atomic type (e.g., uint32) and
use atomic operations so reads/writes are synchronized; update `Enabled()` to
return atomic.LoadUint32(&enabled) == 1 and set the flag in `InitMetrics()` with
atomic.StoreUint32(&enabled, 1) (or 0 on failure) instead of unsynchronized
reads/writes, leaving `initOnce` usage intact to ensure one-time initialization.
- Around line 158-185: Tier 3 counters (TokenRequestsTotal,
TokenTokensUsedTotal, TokenQuotaConsumedTotal) create potentially unbounded
label cardinality (user_id, token_id, model) which can hurt Prometheus; add a
runtime toggle to enable/disable these Tier 3 metrics (e.g., a config flag or
env var read during metrics init), document expected cardinality and resource
guidance for running with this flag enabled, and implement a cleanup/expiration
strategy for these CounterVecs (e.g., periodically calling DeleteLabelValues or
using a short-lived registry) when tokens/users become inactive so labels are
pruned; update the metrics initialization code to respect the new config flag
and add brief docs mentioning Prometheus requirements and recommended limits.
In `@monitor/server.go`:
- Around line 32-34: Replace the direct call to http.ListenAndServe with an
http.Server instance and add graceful shutdown using a context: create server :=
&http.Server{Addr: addr, Handler: mux}, run server.ListenAndServe in a
goroutine, and in the main routine wait for the provided context to be cancelled
(ctx.Done()) then call server.Shutdown(shutdownCtx) with a short timeout context
to allow in-flight requests to finish; update any function signature that
currently starts the server to accept a context.Context so you can invoke
server.Shutdown when the app is stopping (referencing http.ListenAndServe,
server.ListenAndServe, and server.Shutdown).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 14efaaab-95e5-4c48-8953-7aee20d115ed
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
controller/relay.gogo.modmain.gomodel/main.gomonitor/db.gomonitor/metrics.gomonitor/middleware.gomonitor/redis_hook.gomonitor/relay.gomonitor/server.gomonitor/token.goservice/text_quota.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
monitor/db.go (1)
73-99: MakeStartDBPoolCollectoridempotent per DB handle.Calling this multiple times starts duplicate goroutines and can overcount
DBWaitTotal/DBWaitDuration(each goroutine tracks deltas independently).♻️ Suggested direction
+// guard collector startup per *sql.DB to avoid duplicate goroutines/counter inflation +// (implementation can use sync.Map keyed by DB pointer)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor/db.go` around lines 73 - 99, StartDBPoolCollector currently spawns a new goroutine each call for the same sqlDB which causes duplicate metrics; make it idempotent per DB handle by tracking which *sql.DB instances have already had collectors started (e.g. a package-level startedCollectors map[*sql.DB]struct{} protected by a mutex or a sync.Map) and return early if sqlDB is already present; keep the existing metric updates (DBOpenConnections, DBWaitTotal, DBWaitDuration) and ticker logic unchanged, but only start the goroutine when the sqlDB key is newly added.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@monitor/db.go`:
- Around line 38-51: The GORM callback Register(...) calls (e.g.,
db.Callback().Create().Before("*").Register(beforeName, before),
db.Callback().Query().After(...), db.Callback().Raw().Before(...), etc.)
currently ignore returned errors via `_ =`, which masks failures like duplicate
callback names; replace each `_ =` with proper error handling: capture the error
(if err := db.Callback()...Register(...); err != nil { /* either return err from
the enclosing function or log it via the package logger (e.g.,
monitorLogger.Errorf) and continue */ }), ensuring you surface failures when
registering beforeName/afterName for Create, Query, Update, Delete, and Raw
callbacks instead of discarding them.
---
Nitpick comments:
In `@monitor/db.go`:
- Around line 73-99: StartDBPoolCollector currently spawns a new goroutine each
call for the same sqlDB which causes duplicate metrics; make it idempotent per
DB handle by tracking which *sql.DB instances have already had collectors
started (e.g. a package-level startedCollectors map[*sql.DB]struct{} protected
by a mutex or a sync.Map) and return early if sqlDB is already present; keep the
existing metric updates (DBOpenConnections, DBWaitTotal, DBWaitDuration) and
ticker logic unchanged, but only start the goroutine when the sqlDB key is newly
added.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 99eec33e-14ae-439c-83b1-f474ce320d09
📒 Files selected for processing (4)
monitor/db.gomonitor/metrics.gomonitor/middleware.gomonitor/token.go
🚧 Files skipped from review as they are similar to previous changes (2)
- monitor/middleware.go
- monitor/token.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
monitor/db.go (1)
78-99: Pool collector goroutine has no shutdown signal.The ticker goroutine runs for the process lifetime with no stop channel or context. In practice this is fine since
StartDBPoolCollectoris called once fromInitDBand the collector is meant to live as long as the process. Flagging only as an optional future-proofing consideration — ifInitDBever becomes re-entrant (e.g., for tests or DB reconnection), this will leak goroutines and double-write gauges. Not a blocker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor/db.go` around lines 78 - 99, The pool-collector goroutine started in StartDBPoolCollector runs for process lifetime with no shutdown signal; change StartDBPoolCollector to accept a context (or a stop channel), and inside the goroutine replace the for range ticker.C loop with a select that listens on ticker.C and ctx.Done(); on ctx.Done() stop the ticker and return to prevent goroutine leakage and double-writing metrics (keep the existing sqlDB.Stats() and metric updates when ticker.C fires). Update callers (e.g., InitDB) to pass a cancellable context where appropriate so tests or reconnections can cancel the collector.monitor/redis_hook.go (2)
42-58: Minor: pipeline per-command duration is not observed.In
AfterProcessPipeline, only an aggregate"PIPELINE"duration is recorded while per-command counters are still incremented. That's a reasonable design choice (you can't separate individual latencies inside a pipeline), but it meansRedisCommandDurationwill under-count samples relative toRedisCommandsTotalfor commands that ever run inside pipelines — worth noting in a dashboard/alert comment or the metric's Help text so consumers don't compute misleading averages by dividing the two.Also a small nit:
cmd.Err()is evaluated twice per command; caching it in a local would be slightly cleaner:♻️ Optional cleanup
for _, cmd := range cmds { command := strings.ToUpper(cmd.Name()) status := "success" - if cmd.Err() != nil && cmd.Err() != redis.Nil { + if err := cmd.Err(); err != nil && err != redis.Nil { status = "error" } RedisCommandsTotal.WithLabelValues(command, status).Inc() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor/redis_hook.go` around lines 42 - 58, AfterProcessPipeline currently records per-command counts (RedisCommandsTotal in redisMetricsHook.AfterProcessPipeline) but only records a single aggregate duration label ("PIPELINE") in RedisCommandDuration, which can mislead consumers who compute per-command averages; update the metric Help text or add a comment near redisMetricsHook.AfterProcessPipeline/RedisCommandDuration explaining that per-command durations are not available for pipeline commands and that counts may outnumber duration samples, and while here also micro-optimize by caching cmd.Err() into a local variable inside the loop (use the cached err when deciding status) to avoid evaluating cmd.Err() twice.
17-19: Optional: short-circuitBeforeProcess/BeforeProcessPipelinewhen metrics are disabled.When
Enabled()is false, theAfter*handlers return immediately, butBefore*still allocates a new context and callstime.Now()on every Redis command. Sinceenabledis set once viasync.Onceand never changes, you can skip the allocation entirely:♻️ Proposed refactor
func (h *redisMetricsHook) BeforeProcess(ctx context.Context, cmd redis.Cmder) (context.Context, error) { + if !Enabled() { + return ctx, nil + } return context.WithValue(ctx, redisMetricsHookKey{}, time.Now()), nil } @@ func (h *redisMetricsHook) BeforeProcessPipeline(ctx context.Context, cmds []redis.Cmder) (context.Context, error) { + if !Enabled() { + return ctx, nil + } return context.WithValue(ctx, redisMetricsHookKey{}, time.Now()), nil }Note: since
RegisterRedisHookalready gates onEnabled()at startup and the flag is immutable afterwards, this is purely a minor micro-optimization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor/redis_hook.go` around lines 17 - 19, The BeforeProcess and BeforeProcessPipeline methods (redisMetricsHook.BeforeProcess / redisMetricsHook.BeforeProcessPipeline) should short-circuit when metrics are disabled to avoid allocating a new context and calling time.Now() on every command; detect the enabled flag (the existing Enabled() check or the hook's configured state) at the start of each Before* method and immediately return (ctx, nil) if disabled, leaving After* unchanged. This uses the immutability of the enabled flag (set once via sync.Once) to safely skip the context.WithValue/time.Now allocation when metrics are off.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controller/relay.go`:
- Around line 250-263: The failure-path metrics block records
relayInfo.ChannelId/ChannelType even when no channel was selected (e.g.,
getChannel failed in the retry loop), producing misleading series; update the
failure-path before calling monitor.RecordRelayRequest /
monitor.RecordTokenRequest to check whether a channel was actually selected
(inspect relayInfo.ChannelId and relayInfo.ChannelType) and either skip emitting
metrics when they are unset or replace them with a clear sentinel (e.g., -1 or
"none"); ensure this guard is applied in the block that handles newAPIError (the
code that calls monitor.RecordRelayRequest and monitor.RecordTokenRequest) so
only real channel selections are recorded.
---
Nitpick comments:
In `@monitor/db.go`:
- Around line 78-99: The pool-collector goroutine started in
StartDBPoolCollector runs for process lifetime with no shutdown signal; change
StartDBPoolCollector to accept a context (or a stop channel), and inside the
goroutine replace the for range ticker.C loop with a select that listens on
ticker.C and ctx.Done(); on ctx.Done() stop the ticker and return to prevent
goroutine leakage and double-writing metrics (keep the existing sqlDB.Stats()
and metric updates when ticker.C fires). Update callers (e.g., InitDB) to pass a
cancellable context where appropriate so tests or reconnections can cancel the
collector.
In `@monitor/redis_hook.go`:
- Around line 42-58: AfterProcessPipeline currently records per-command counts
(RedisCommandsTotal in redisMetricsHook.AfterProcessPipeline) but only records a
single aggregate duration label ("PIPELINE") in RedisCommandDuration, which can
mislead consumers who compute per-command averages; update the metric Help text
or add a comment near redisMetricsHook.AfterProcessPipeline/RedisCommandDuration
explaining that per-command durations are not available for pipeline commands
and that counts may outnumber duration samples, and while here also
micro-optimize by caching cmd.Err() into a local variable inside the loop (use
the cached err when deciding status) to avoid evaluating cmd.Err() twice.
- Around line 17-19: The BeforeProcess and BeforeProcessPipeline methods
(redisMetricsHook.BeforeProcess / redisMetricsHook.BeforeProcessPipeline) should
short-circuit when metrics are disabled to avoid allocating a new context and
calling time.Now() on every command; detect the enabled flag (the existing
Enabled() check or the hook's configured state) at the start of each Before*
method and immediately return (ctx, nil) if disabled, leaving After* unchanged.
This uses the immutability of the enabled flag (set once via sync.Once) to
safely skip the context.WithValue/time.Now allocation when metrics are off.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10437a1e-fe32-42a0-b89a-9f93f681e7cb
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
controller/relay.gogo.modmain.gomodel/main.gomonitor/db.gomonitor/metrics.gomonitor/middleware.gomonitor/redis_hook.gomonitor/relay.gomonitor/server.gomonitor/token.goservice/text_quota.go
✅ Files skipped from review due to trivial changes (3)
- go.mod
- model/main.go
- monitor/token.go
🚧 Files skipped from review as they are similar to previous changes (5)
- service/text_quota.go
- monitor/server.go
- main.go
- monitor/middleware.go
- monitor/metrics.go
Comment on lines
+250
to
263
| // Record metrics for failed relay | ||
| if newAPIError != nil { | ||
| monitor.RecordRelayRequest(&monitor.RelayMetricsData{ | ||
| ChannelId: relayInfo.ChannelId, | ||
| ChannelType: relayInfo.ChannelType, | ||
| Model: relayInfo.OriginModelName, | ||
| RelayMode: relayInfo.RelayMode, | ||
| StatusCode: newAPIError.StatusCode, | ||
| StartTime: relayInfo.StartTime, | ||
| IsStream: relayInfo.IsStream, | ||
| ErrorType: string(newAPIError.GetErrorCode()), | ||
| }) | ||
| monitor.RecordTokenRequest(relayInfo.UserId, relayInfo.TokenId) | ||
| } |
Contributor
There was a problem hiding this comment.
Failure-path metrics can record channel_id=0 / channel_type=0.
If the retry loop breaks at line 196 because getChannel returned an error on the first iteration, relayInfo.ChannelId/ChannelType are never populated and you'll emit a failure series labeled with channel_id="0", channel_type="0". Over time this conflates "no channel available" with legitimate channel 0 and contributes an extra series for every error code seen before channel selection.
Consider skipping the emission (or using a sentinel like -1/"none") when no channel was actually selected, e.g.:
Proposed guard
if newAPIError != nil {
+ // Only record channel-scoped metrics if a channel was actually selected.
+ if relayInfo.ChannelId != 0 {
monitor.RecordRelayRequest(&monitor.RelayMetricsData{
ChannelId: relayInfo.ChannelId,
...
ErrorType: string(newAPIError.GetErrorCode()),
})
+ }
monitor.RecordTokenRequest(relayInfo.UserId, relayInfo.TokenId)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/relay.go` around lines 250 - 263, The failure-path metrics block
records relayInfo.ChannelId/ChannelType even when no channel was selected (e.g.,
getChannel failed in the retry loop), producing misleading series; update the
failure-path before calling monitor.RecordRelayRequest /
monitor.RecordTokenRequest to check whether a channel was actually selected
(inspect relayInfo.ChannelId and relayInfo.ChannelType) and either skip emitting
metrics when they are unset or replace them with a clear sentinel (e.g., -1 or
"none"); ensure this guard is applied in the block that handles newAPIError (the
code that calls monitor.RecordRelayRequest and monitor.RecordTokenRequest) so
only real channel selections are recorded.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📝 变更描述 / Description
通过三级分层添加 Prometheus 指标，以控制 label 的基数。指标通过专用的内部端口（/metrics）提供，与主 API 服务器相互独立。
Adds comprehensive Prometheus metrics to the API gateway with a three-tiered approach to control label cardinality. Metrics are served on a dedicated internal port (
/metrics), separate from the main API server.🔗 关联任务 / Related Issue
Metric Tiers
Tier 1 — System-Level (Low Cardinality)
newapi_http_requests_totalmethod,path,status_codenewapi_http_request_duration_secondsmethod,path,status_codenewapi_http_active_connectionsnewapi_redis_commands_totalcommand,statusnewapi_redis_command_duration_secondscommandnewapi_db_queries_totaloperation,statusnewapi_db_query_duration_secondsoperationnewapi_db_open_connectionsstatenewapi_db_wait_totalnewapi_db_wait_duration_seconds_totalTier 2 — Relay/Channel-Level (Moderate Cardinality)
newapi_relay_requests_totalchannel_id,channel_type,model,status_code,relay_modenewapi_relay_request_duration_secondschannel_id,channel_type,model,relay_modenewapi_relay_first_token_secondschannel_id,channel_type,modelnewapi_relay_tokens_used_totalchannel_id,channel_type,model,directionnewapi_relay_errors_totalchannel_id,channel_type,error_typenewapi_relay_retries_totalchannel_id,channel_typeTier 3 — User/Token-Level (High Cardinality, Counters Only)
newapi_token_requests_totaluser_id,token_idnewapi_token_tokens_used_totaluser_id,token_id,model,directionnewapi_token_quota_consumed_totaluser_id,token_idArchitecture
New
monitor/package with 7 files:metrics.goInitMetrics(),Enabled()server.go/metricsendpointmiddleware.goredis_hook.goHookfor command-level Redis metricsdb.gorelay.gotoken.goIntegration Points
main.go: Initialize metrics early in startup, register Gin middleware, attach Redis hook after client initmodel/main.go: Register GORM callbacks and start DB pool stats collector after DB initcontroller/relay.go: Record relay request/retry/error metrics on success and failure pathsservice/text_quota.go: Record token usage and quota consumption after billing calculationConfiguration
PROMETHEUS_ENABLEDtrue"false"to fully disable (zero overhead)PROMETHEUS_PORT9090Design Decisions
c.FullPath()(Gin route patterns) to avoid cardinality explosion from path parametersmonitor/uses stdliblogfor its own logging instead of importingcommonEnabled()guard on every call pathSummary by CodeRabbit