Skip to content

Record Redis command latency metrics#38

Merged
haasonsaas merged 1 commit intomainfrom
jonathanhaas/redis-command-latency
Apr 14, 2026
Merged

Record Redis command latency metrics#38
haasonsaas merged 1 commit intomainfrom
jonathanhaas/redis-command-latency

Conversation

@haasonsaas
Copy link
Copy Markdown
Collaborator

Summary

  • attach the shared Redis command metrics hook to the ASB runtime-store client during bootstrap
  • keep the instrumentation testable with an injectable registerer while production uses the default registry
  • bump service-runtime to v0.1.18 and add a miniredis-backed metrics assertion

Testing

  • go test ./internal/bootstrap ./cmd/asb-api ./internal/api/httpapi
  • go test ./...
  • git diff --check

@cursor
Copy link
Copy Markdown

cursor bot commented Apr 14, 2026

PR Summary

Low Risk
Low risk: adds Prometheus instrumentation and a small bootstrap-time error path when the metrics hook can’t be registered, without changing Redis command behavior or data handling.

Overview
Adds Redis command observability during bootstrap by attaching the service-runtime Prometheus hook to the runtime-store Redis client (using the default registry in production, but allowing an injected registerer for tests).

Introduces a miniredis-backed unit test that exercises SET/GET and asserts the emitted asb_redis_commands_total and duration histogram metrics, and bumps github.com/evalops/service-runtime to v0.1.18 to pick up the shared hook implementation.

Reviewed by Cursor Bugbot for commit bca977c. Bugbot is set up for automated code reviews on this repo. Configure here.

@haasonsaas haasonsaas merged commit 1ab13c4 into main Apr 14, 2026
4 checks passed
@haasonsaas haasonsaas deleted the jonathanhaas/redis-command-latency branch April 14, 2026 02:08
Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is ON, but it could not run because the branch was deleted or merged before autofix could start.

Reviewed by Cursor Bugbot for commit bca977c. Configure here.

})
if err := instrumentDefaultRedisClient(client); err != nil {
return nil, nil, nil, nil, err
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redis client leaked on instrumentation error path

Low Severity

If instrumentDefaultRedisClient returns an error, newRuntimeStore returns without closing the newly created client. The Close function is only returned to the caller on the success path (line 478), so this error path leaks the Redis client. Adding client.Close() before returning the error would ensure cleanup.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit bca977c. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant