feat: export BaseMetrics for forward-compatible custom implementations#2
feat: export BaseMetrics for forward-compatible custom implementations#2
Conversation
Replace unexported noopMetrics with exported BaseMetrics. Custom
Metrics implementations embed BaseMetrics so that new methods added
to the interface get safe no-op defaults. Remove redundant NoopMetrics
var — use &BaseMetrics{} directly.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 46 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR introduces an exported BaseMetrics no-op implementation intended to be embedded by custom Metrics implementations, and updates defaults to use BaseMetrics instead of NoopMetrics.
Changes:
- Add exported
BaseMetricswith no-op implementations for allMetricsmethods. - Switch default metrics behavior in
Run/worker context resolution fromNoopMetricsto&BaseMetrics{}. - Update README/docs to document
BaseMetricsand removeNoopMetricsreferences.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| worker.go | Default nil metrics to &BaseMetrics{} in worker context creation. |
| run.go | Default metrics in config and resolution now use &BaseMetrics{}; docs updated accordingly. |
| metrics.go | Replaces NoopMetrics/noopMetrics with exported BaseMetrics type and no-op methods. |
| README.md | Adds BaseMetrics docs and updates references from NoopMetrics to BaseMetrics. |
Comments suppressed due to low confidence (1)
run.go:27
- WithMetrics currently accepts nil and will set c.metrics to nil, which can lead to nil-interface panics in code paths that call metrics methods (e.g., the suture EventHook). Consider treating a nil argument as “disable metrics” by defaulting to BaseMetrics (or rejecting nil).
// WithMetrics sets the metrics implementation for all workers started by Run.
// Workers inherit this unless they override via Worker.WithMetrics.
// If not set, &BaseMetrics{} is used.
func WithMetrics(m Metrics) RunOption {
return func(c *runConfig) {
c.metrics = m
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
BaseMetricsstruct with no-op implementations of allMetricsmethodsBaseMetricsso new methods added to the interface get safe no-op defaults instead of breaking buildsNoopMetricsvar — use&BaseMetrics{}directlyWithMetricsis passed is&BaseMetrics{}(zero overhead)Test plan
go test -race ./...passesmake lint— 0 issues