Skip to content

feat(otel): richer spans — full summary on query span, span_attributes, and dangerously_log_query_text#950

Merged
peter-leonov-ch merged 5 commits into
mainfrom
feat/otel-attributes-langfuse
Jul 16, 2026
Merged

feat(otel): richer spans — full summary on query span, span_attributes, and dangerously_log_query_text#950
peter-leonov-ch merged 5 commits into
mainfrom
feat/otel-attributes-langfuse

Conversation

@peter-leonov-ch

@peter-leonov-ch peter-leonov-ch commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Summary

Implements the clickhouse-js instrumentation feedback from Langfusecloses #948 (Slack thread with Steffen Schmitz). The goal: make the client's OpenTelemetry spans carry enough detail that a downstream app can drop its own hand-rolled ClickHouse span wrapper.

Coverage of #948

1. Record X-ClickHouse-Summary on the query() span, and record all keys.

  • Node parses the summary on connection.query (parse_summary: true) and records it on the clickhouse.query span (previously only command/exec/insert carried it).
  • The Web client now parses the X-ClickHouse-Summary header for query/command/exec/insert — it parsed none before.
  • setResponseSpanAttributes now iterates over every key in the parsed summary (clickhouse.summary.<key>) instead of a hardcoded subset, so total_rows_to_read, memory_usage, and future keys (real_time_microseconds, …) arrive for free. memory_usage added to ClickHouseSummary.

2. Opt-in query text attribute (db.query.text).

  • New dangerously_log_query_text client option (default false). When enabled, the raw SQL is attached as OTEL db.query.text and (Node) included in the error-level request-failure logs. Bound query_params values and credentials are never logged or traced. Parameterized queries record only the statement text (with placeholders), which is exactly Langfuse's use case.

3. Row counts for non-streaming result consumption.

  • db.response.returned_rows is now set for non-streaming json() consumption (JSON / JSONObjectEachRow / other single-document JSON formats) too, not just row-streaming paths. Parity across node and web ResultSet.

4. A small, scoped hook for enriching span attributes.

  • New per-request span_attributes bag on query/command/exec/insert/ping (the per-call option Steffen preferred). Merged into the operation span; caller attributes never override the client's own db.*/server.*/clickhouse.* attributes.

Relationship to #884

This supersedes #884. That PR drops the query-text plumbing from the Node log/transport layer; per the design discussion, this PR instead guards it behind dangerously_log_query_text (safe-by-default — query text is absent from logs unless explicitly opted in). The logRequestError hardening from #884 (scalars only, no full params/search-params object) is preserved here, and the no-SQL-leak regression test is carried forward and extended to cover the flag-on path. #884 can be closed once this lands.

Verification

  • New unit tests: common tracer (db.query.text on/off, summary on the query span, all-keys recording, span_attributes merge + precedence), node ResultSet span (returned_rows for JSON/JSONObjectEachRow), node no-leak/guarded-log regression, web summary parsing (present/absent/malformed).
  • npm run build, npm run typecheck, npm run lint, full Node (406) and Web (255) unit suites all pass.
  • End-to-end against a live ClickHouse: confirmed the clickhouse.query span carries the full summary (incl. total_rows_to_read, memory_usage), db.query.text, and the caller's span_attributes with core attributes intact; and returned_rows is present for non-streaming json().

Checklist

  • Unit tests added
  • Docs updated (docs/howto/tracing.md)
  • CHANGELOG entries added (client-node, client-web)

🤖 Generated with Claude Code

…s, dangerously_log_query_text

Implements the clickhouse-js instrumentation feedback from Langfuse
(Slack thread with Steffen Schmitz):

- Attach the X-ClickHouse-Summary counters to the `clickhouse.query` span
  too (previously only on command/exec/insert). Node now parses the summary
  on `connection.query`; the Web client learns to parse the summary header
  for query/command/exec/insert (it parsed none before).
- Add `clickhouse.summary.total_rows_to_read` and
  `clickhouse.summary.memory_usage`, plus `real_time_microseconds` on
  servers that report it. Add `memory_usage` to `ClickHouseSummary`.
- Add a per-request `span_attributes` bag (query/command/exec/insert/ping)
  for app-level context (e.g. mirroring `log_comment` tags). Caller
  attributes never override the client's own db.*/server.*/clickhouse.*.
- Add a `dangerously_log_query_text` client option (default false). When
  enabled, the raw SQL is attached as OTEL `db.query.text` on spans and
  (Node) included in error-level request logs. Bound query_params values
  and credentials are never logged or traced. This guards — rather than
  drops — the query-text plumbing that PR #884 removes, superseding it.

