Enable beholder metrics for sqlutil - #2353
Conversation
|
👋 dhaidashenko, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
📊 API Diff Results
|
There was a problem hiding this comment.
🟡 Changes recommended
The new Beholder DB wait-time metric records a cumulative total into a histogram with mismatched naming/units vs the existing Prometheus gauge, which will produce misleading telemetry.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds OpenTelemetry/Beholder metric emission for existing sqlutil and Postgres DB stats instrumentation, alongside the current Prometheus metrics, to support Beholder-based observability.
Changes:
- Extend
pg.StatsReporterto record DB connection pool stats to Beholder on each reporting interval. - Add Beholder metric plumbing for SQL query timeout-percentage histogram, mirroring the existing Prometheus histogram.
- Refactor shared bucket definitions for SQL query timeout-percentage metrics.
File summaries
| File | Description |
|---|---|
| pkg/sqlutil/pg/stats.go | Routes each stats tick through a new report(ctx) that also records Beholder DB stats. |
| pkg/sqlutil/pg/beholder_metrics.go | Introduces Beholder instruments and recording for sql.DBStats fields. |
| pkg/sqlutil/monitor.go | Reuses a shared bucket list and records Beholder histogram alongside Prometheus. |
| pkg/sqlutil/beholder_metrics.go | Adds lazy-initialized Beholder histogram for sql_query_timeout_percent. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
7e9f6a6 to
86bf7f6
Compare
| "go.opentelemetry.io/otel/metric" | ||
| "go.opentelemetry.io/otel/metric/noop" | ||
|
|
||
| "github.com/smartcontractkit/chainlink-common/pkg/beholder" |
There was a problem hiding this comment.
I don't think this package should import beholder. We already provide a means of setting a custom hook via StatsCustomReporterFn. Why don't we use that?
| sqlQueryTimeMetricOnce sync.Once | ||
| globalSQLQueryTimeMetric sqlQueryTimeMetric |
There was a problem hiding this comment.
Do we strictly need a global instance? or could we inject something to use or construct from instead? We don't typically need to suppress errors for metric creation.
No description provided.