Skip to content

Migrate db.sql.latency metric to db.client.operation.duration - #480

Merged
XSAM merged 11 commits into
mainfrom
metric-semantic-migration
Apr 27, 2025
Merged

Migrate db.sql.latency metric to db.client.operation.duration#480
XSAM merged 11 commits into
mainfrom
metric-semantic-migration

Conversation

@XSAM

@XSAM XSAM commented Apr 26, 2025

Copy link
Copy Markdown
Owner

Part of #388

@codecov

codecov Bot commented Apr 26, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 90.00000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 85.3%. Comparing base (757add3) to head (4c0e9f8).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
instruments.go 55.5% 3 Missing and 1 partial ⚠️
internal/semconv/attributes.go 86.9% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #480     +/-   ##
=======================================
+ Coverage   85.1%   85.3%   +0.1%     
=======================================
  Files         16      16             
  Lines        855     906     +51     
=======================================
+ Hits         728     773     +45     
- Misses       103     107      +4     
- Partials      24      26      +2     

☔ View full report in Codecov by Sentry.
📢 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.

@XSAM
XSAM marked this pull request as ready for review April 26, 2025 23:25
@XSAM
XSAM requested a review from Copilot April 26, 2025 23:46

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

This PR migrates the legacy db.sql.latency metric to the new db.client.operation.duration metric, while supporting multiple semantic convention stability options. Key changes include:

  • Introducing new functions recordLegacyLatency and recordDuration to handle legacy and new metric formats.
  • Updating the recordMetric function to switch between legacy and new behavior based on the OTEL_SEMCONV_STABILITY_OPT_IN setting.
  • Adjusting tests and instrument creation (including newMockConfig invocations) to accommodate the new metric signature and parameters.

Reviewed Changes

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

Show a summary per file
File Description
utils.go Introduces recordLegacyLatency, recordDuration, and updates recordMetric conversion logic.
tx_test.go, stmt_test.go, rows_test.go, connector_test.go, conn_test.go Updates newMockConfig calls to pass an additional nil parameter.
instruments.go Replaces the legacy 'latency' instrument with new 'legacyLatency' and 'duration' instruments.
README.md, CHANGELOG.md Updates metric descriptions and documentation to reflect the migration.
internal/semconv/* Updates error type attribution and attributes as needed by the new semantic conventions.
Comments suppressed due to low confidence (1)

tx_test.go:98

  • The updated newMockConfig calls now include an extra nil parameter. It would be helpful to update the function's documentation or inline comment to explain the purpose of this new parameter.
cfg := newMockConfig(t, tracer, nil)

Comment thread utils.go Outdated
@XSAM
XSAM requested a review from Copilot April 26, 2025 23:53

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

This PR migrates the legacy db.sql.latency metric to the new db.client.operation.duration metric following the OpenTelemetry semantic conventions while still allowing for legacy behavior based on a stability opt‐in setting.

  • Introduces two new functions, recordLegacyLatency and recordDuration, to separately handle legacy and new metric formats.
  • Updates recordMetric to switch between legacy, new, or combined metric recording based on cfg.SemConvStabilityOptIn and updates tests and documentation to support the metric migration.

Reviewed Changes

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

Show a summary per file
File Description
utils.go Updates metric recording functions and abstracts legacy/new recording strategies.
tx_test.go, stmt_test.go, rows_test.go, connector_test.go, conn_test.go Updates calls to newMockConfig with an extra parameter.
internal/semconv/attributes.go, attributes_test.go Adds error type attribute conversion for new metrics.
instruments.go, instruments_test.go Renames legacy latency instrument and creates a new duration instrument.
README.md, CHANGELOG.md Documents and logs the migration to the new metric.
Comments suppressed due to low confidence (1)

utils.go:72

  • The code uses 'driver.ErrSkip' but the 'database/sql/driver' package is not imported, which may lead to a compilation error. Consider adding the missing import.
if cfg.DisableSkipErrMeasurement && err == driver.ErrSkip {

Comment thread utils.go
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@XSAM
XSAM merged commit b27545f into main Apr 27, 2025
@XSAM
XSAM deleted the metric-semantic-migration branch April 27, 2025 00:00
@XSAM XSAM mentioned this pull request Jun 5, 2025
XSAM added a commit that referenced this pull request Jun 5, 2025
> [!WARNING]
> The new introduced `OTEL_SEMCONV_STABILITY_OPT_IN` environment
variable will be supported for at least six months from this release.
After this period, support for legacy metrics and Semantic Conventions
`v1.24.0` may be removed in the next release.
>
> You can start the migration to the new Semantic Conventions `v1.30.0`
by setting the `OTEL_SEMCONV_STABILITY_OPT_IN=database/dup` or
`OTEL_SEMCONV_STABILITY_OPT_IN=database` environment variable in your
application.
>
> See also the [Semantic conventions for database client
metrics](https://opentelemetry.io/docs/specs/semconv/database/database-metrics/).

### Added

- Support to emit query related attributes for the v1.24.0 and v1.30.0
semantic conventions based on the value of the
`OTEL_SEMCONV_STABILITY_OPT_IN` environment variable. (#478)

- `database/dup`: Emit both `db.statement` and `db.query.text`
attributes.
  - `database`: Emit `db.query.text` attribute.
  - by default: Emit `db.statement` attribute.

- New `db.client.operation.duration` metric following OpenTelemetry
semantic conventions. (#480)
- Support for configuring metrics behavior based on
`OTEL_SEMCONV_STABILITY_OPT_IN` setting. (#480)

- `database/dup`: Emit both legacy latency and new duration
`db.client.operation.duration` metrics.
  - `database`: Emit new duration `db.client.operation.duration` metric.
  - by default: Emit only the legacy latency metric.

### Changed

- Upgrade semantic conventions to `semconv/v1.30.0`. (#478)
- Improve memory usage when recording metrics or creating spans. (#497)
- Upgrade OTel to `v1.36.0/v0.58.0`. (#495)

### Fixed

- Data race issues when recording metrics or creating spans. (#497)
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.

2 participants