Adds unit tests (common tracer, node no-leak/guarded-log, web summary
parsing) and updates docs/howto/tracing.md and the package CHANGELOGs.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 16, 2026 13:26
@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 73.33333% with 16 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
packages/client-web/src/result_set.ts 40.00% 5 Missing and 4 partials ⚠️
packages/client-node/src/result_set.ts 66.66% 2 Missing and 3 partials ⚠️
packages/client-common/src/client.ts 88.88% 0 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

… json()

Addresses the remaining points from issue #948:

- Record every key present in the X-ClickHouse-Summary header as
  `clickhouse.summary.<key>` instead of a hardcoded subset, so
  `total_rows_to_read`, `memory_usage`, and future server-side additions
  (e.g. `real_time_microseconds`) are picked up automatically.
- Set `db.response.returned_rows` for non-streaming `json()` consumption too
  (JSON / JSONObjectEachRow / other single-document JSON formats), not just
  row-streaming paths — parity across node and web ResultSet.

Adds unit tests and updates docs/CHANGELOGs (with the #948/#950 refs).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

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

This PR enhances ClickHouse JS client OpenTelemetry instrumentation so downstream apps can rely on the client’s spans (and optional SQL text) without maintaining their own ClickHouse span wrapper.

Changes:

  • Attach full X-ClickHouse-Summary counters to the outer clickhouse.query span (Node) and add header parsing on the Web client so web spans also get summary counters.
  • Add per-request span_attributes to enrich spans with app-level context, with precedence rules to avoid overriding core client attributes.
  • Introduce dangerously_log_query_text (default false) to optionally attach raw SQL as db.query.text on spans and include it in Node error logs on request failure.

Reviewed changes

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

Show a summary per file
File Description
packages/client-web/src/connection/web_connection.ts Parse X-ClickHouse-Summary header and propagate summary through query/exec/command/insert results.
packages/client-web/CHANGELOG.md Document new tracing/span features for the web package.
packages/client-web/tests/unit/web_summary.test.ts Add unit coverage for Web summary-header parsing behavior (present/absent/malformed).
packages/client-node/src/connection/node_base_connection.ts Parse summary for query() and gate raw SQL logging behind dangerously_log_query_text.
packages/client-node/CHANGELOG.md Document new tracing/span features for the node package.
packages/client-node/tests/unit/node_log_query_text.test.ts Add regression tests ensuring params/credentials never log, and SQL logs only when opted in.
packages/client-common/src/connection.ts Plumb dangerously_log_query_text into connection params; ensure query results carry optional summary.
packages/client-common/src/config.ts Add dangerously_log_query_text config option and pass it into normalized connection params.
packages/client-common/src/client.ts Merge span_attributes, attach db.query.text when enabled, and set new summary span attributes.
packages/client-common/src/clickhouse_types.ts Extend ClickHouseSummary with optional memory_usage.
packages/client-common/tests/unit/tracing.test.ts Add tests for summary-on-query-span, optional summary fields, query text on/off, and span_attributes behavior.
docs/howto/tracing.md Document span_attributes, summary counters on spans, and the dangerously_log_query_text opt-in.

Comment thread packages/client-common/src/client.ts
Comment thread packages/client-node/CHANGELOG.md Outdated
Comment thread packages/client-web/CHANGELOG.md Outdated
…flag

Copilot review: a caller could set `span_attributes: { "db.query.text": ... }`
and, with `dangerously_log_query_text` disabled, the value survived onto the
span — undermining the safe-by-default guarantee. `db.query.text` is now
reserved and only ever set through the `dangerously_log_query_text` path.
Adds tests for both flag states.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

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 15 out of 15 changed files in this pull request and generated no new comments.

peter-leonov-ch and others added 2 commits July 16, 2026 16:06
CODE_OF_CONDUCT.md (added to main in #942) was not prettier-formatted, so
the repo-wide `prettier --check .` in the code-quality job failed on every
PR that merges with main. Reformat it (`*` list markers -> `-`) to unblock
the check. Folded into this PR per maintainer request.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@peter-leonov-ch
peter-leonov-ch merged commit 06d549b into main Jul 16, 2026
72 of 73 checks passed
@peter-leonov-ch
peter-leonov-ch deleted the feat/otel-attributes-langfuse branch July 16, 2026 14:17
peter-leonov-ch added a commit that referenced this pull request Jul 16, 2026
Retarget the staged (unreleased) `# 1.23.2` CHANGELOG section to `#
1.24.0` in both client packages, ahead of cutting the 1.24.0 beta.

#950 added new public API (`dangerously_log_query_text` config option +
per-request `span_attributes`), which is an additive **minor** release.
`1.23.2` was never published, so the section's contents — the OTEL
features (#950) and the #947 `Array(Date)` binding fix — ship together
as `1.24.0`.

Header-only change; content is unchanged. Required by the release
skill's Step 1 (the CHANGELOG must carry the exact version header before
the version bump).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@peter-leonov-ch peter-leonov-ch mentioned this pull request Jul 16, 2026
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.

Extended properties in OTel spans

2 participants