Add opt-in db.operation.name attribute on db.client.operation.duration/errors

[fatmcgav]

Closes #86.

Summary

WithMetricOperationName(fn OperationNameFunc) — a new, opt-in option — attaches db.operation.name to db.client.operation.duration/db.client.operation.errors for query, prepare, and per-statement batch-query calls, per the OTel database metrics semconv. Default (unset): unchanged behaviour, no new attribute.

Design decisions

• Default-off, independent option, per review: decoupled entirely from spanNameCtxFunc/WithSpanNameFunc/WithSpanNameCtxFunc. The span path is untouched. fn only runs once WithMetricOperationName is set, and then runs on every call (sampled or not), since metric recording is decoupled from trace sampling.
• Exported the existing default SQL-verb parser as SQLOperationName (was defaultSpanNameCtxFunc) so it can be passed explicitly: WithMetricOperationName(otelpgx.SQLOperationName). Rebased onto the Default behvior of sqlOperationName does not ignore comments #63 fix, so it skips leading SQL comments — sqlc-style -- name: ... queries produce the real verb rather than a -- label on the metric.
• No cache: benchmarked first — cached attribute.Set lookups were ~16.6ns/0 allocs vs. ~162.6ns/3 allocs uncached. That ~146ns difference is immaterial next to a real Postgres round trip (tens–hundreds of µs even on loopback), so I dropped the cache entirely rather than capping it.
• batch: TraceBatchQuery's per-statement error increment gets its own operation name; the whole-batch aggregate in TraceBatchEnd stays unlabelled (no single name for a possibly-mixed batch).
• connect/acquire/copy unaffected.

Testing

TestTracer_metricOperationName covers: unset (no attribute), SELECT/INSERT/DELETE/UPDATE, prepare, per-statement batch error, batch aggregate (no name), and a custom OperationNameFunc. go build, go vet, gofmt -l, go test -race ./... all pass locally.

[fatmcgav]

@obitech Thoughts on this one welcome 👍

[obitech]

I'm not a big fan of this in its current state because the claim "cardinality remains small" is only true when using the default parser. Complex span name parsing logic would now lead to:

• The func being applied on every call instead of just when spans are recording, incurring that cost for everyone.
• Potentially unbounded label cardinality
• A potentially unbounded sync.Map cache

I'd prefer this to be default-off and then have a dedicated option that decouples the metric attribute from the span name hook entirely. Something like:

// OperationNameFunc returns the value for db.operation.name on the
// operation duration and error metrics. Return "" to omit the attribute.
type OperationNameFunc func(ctx context.Context, sql string) string

// WithMetricOperationName enables db.operation.name on
// db.client.operation.duration and db.client.operation.errors.
func WithMetricOperationName(fn OperationNameFunc) Option

We should also export the default parser so users can write WithMetricOperationName(otelpgx.SQLOperationName) instead of some special nil argument.

The upsides here are that the span path stays as it is today, and nobody gets a new label or a hook running on unsampled calls just by bumping the lib. Also the separate function forces a conscious decision to increase cardinality.

For the cache: please benchmark whether it's even measurable next to the Postgres round trip. If not, you can drop it. If it is, cap the number of entries and build the attribute set uncached past the cap. I don't think we need any special eviction logic here.

The batch handling and the new test are good and should carry over.

Last note: the PR description is much longer than the diff warrants. A short summary and the design decisions are enough.

[fatmcgav]

Thanks for the review @obitech... Will re-work...

Also, just noticed that there is still a "legacy" SQLOperationName func for go < 1.24... Now that those versions are long since deprecated, any objections to cleaning up?

[fatmcgav]

Thanks for the detailed review — pushed a redesign addressing all of it:

• Opt-in, decoupled from span naming: replaced default-on with a new WithMetricOperationName(fn OperationNameFunc) option, exactly as you sketched. db.client.operation.duration/db.client.operation.errors carry no db.operation.name unless this is explicitly set, and fn is entirely independent of spanNameCtxFunc/WithSpanNameFunc/WithSpanNameCtxFunc — the span path is untouched.
• Exported the default parser as SQLOperationName (was defaultSpanNameCtxFunc), so WithMetricOperationName(otelpgx.SQLOperationName) gets the same low-cardinality behaviour as the span path's default, or a custom fn for anything else.
• Benchmarked the cache before touching it: cached attribute.Set lookups were ~16.6ns/0 allocs vs. ~162.6ns/3 allocs (448B) building fresh. That ~146ns difference is immaterial next to a real Postgres round trip (tens–hundreds of µs even on loopback), so I dropped the cache entirely rather than capping it.
• Batch handling and the test carried over as you suggested (extended with an "unset by default" case and a custom-fn case).
• Trimmed the PR description and dropped the README section.

Ready for another look whenever convenient.

[fatmcgav]

Also, just noticed that there is still a "legacy" SQLOperationName func for go < 1.24... Now that those versions are long since deprecated, any objections to cleaning up?

Split this into #88 👍

[fatmcgav]

@obitech LMK if you think this needs any further tweaks 👍

[fatmcgav]

@obitech Hope you had a good weekend :) Could I get another review on this one? 🙇‍♂️

[obitech]

@fatmcgav could you rebase please?

[fatmcgav]

@obitech Done 👍