[superlog] Warn on tracking health ClickHouse timeouts#516
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
|
The latest updates on your projects. Learn more about Unkey Deploy
|
Greptile SummaryThis PR extracts the ClickHouse tracking-health timeout into a shared constant and a small
Confidence Score: 4/5Safe to merge — changes are narrowly scoped to log-level routing for a known, self-generated timeout error; no data path or return value is affected. Both catch blocks now resolve log level through a pure, unit-tested helper, and the only call-sites of the new constant are the same setTimeout rejections that produced the original hardcoded string. The one rough edge is a hardcoded string literal in the test file that duplicates the constant's value — minor and non-blocking, but worth tidying before the constant is ever renamed. No files require special attention; the test file has a minor literal duplication worth a quick fix. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ClickHouse query / Promise.race] --> B{Race resolves?}
B -- yes --> C[Return result]
B -- no / timeout --> D["reject(new Error(CLICKHOUSE_TRACKING_HEALTH_TIMEOUT_MESSAGE))"]
D --> E[catch block]
E --> F[getTrackingHealthErrorLogLevel]
F --> G{error instanceof Error\nAND message === constant?}
G -- yes --> H["level = 'warn'"]
G -- no --> I["level = 'error'"]
H --> J["logger.warn(...)"]
I --> K["logger.error(...)"]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[ClickHouse query / Promise.race] --> B{Race resolves?}
B -- yes --> C[Return result]
B -- no / timeout --> D["reject(new Error(CLICKHOUSE_TRACKING_HEALTH_TIMEOUT_MESSAGE))"]
D --> E[catch block]
E --> F[getTrackingHealthErrorLogLevel]
F --> G{error instanceof Error\nAND message === constant?}
G -- yes --> H["level = 'warn'"]
G -- no --> I["level = 'error'"]
H --> J["logger.warn(...)"]
I --> K["logger.error(...)"]
Reviews (1): Last reviewed commit: "fix(rpc): warn on tracking health timeou..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| it("keeps non-Error values at error", () => { | ||
| expect(getTrackingHealthErrorLogLevel("ClickHouse query timeout")).toBe( | ||
| "error" | ||
| ); | ||
| }); |
There was a problem hiding this comment.
"ClickHouse query timeout" rather than the already-imported CLICKHOUSE_TRACKING_HEALTH_TIMEOUT_MESSAGE constant. If the constant's value is ever updated, the literal won't track the change and the test may silently stop exercising the intended edge-case (exact-match string as non-Error).
| it("keeps non-Error values at error", () => { | |
| expect(getTrackingHealthErrorLogLevel("ClickHouse query timeout")).toBe( | |
| "error" | |
| ); | |
| }); | |
| it("keeps non-Error values at error", () => { | |
| expect(getTrackingHealthErrorLogLevel(CLICKHOUSE_TRACKING_HEALTH_TIMEOUT_MESSAGE)).toBe( | |
| "error" | |
| ); | |
| }); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Clean replacement for #468, based on staging.\n\nWhat changed:\n- Adds a tested tracking-health log-level helper.\n- Downgrades only the canonical ClickHouse tracking-health timeout to WARN.\n- Keeps unexpected tracking event / blocked-traffic failures at ERROR.\n- Reuses one timeout message constant for both tracking-health ClickHouse probes.\n\nVerification:\n- cd packages/rpc && bun test src/routers/tracking-health-errors.test.ts\n- bunx ultracite check packages/rpc/src/routers/websites.ts packages/rpc/src/routers/tracking-health-errors.ts packages/rpc/src/routers/tracking-health-errors.test.ts\n- cd packages/rpc && bun run check-types\n- cd packages/rpc && bun run test\n- bun run lint\n- bun run check-types\n- bun run test
Summary by cubic
Downgraded the canonical ClickHouse tracking-health timeout to warn to reduce noisy error logs. Added a small helper and constant to centralize log-level handling while keeping unexpected failures at error.
CLICKHOUSE_TRACKING_HEALTH_TIMEOUT_MESSAGEandgetTrackingHealthErrorLogLevel()and applied to both probes.Written for commit 3b5da1d. Summary will update on new commits.