Skip to content

feat: [#726] database auto instrumentation [8] - #1477

Merged
krishankumar01 merged 27 commits into
masterfrom
kkumar-gcc/#726-database-instrumentation
Jun 22, 2026
Merged

feat: [#726] database auto instrumentation [8]#1477
krishankumar01 merged 27 commits into
masterfrom
kkumar-gcc/#726-database-instrumentation

Conversation

@krishankumar01

@krishankumar01 krishankumar01 commented Jun 7, 2026

Copy link
Copy Markdown
Member

📑 Description

RelatedTo goravel/goravel#726

The #726 series instruments HTTP, gRPC, and logs, but database queries stay invisible: no spans, no query metrics, no connection pool visibility. Telemetry is unreleased, so this lands before the v1.18 surface freezes.

What is changing:

  • Both database facades are instrumented. facades.Orm() through a gorm callback plugin (create/query/update/delete/row/raw), and facades.DB() (the sqlx query builder, raw and fluent) through a CommonBuilder decorator. The builder runs sqlx on the raw *sql.DB and never reaches gorm callbacks, so the two paths stay disjoint: one operation produces one span.
  • One shared instrument core per connection, gated by telemetry.instrumentation.database.enabled (default true). When disabled, the builder is returned unwrapped, so there is no overhead. The instrument is cached with the connection, so every facades.DB() / facades.Orm() call on the same connection (including Connection("...") switches) reuses it and both facades stay instrumented.

What it provides:

  • Traces: one client span per operation, named like SELECT users, nested under the request span, with semconv v1.37 attributes. Structured queries on both facades carry db.collection.name; raw SQL stays operation-only, matching Orm().Raw(). db.query.text keeps placeholders only. ErrRecordNotFound and sql.ErrNoRows are not treated as errors.
  • Metrics: the db.client.operation.duration histogram ({operation, table}), plus connection pool gauges db.client.connection.count (by state idle/used) and db.client.connection.max.

How to use:

Nothing to wire up. With telemetry enabled, both facades emit automatically:

facades.Orm().Query().Where("name = ?", "Goravel").Find(&users)
facades.DB().Table("users").Where("name", "Goravel").Get(&users)

Each produces a SELECT users client span under the request span and a db.client.operation.duration{operation=SELECT, table=users} measurement. Set telemetry.instrumentation.database.enabled to false to turn it off.

Caveats:

  • Bound parameter values are never recorded. db.query.text carries the parameterized SQL with placeholders only, so values (including PII) do not leak into traces.
  • Query metrics deliberately omit db.query.text (high cardinality); the query text lives on spans only.
  • Connection pool metrics expose count and max only. Richer pool signals (wait time, timeouts) are not exported because sql.DBStats gives cumulative snapshots, not the per-event observations the OTel pool conventions expect.
  • Pool metrics cover the writer pool only. In read/write (dbresolver) setups the replica pools are internal gorm.ConnPool instances with no public sql.DBStats, so they are not measured; spans still carry db.client.connection.pool.state to distinguish source from replica.
  • This PR covers the database layer only. Request/HTTP, gRPC, and log signals come from the existing feat: [#280] Implement Sqlserver driver #726 instrumentation; DB spans nest under those request spans.
image image

✅ Checks

  • Added test cases for my code

@krishankumar01
krishankumar01 requested a review from a team as a code owner June 7, 2026 10:39
Copilot AI review requested due to automatic review settings June 7, 2026 10:39
@krishankumar01 krishankumar01 changed the title Kkumar gcc/#726 database instrumentation feat: [#726] database auto instrumentation Jun 7, 2026
@krishankumar01
krishankumar01 marked this pull request as draft June 7, 2026 10:40
@krishankumar01 krishankumar01 changed the title feat: [#726] database auto instrumentation feat: [#726] database auto instrumentation [8] Jun 7, 2026
@codecov

codecov Bot commented Jun 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 57.77126% with 144 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.63%. Comparing base (af5e263) to head (fe50224).
⚠️ Report is 11 commits behind head on master.

Files with missing lines Patch % Lines
telemetry/instrumentation/database/gorm.go 34.69% 16 Missing and 16 partials ⚠️
database/db/db.go 13.79% 23 Missing and 2 partials ⚠️
database/orm/orm.go 0.00% 19 Missing ⚠️
database/driver/gorm.go 30.76% 15 Missing and 3 partials ⚠️
telemetry/instrumentation/database/builder.go 73.58% 11 Missing and 3 partials ⚠️
telemetry/instrumentation/database/database.go 87.61% 12 Missing and 2 partials ⚠️
database/gorm/query.go 58.82% 7 Missing ⚠️
database/service_provider.go 0.00% 7 Missing ⚠️
telemetry/instrumentation/database/pool.go 83.33% 2 Missing and 2 partials ⚠️
telemetry/setup/stubs.go 0.00% 3 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1477      +/-   ##
==========================================
+ Coverage   69.46%   69.63%   +0.16%     
==========================================
  Files         378      384       +6     
  Lines       29710    30183     +473     
==========================================
+ Hits        20639    21017     +378     
- Misses       8118     8174      +56     
- Partials      953      992      +39     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds OpenTelemetry database instrumentation to Goravel, covering GORM operations, query-builder calls, and connection-pool metrics, with a config toggle to enable/disable the feature.

Changes:

  • Introduces a new telemetry/instrumentation/database package (spans, operation-duration histogram, pool stats metrics, and wrappers for DB builders).
  • Wires database instrumentation into GORM connection setup and DB query builder execution paths behind telemetry.instrumentation.database.enabled.
  • Adjusts Tx.Select to execute parameterized SQL (placeholders) while still logging the explained SQL, and adds tests around the behavior.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
telemetry/setup/stubs.go Adds default config stub for database instrumentation enablement.
telemetry/instrumentation/database/database.go Core DB span + duration metric helpers and semantic attributes.
telemetry/instrumentation/database/database_test.go Unit tests for DB system mapping, operation parsing, and recordable-error filtering.
telemetry/instrumentation/database/pool.go Registers observable metrics from sql.DBStats for connection pool telemetry.
telemetry/instrumentation/database/pool_test.go Validates pool metric registration and emitted metric names/attributes.
telemetry/instrumentation/database/gorm.go GORM plugin to create spans for ORM operations and optionally register pool metrics once.
telemetry/instrumentation/database/gorm_test.go Ensures GORM spans are created, named, and do not include interpolated bound values.
telemetry/instrumentation/database/builder.go Wraps query-builder interfaces to create spans/metrics for sqlx operations.
telemetry/instrumentation/database/builder_test.go Tests traced builder wrappers for spans and error status behavior.
telemetry/instrumentation/database/helpers_test.go Shared test telemetry setup (recording exporter + facade mocking).
database/driver/gorm.go Enables GORM plugin registration (with pool metrics) when database instrumentation is enabled.
database/db/db.go Enables builder wrapping + fixes Tx.Select to execute parameterized SQL while logging explained SQL.
database/db/db_test.go Tests that Tx.Select passes placeholder SQL through to builder methods.

Comment thread telemetry/instrumentation/database/pool.go Outdated
Comment thread telemetry/instrumentation/database/database.go Outdated
@krishankumar01
krishankumar01 marked this pull request as ready for review June 13, 2026 11:48
Copilot AI review requested due to automatic review settings June 13, 2026 11:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Comment thread database/driver/gorm.go Outdated
Comment thread telemetry/instrumentation/database/pool.go Outdated
Comment thread telemetry/instrumentation/database/pool_test.go Outdated
Copilot AI review requested due to automatic review settings June 13, 2026 14:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Comment thread telemetry/instrumentation/database/database.go

@hwbrzzl hwbrzzl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great PR, could you add some screenshots for this feature to confirm if it works?

Comment thread telemetry/instrumentation/database/builder_test.go Outdated
Comment thread database/db/db.go
Comment thread telemetry/instrumentation/database/database_test.go Outdated
Comment thread database/db/query.go Outdated
Copilot AI review requested due to automatic review settings June 20, 2026 07:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

Comment thread telemetry/instrumentation/database/database.go Outdated
Comment thread database/db/query.go Outdated
@hwbrzzl

hwbrzzl commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Please let me know if it's ready, wish to release it in v1.18 if we can.

The gorm plugin and query-builder decorator captured the telemetry facade
when the connection was built. If the database connection is established
before the telemetry provider boots, the facade was still nil, so no plugin
was registered and the cached connection stayed uninstrumented.

Resolve telemetry lazily through a resolver on first query instead: always
register the plugin and wrap the builder, and no-op until telemetry is
available and enabled.
@krishankumar01
krishankumar01 force-pushed the kkumar-gcc/#726-database-instrumentation branch from 4337f0e to e0e0131 Compare June 20, 2026 09:48
Replace the hand-rolled ready/disabled atomics and double-checked locking
in Instrument.active with a sync.Once, and read the telemetry facade
directly instead of through a single-implementation resolver. Drop the now
dead nil guards in WrapBuilder/WrapTxBuilder and the redundant active checks
in startSpan/endSpan.
Copilot AI review requested due to automatic review settings June 20, 2026 10:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.

Comment thread telemetry/instrumentation/database/database.go Outdated
Comment thread database/db/query.go Outdated
Comment thread database/db/query.go Outdated
Comment thread telemetry/instrumentation/database/builder.go
Switch database instrumentation from direct telemetry injection to the
lazy resolver pattern used by the log handler. The Instrument is created
once per DB connection and shared across all Tx instances, eliminating
per-transaction allocations.

What changed:
- NewInstrument takes a Resolver closure instead of a concrete Telemetry
- active() resolves lazily with nil-safe guards and mutex for thread safety
- Shared Instrument: DB creates one, passes it to every Tx/BeginTransaction
- operationName uses TrimLeft+IndexByte instead of strings.Fields (zero alloc)
- Pool metrics now include wait_time and timeouts from sql.DBStats
- ContextWithTable moved from NewQuery to Tx.Table to avoid breaking clone ctx
- builder_test refactored to use testify suite pattern
- Added tests: nil resolver guard, raw query without table, negative rows,
  operationName edge cases (whitespace, newline, single word)
Expose Enabled() to skip plugin registration when disabled. Remove
config dependency from Instrument and GormPlugin. Replace contextWrapper
with gorm's tx.Statement.Settings for passing span state between
before/after callbacks. Rewrite all test files to testify suite with
table-driven cases.
Copilot AI review requested due to automatic review settings June 20, 2026 16:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.

Comment thread database/driver/gorm.go Outdated
Comment thread database/db/db.go Outdated
Comment thread telemetry/instrumentation/database/pool.go
Comment thread database/orm/orm.go
Comment thread database/db/db.go Outdated
Comment thread database/driver/gorm.go Outdated
Comment thread database/driver/gorm_test.go
Comment thread database/db/db.go Outdated
Comment thread telemetry/instrumentation/database/builder_test.go Outdated
Copilot AI review requested due to automatic review settings June 21, 2026 06:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 22 out of 23 changed files in this pull request and generated 2 comments.

Comment thread database/driver/gorm.go
Comment thread telemetry/instrumentation/database/database.go
@krishankumar01
krishankumar01 requested a review from hwbrzzl June 21, 2026 07:46

@hwbrzzl hwbrzzl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, is it possible to add tests in goravel/example to test this feature?

@krishankumar01

krishankumar01 commented Jun 22, 2026

Copy link
Copy Markdown
Member Author

LGTM, is it possible to add tests in goravel/example to test this feature?

Sure will add tests

@krishankumar01
krishankumar01 merged commit 0491095 into master Jun 22, 2026
16 of 19 checks passed
@krishankumar01
krishankumar01 deleted the kkumar-gcc/#726-database-instrumentation branch June 22, 2026 08:53
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.

4 participants