[artile]

Statement spans give no hook for duration, rows and errors, and there is no rule for parameter values. A Redacted value in sql...`` was also bound as the Redacted object, so the database stored the text ``.

Closes #77.
Part of #52.

Changes

• observeExit on fromSqlClient: called with a StatementObservation when each statement ends. It has sql, parameters, system, operation, collection, summary, tenantScope, route, durationMillis, rows and errorType.
• spanParameters: true puts the bound values on the statement span as db.query.parameter.<index> (OpenTelemetry semconv). Off by default, so spans carry no values.
• CompiledQuery.sensitiveParameters: the indexes of values given as Redacted. This covers compared values, param.* values, and values that an insert or update writes. Use a column such as PG.brand(PG.text, Schema.RedactedFromValue(Schema.String)). The driver gets the plain value. Spans write <redacted>. observeExit gets the value still Redacted.
• sql...``: binds a Redacted value as its plain value and marks it (fix). On ClickHouse, which writes values into the text, it refuses a `Redacted` value.
• Statement.sensitiveParameters, docs (database.md "Observing statements" and "Redaction", params-and-compilation.md, reference.md) and a minor changeset.

Left out

• One span per transaction or batch: this needs a statement collector design of its own. Also, D1 batch() (D1 batch() as a typed atomic unit #55) is not in yet.
• Redaction on ClickHouse: the dialect writes values into the SQL text. Effect's sql.execute span (db.query.text) and DatabaseError.sql carry that text. A fix needs bound params over HTTP.
• in_ on a Redacted column fails to encode today (an array goes to the element codec). This is a separate issue in the builder.
• CH.sql fragments inside a builder query do not mark Redacted values.
• observeExit does not see BEGIN, COMMIT and ROLLBACK. Effect's withTransaction sends them, not Database.
• observe (before the statement) is unchanged and still gets the plain values.
• No SQLite, MySQL or D1 part.

Validation

With EFFECT_ORM_CLICKHOUSE_URL=http://127.0.0.1:8123 (ClickHouse 26.8.2.7, the CI version), EFFECT_ORM_CLICKHOUSE_USER=maple and EFFECT_ORM_CLICKHOUSE_PASSWORD=maple:

• bun run build: ok.
• bun run typecheck: ok, no errors.
• bunx vitest run --testTimeout=60000 --hookTimeout=60000: 61 files passed, 1046 tests passed, 0 skipped. The longer timeouts are for local machine load only.
• node scripts/check-doc-citations.mjs: ok (25 files). node scripts/check-exports-documented.mjs: ok. node scripts/check-doc-examples.mjs: ok (21 files).
• bun scripts/check-package.ts: ok (tarball imports, types and CommonJS consumer).

New tests: src/pg/sensitive-parameters.test.ts (5), 4 in src/database/tracing.test.ts (observeExit, span parameters), and 2 in src/database/database.test.ts (sql with Redacted).

Upstream CI for fork PRs waits for a maintainer to approve the run.

---

^{Need help on this PR? Tag @codesmith-bot with what you need.}

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 39 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 9bc4b84b-f27f-4cbf-bcb9-5e64ebfdfa63

Commits

Reviewing files that changed from the base of the PR and between b57faf1 and eb5d284.

Files selected for processing: 14

• .changeset/observe-exit-redaction.md
• docs/database.md
• docs/params-and-compilation.md
• docs/reference.md
• src/ch/compile.ts
• src/ch/literal.ts
• src/database.ts
• src/database/database.test.ts
• src/database/database.ts
• src/database/sql.ts
• src/database/tracing.test.ts
• src/database/tracing.ts
• src/pg/sensitive-parameters.test.ts
• src/sql/literal-binder.ts

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[Makisuo]

Merge-readiness pass (head eb5d284):

Changed

• Merged main. The only conflicts were imports in src/ch/compile.ts (main's schemaOf next to isSensitive) and src/database/tracing.ts.
• SQLite and D1 bind ? values and go through the same renderParams, withBoundLiterals and valueCells path, so they mark Redacted values the same way Postgres does. New tests: compiled selects and inserts on sqliteDialect and d1Dialect mark the right indexes. A node:sqlite Database with spanParameters and observeExit stores the plain value (builder insert and sql template), observes it still Redacted, and leaves no secret in the observations. system is sqlite.
• Docs and changeset now say which dialects redact (Postgres, SQLite, D1). They also say that on ClickHouse a Redacted column value written by the builder is inlined into the statement text.

For the maintainer to decide

• Overlap with feat(database): make the statement span the only span a statement gets #126 (statement span only). I trial-merged feat(database): make the statement span the only span a statement gets #126 into this branch locally (not pushed). It conflicts in database.ts, tracing.ts and tracing.test.ts. Both sides add options and helpers, and both rewrite the same tapError. Resolving that means keeping errorTypeOf and adding feat(database): make the statement span the only span a statement gets #126's db.response.status_code to it. There are also semantic changes once both are in:
  • feat(database): make the statement span the only span a statement gets #126 reads the operation and table off plain SQL, so StatementObservation.operation and collection would be set for sql templates. This PR's docs say "Absent for sql...``", and the observeExit error test expects `operation` to be undefined. With feat(database): make the statement span the only span a statement gets #126 it would be `SELEKT`.
  • feat(database): make the statement span the only span a statement gets #126 puts db.query.text on the statement span. On ClickHouse that is the inlined text, including any Redacted builder value. Today the same text is on Effect's driver span, so exposure does not change, but it moves to the span users keep.
  • This PR's "no secret on any span" test assumes the driver span can be present. That still holds with feat(database): make the statement span the only span a statement gets #126's driverSpans default.
• Recommendation: land feat(database): make the statement span the only span a statement gets #126 first, then merge main into this branch and adapt: combine the tapError, update the operation/collection docs and that test, and consider adding statusCode to StatementObservation. feat(database): make the statement span the only span a statement gets #126 is smaller and changes the default trace shape this PR builds on, so it is cheaper to adapt here than in feat(database): make the statement span the only span a statement gets #126. I estimate a short, mechanical follow-up.
• ClickHouse builder queries with a Redacted column still inline the plain value. sql templates refuse one on ClickHouse, but the builder does not, so the two behave differently. I left that alone because refusing would break existing ClickHouse users of such columns.

Verified

• bun run typecheck and bun run build. bun run test with a live ClickHouse 26.8: 1443 of 1444 passed. The one failure was the known ClickHouse "Division by zero" flake in "generated fixtures: union results agree (clickhouse-join-nulls)", which test(fuzz): fix three flaky generated-data properties #131 fixes. Doc checks passed when run separately.
• FUZZ_SCALE=0.3 vitest run tests/fuzz: 269 passed. tests/database.clickhouse.test.ts and src/database/tracing.test.ts live: green.
• CI run 41474368094 was in progress when I posted this.