Skip to content

fix(tracer): skip leading SQL comments in default operation name - #89

Open
obitech wants to merge 2 commits into
mainfrom
fix/skip-leading-sql-comments
Open

obitech wants to merge 2 commits into
mainfrom
fix/skip-leading-sql-comments

Conversation

@obitech

@obitech obitech commented Sep 12, 2026

Copy link
Copy Markdown
Member

Fixes #63.

Queries that start with a SQL comment (sqlc's -- name: GetFoo :one prefix is the common case) made the default operation-name parser return -- for db.operation.name, and since v0.12.0 for the span name as well. The default parser now skips leading -- line comments and /* */ block comments before taking the first word. A user-supplied WithSpanNameFunc is unaffected and still sees the raw statement.

Second commit removes the go1.24 build constraints: go.mod requires 1.25, so the !go1.24 variant never compiled and the tags were always satisfied.

Benchmark

BenchmarkDefaultSpanNameCtxFunc, Apple M-series, -count 3 median. Zero allocations before and after.

Query main (332afb9) this PR Result on main
SELECT ... 17.4 ns/op 20.9 ns/op SELECT
-- name: GetFoo :one\nSELECT ... 8.9 ns/op 28.4 ns/op -- (wrong)
/* name: GetFoo :one */\nSELECT ... 8.9 ns/op 34.1 ns/op /* (wrong)

The plain case pays about 4 ns for the leading-whitespace trim and two prefix checks. The comment cases were only faster on main because they stopped at the wrong token.

Not in scope

  • Nested /* /* */ */ block comments are not handled and fall back to UNKNOWN. Documented on the helper.
  • A sqlc helper that extracts the query name (GetFoo) is a separate design question, since spanNameCtxFunc currently feeds both the span name and db.operation.name.

🤖 Generated with Claude Code

obitech and others added 2 commits September 12, 2026 13:11
Queries prefixed with a "-- name: ..." comment, such as sqlc emits,
made the default operation-name parser return "--", which became the
span name too once span names started defaulting to the operation
name. Skip leading "--" line comments and "/* */" block comments
before taking the first word.

Fixes #63

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
go.mod requires go 1.25, so the "!go1.24" implementation could never
compile and the "go1.24" tags were always satisfied. Consolidate into
a single implementation and drop the now-meaningless build tags.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@obitech
obitech marked this pull request as ready for review September 12, 2026 11:15

@costela costela left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do the string cuts cause a new allocation? I think they don't.
If that's the case, it may be a good idea to clone the string before returning, to avoid keeping a reference to the whole query. Otherwise we could keep large queries from being collected by the GC as long as the span is live.

I guess it depends if the string we see here is the "raw" query without template substitutions (from which we'd have a single copy in memory) or the templated query, from which we may have arbitrarily many. 🤔

Maybe it's premature optimization...

@bendiknesbo

Copy link
Copy Markdown

Spending 20-25 ns to get better span names seems worth it to me, and trying to optimize it should not be a blocker for getting this merged, in my opinion. :)

@danielbprice

Copy link
Copy Markdown

FWiW, I pulled this fix into my project to test it, and it worked as described. Excited to see this land.

@obitech

obitech commented Oct 1, 2026

Copy link
Copy Markdown
Member Author

I'd also say we merge this as is and see if we can further optimize this down the line, if needed. @costela any objections? Otherwise I'd merge it.

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.

Default behvior of sqlOperationName does not ignore comments

4 participants