feat(gql): log + count ClickHouse 158 TOO_MANY_ROWS at the origin#816
feat(gql): log + count ClickHouse 158 TOO_MANY_ROWS at the origin#816arielmelendez wants to merge 2 commits into
Conversation
GQL stable-leg queries that exceed ClickHouse `max_rows_to_read` throw
Code 158 (TOO_MANY_ROWS). Until now the only origin-side signal was the
owner-projection windowing warn, which is gated behind a default-on-in-prod
flag but only covers owner queries; the dominant 158 source — multi-id
`transactions(ids:[...])` lookups tripping the id_bloom row cap — re-threw
with no log or metric, leaving only a generic "Upstream source failed"
warn on the fan-out side.
Record every 158 at the point it is classified in the composite CH read
path, for both the recovered (owner-projection windowing) and the
fail-fast paths:
- `clickhouse_gql_too_many_rows_total{filter,recovery,id_count}` counter.
`filter` is a low-cardinality descriptor of the filter families used,
`recovery` is `windowed` or `none`, and `id_count` buckets the id-list
size (`0`/`1`/`2`/`3-5`/`6-20`/`21+`) — the load-bearing dimension, since
id_bloom scatter scales with id count and the cap is crossed around 2-3
ids.
- A dedicated warn log carrying the concrete query shape (idCount, ids,
owners, recipients, bundledIn, tags, height range, page size, sort
order) so operators can audit exactly which queries trip the cap.
The prior owner-projection-only warn is folded into the unified log.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #816 +/- ##
===========================================
+ Coverage 78.77% 78.79% +0.01%
===========================================
Files 133 133
Lines 50477 50566 +89
Branches 3815 3827 +12
===========================================
+ Hits 39761 39841 +80
- Misses 10666 10674 +8
- Partials 50 51 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a new Prometheus counter for ClickHouse Code 158 events, classifies those events by query shape and id-count bucket, records the metric during stable-leg retry handling, and extends tests for windowed and fail-fast paths. ChangesCode 158 Metrics and Retry Handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant getGqlTransactions
participant ClickHouse
participant metrics
Client->>getGqlTransactions: query stable transactions
getGqlTransactions->>ClickHouse: transactions query
ClickHouse-->>getGqlTransactions: Code 158 TOO_MANY_ROWS
getGqlTransactions->>getGqlTransactions: describeGqlFilterShape / bucketIdCount
getGqlTransactions->>metrics: clickhouseGqlTooManyRowsTotal.inc(filter, recovery, id_count)
alt owner-projection applies and ids.length === 0
getGqlTransactions->>ClickHouse: queryStableTransactionsWindowed retry
ClickHouse-->>getGqlTransactions: windowed results
getGqlTransactions-->>Client: response
else
getGqlTransactions-->>Client: re-thrown error
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/metrics.ts (1)
676-690: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider TSDoc format for the new exported counter.
The descriptive comment is thorough, but per coding guidelines new/touched code in
src/**/*.tsshould use TSDoc (/** ... */) rather than a plain//block, especially for an exported symbol.As per coding guidelines: "Add or improve TSDoc comments on code you touch".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/metrics.ts` around lines 676 - 690, The exported counter clickhouseGqlTooManyRowsTotal is documented with a plain line comment instead of TSDoc; convert the existing description into a proper TSDoc block on the exported symbol. Keep the same summary/details about the metric and its labels, and ensure the comment is attached directly to clickhouseGqlTooManyRowsTotal so it satisfies the codebase guideline for touched exported TypeScript symbols.Source: Coding guidelines
src/database/composite-clickhouse.test.ts (1)
652-683: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid coverage; consider also asserting the
filterlabel.The
recovery/id_countassertions are correctly derived from the implementation. SincedescribeGqlFilterShapeis also new logic feeding this same counter, consider extending one of these tests (e.g. this multi-id case) to assertv.labels.filter === 'ids'for extra regression protection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/database/composite-clickhouse.test.ts` around lines 652 - 683, The multi-id fail-fast counter test covers recovery and id_count, but it should also verify the filter label emitted by describeGqlFilterShape. Update the existing composite.getGqlTransactions test (or a similar counter assertion in composite-clickhouse.test.ts) to capture the counter labels and assert that v.labels.filter equals 'ids', alongside the current recovery and id_count checks.src/database/composite-clickhouse.ts (1)
160-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHelper logic looks correct; consider TSDoc format.
describeGqlFilterShapeandbucketIdCountare correctly bounded/low-cardinality per their doc comments. Per coding guidelines, new functions insrc/**/*.tsshould use TSDoc (/** ... */) rather than plain//blocks — the file already uses TSDoc elsewhere (e.g.queryStableTransactionsWindowed).As per coding guidelines: "Add or improve TSDoc comments on code you touch".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/database/composite-clickhouse.ts` around lines 160 - 196, The helper logic in describeGqlFilterShape and bucketIdCount is fine, but their documentation uses plain line comments instead of TSDoc. Replace the existing comment blocks with proper TSDoc-style comments so they match the rest of composite-clickhouse.ts and the project guideline for touched code, keeping the descriptions and intent the same.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/database/composite-clickhouse.ts`:
- Around line 1200-1232: The metric and warning log use the same recovery
concept but different values, which breaks easy correlation between the counter
and the log. Update the `metrics.clickhouseGqlTooManyRowsTotal.inc` call and the
paired `this.log.warn` payload in `composite-clickhouse.ts` so `recovery` uses
the same vocabulary in both places, or rename one field to avoid ambiguity. Keep
the existing `willRetryWindowed` decision in sync with the `recovery` value
produced by `ownerProjectionApplies`, `ids`, and the ClickHouse max-rows retry
path.
---
Nitpick comments:
In `@src/database/composite-clickhouse.test.ts`:
- Around line 652-683: The multi-id fail-fast counter test covers recovery and
id_count, but it should also verify the filter label emitted by
describeGqlFilterShape. Update the existing composite.getGqlTransactions test
(or a similar counter assertion in composite-clickhouse.test.ts) to capture the
counter labels and assert that v.labels.filter equals 'ids', alongside the
current recovery and id_count checks.
In `@src/database/composite-clickhouse.ts`:
- Around line 160-196: The helper logic in describeGqlFilterShape and
bucketIdCount is fine, but their documentation uses plain line comments instead
of TSDoc. Replace the existing comment blocks with proper TSDoc-style comments
so they match the rest of composite-clickhouse.ts and the project guideline for
touched code, keeping the descriptions and intent the same.
In `@src/metrics.ts`:
- Around line 676-690: The exported counter clickhouseGqlTooManyRowsTotal is
documented with a plain line comment instead of TSDoc; convert the existing
description into a proper TSDoc block on the exported symbol. Keep the same
summary/details about the metric and its labels, and ensure the comment is
attached directly to clickhouseGqlTooManyRowsTotal so it satisfies the codebase
guideline for touched exported TypeScript symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c5b50f9c-1878-44bb-a6fd-c337d17dcce2
📒 Files selected for processing (3)
src/database/composite-clickhouse.test.tssrc/database/composite-clickhouse.tssrc/metrics.ts
- Align the warn log's `recovery` value with the counter label (`windowed`/`none`) so logs and the metric correlate directly. - Convert the new counter and helper doc comments to TSDoc. - Assert the `filter="ids"` label in the multi-id 158 counter test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks @coderabbitai — addressed the actionable comment plus all three nitpicks in
|
|
Tip For best results, initiate chat on the files or code changes.
I'll take a look at commit (=^･ω･^=) |
1 participant
What
GQL stable-leg queries that exceed ClickHouse
max_rows_to_readthrowCode: 158 TOO_MANY_ROWS. Today the only origin-side signal is the owner-projection height-windowing warn, which covers owner queries only and is easy to miss — the dominant 158 source (multi-idtransactions(ids:[...])lookups tripping theid_bloomrow cap) re-throws with no log or metric, leaving only a generic "Upstream source failed" warn on the fan-out side whose error string has to be substring-matched forTOO_MANY_ROWS.This records every 158 at the point it's classified in the composite ClickHouse read path, for both the recovered (owner-projection windowing) and the fail-fast paths:
clickhouse_gql_too_many_rows_total{filter, recovery, id_count}filter— low-cardinality descriptor of the filter families used (e.g.ids,owners+tags,none)recovery—windowedwhen the owner-projection height-windowing fallback re-ran it,nonewhen the 158 surfaced to the callerid_count— coarse bucket of the id-list size (0/1/2/3-5/6-20/21+). This is the load-bearing dimension:id_bloomfalse-positive scatter scales ~linearly with id count, and the cap is crossed around 2–3 ids.idCount, ids, owners, recipients, bundledIn, tags, height range, page size, sort order) so operators can audit exactly which queries trip the cap without parsing rendered SQL out ofsystem.query_log.The prior owner-projection-only warn is folded into the unified log.
Why
Investigating 158 load on the canary gateways, the only retrospective source was ClickHouse
system.query_log(full SQL, ~3-month retention). That's fine for offline audits but there's no live signal — nothing to alert or trend on, and the app logs carried no usable trace at the origin. This adds that live signal with the id-count dimension the audit showed actually matters.Risk
Purely additive — a counter increment and a warn log inside an existing
catchblock. No change to query results or control flow. Low log volume (a warn per 158), bounded counter cardinality.Tests
New unit coverage asserts the counter fires with the right
recoveryoutcome on both the windowed and fail-fast paths, and bucketsid_countcorrectly for a multi-id fail-fast query.tsc, eslint, prettier, and thecomposite-clickhousesuite all pass.