Repository navigation
Fix OpenTelemetry: db.name reports the configured database, not the database sent with the request - #687
Open
polyglotAI-bot wants to merge 1 commit into
Open
Fix OpenTelemetry: db.name reports the configured database, not the database sent with the request#687polyglotAI-bot wants to merge 1 commit into
polyglotAI-bot wants to merge 1 commit into
Conversation
The HTTP client set the db.name span tag from ClickHouseClientSettings.Database, so a query sent to another database (QueryOptions.Database, InsertOptions.Database, or ClickHouseConnection.ChangeDatabase for commands) still reported the configured database. PostSqlQueryAsync and PostStreamAsync now build the URI first and pass its resolved Database to StartActivity. Fixes: #685 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
polyglotAI-bot
requested review from
alex-clickhouse and
mzitnik
as code owners
October 15, 2026 07:22
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The narrow fix reuses existing database resolution, covers both span sites, and includes regression tests and matching documentation.
0 open findings
What changed in this PR
Fixes #685 in the HTTP client by making OpenTelemetry db.name match the database sent with each request, without changing query behavior.
Changes:
- Pass the resolved request database to both span creation sites.
- Add regression tests for query overrides, inserts, and
ChangeDatabase. - Document the corrected behavior and add a changelog fragment.
| File | Description |
|---|---|
docs/http.mdx |
Clarifies how db.name is resolved. |
ClickHouse.Driver/Diagnostic/ActivitySourceHelper.cs |
Accepts the resolved database for span tagging. |
ClickHouse.Driver/ClickHouseClient.cs |
Supplies the request database at both span sites. |
ClickHouse.Driver.Tests/ActivitySourceHelperTests.cs |
Adds database-tag regression coverage. |
changelog.d/685-otel-db-name.fixes.md |
Records the telemetry correction. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #685.
Root cause.
ActivitySourceHelper.StartActivitysetdb.namefromclient.Settings.Database. It did not get the per-query options. The request itself usesCreateUriBuilder, which sendsqueryOverride?.Database ?? Settings.Database. So a query sent withQueryOptions.Database/InsertOptions.Databasereported the configured database, or""when the connection string has noDatabase. This also applied toClickHouseCommandafterClickHouseConnection.ChangeDatabase, because the command forwards the selected database asQueryOptions.Database. Only the trace was wrong: the query ran in the correct database.Fix.
PostSqlQueryAsyncand the privatePostStreamAsync(the only two span sites) now build the URI first and passuriBuilder.DatabasetoStartActivity. The tag therefore comes from the same value the request sends, so the two cannot drift. AllExecute*methods,ClickHouseCommand, theInsertBinaryAsyncschema lookup and batches,InsertRawStreamAsync, and the publicPostStreamAsyncoverloads go through these two sites.Changes
Diagnostic/ActivitySourceHelper.cs:StartActivitygets an optionaldatabaseargument. When the argument isnull, the tag stays the client's configured database.ClickHouseClient.cs: both span sites pass the resolved database.docs/http.mdx: thedb.namerow now says which database is reported.changelog.d/685-otel-db-name.fixes.md: changelog fragment.Behavior
DatabaseQueryOptions.Databasedb.namebeforedb.nameafter""""""(unchanged)systemsystemsystem(unchanged)""system""systemsystemdefaultsystemdefaultsystem""system""(the request sends nodatabase, so the server uses the user's default database)Not changed, for a maintainer decision (raised in the issue): when neither the connection string nor the query sets a database, the tag is still
"". The client does not know the user's server-side default database, so leaving the tag out may be better.Tests
New tests in
ActivitySourceHelperTestsgo through the real entry points against a live server:ExecuteScalarAsync_ConfiguredAndQueryOptionsDatabase_SetsDbNameToDatabaseSentWithRequest: one parametrized test for the five rows above. Each case uses its own client with an explicit configured database. The two no-override rows pin the unchanged behavior.InsertBinaryAsync_WithInsertOptionsDatabase_SetsDbNameToInsertDatabaseOnEverySpan: both spans (the schema lookupPostSqlQueryAsyncand thePostStreamAsyncbatch) reportInsertOptions.Database.ExecuteScalarAsync_AfterChangeDatabase_SetsDbNameToSelectedDatabase: afterChangeDatabase("system"), the tag equals the server'scurrentDatabase().On unpatched
main, 5 of the new cases fail with the wrong tag and the 2 no-override cases pass. With the fix, all pass. The fullClickHouse.Driver.Testssuite passes on net10.0 against ClickHouse 26.9 (14823 passed, 0 failed, 142 skipped). All target frameworks build with no new analyzer warnings.Related: #684 (
ClickHouseConnection.PostStreamAsyncandClickHouseBulkCopyignoreChangeDatabase). It does not overlap with this change. When #684 sends the selected database on those paths, their spans report it automatically, because the tag now comes from the URI builder.Checklist
main; the new tests fail without the fix and pass with itdocs/http.mdx); no public API change (StartActivityis internal)🤖 Generated with Claude Code