Repository navigation
fix(links): report Kafka probe failures as degraded, not unavailable - #1140
Tyagiquamar wants to merge 13 commits into
Conversation
…ll assertions Co-authored-by: Cursor <cursoragent@cursor.com>
…ss decide HTTP status Removes the health endpoint short-circuit that returned 503 whenever didKafkaConnectFail() was set. Kafka down with ClickHouse up is a degradation (HTTP 200), which calculateLinkReadiness already encodes. The loud rejection from refreshProducerConnection and the kafka_health_connect_failed attribute are unchanged. Also extends the links CI status poll past the 60s producer reconnect cooldown so one transient first-connect failure does not fail the job.
External pull requests cannot modify workflow files in this repo (tripwire protected-file rule). The links status poll extension is dropped from this PR; a maintainer can land it separately if wanted.
Keep calculateLinkReadiness as the single HTTP status policy. Treat idle producer state after a connect failure as redpanda error so Kafka+CH stays HTTP 200 degraded instead of falsely reporting ok.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
@Tyagiquamar is attempting to deploy a commit to the Databuddy OSS Team on Vercel. A member of the Team first needs to authorize it. |
 [Medium impact] The changes affect health reporting and Kafka connections within the Links service. The behavior changes look sound, but the new tests must satisfy the explicit project rules before merging. Findings
SummaryThe PR reports failed Kafka probes as degraded without making Kafka failure alone return HTTP 503.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Kafka health probe] --> B{Connected?}
B -->|Yes| C[Redpanda ok]
B -->|No| D[Redpanda error]
C --> E[calculateLinkReadiness]
D --> E
E --> F{Postgres and Redis healthy?}
F -->|No| G[HTTP 503 unavailable]
F -->|Yes| H{ClickHouse or Redpanda healthy?}
H -->|No| G
H -->|Yes, one failed| I[HTTP 200 degraded]
H -->|Yes, none failed| J[HTTP 200 ok]
Reviews (1) · Last reviewed commit: "fix(links): map failed Kafka probe to de..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| process.env.REDPANDA_BROKER = "redpanda.test:9092"; | ||
| const realNow = Date.now; | ||
| let now = 1000; | ||
| Date.now = () => now; |
There was a problem hiding this comment.
The new tests assign directly to Date.now and use delete on environment fields. The project guide forbids assignments to native objects and use of the delete operator. Use a restorable clock spy and Reflect.deleteProperty for the environment cleanup. These repository requirements must be satisfied before merging.
Context Used: Ultracite Rules - AI-Ready Formatter and Linter (source)
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!
|
Supersedes closed #721 per izadoesdev review: no Kafka 503 short-circuit; \mapRedpandaServiceStatus\ maps \idle\ + \didKafkaConnectFail()\ to \error\ so Kafka-down + ClickHouse-up stays HTTP 200 \degraded. Tests: \�un test apps/links/src/lib/health.test.ts\ (6 pass) on \c055751c0. |
Use spyOn for Date.now and Reflect.deleteProperty for env cleanup in producer tests.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
This branch was previously deployed
2 participants
Summary
Addresses maintainer feedback on #721 (supersedes the closed PR).
calculateLinkReadinessas the single HTTP status policy: Kafka down with ClickHouse up stays HTTP 200 degraded.errorwhen the health probe leaves the producer inidlebutdidKafkaConnectFail()is true (fixes falseokreadiness).Linked issue
Fixes #719
Test plan
bun test apps/links/src/lib/health.test.ts(6 pass)AI disclosure
AI-assisted implementation; human-reviewed against izadoesdev review on #721.
Summary by cubic
Fixes the links service health check so Kafka failures report degraded instead of unavailable, and no longer reports false
okreadiness after a connect failure.calculateLinkReadiness, replacing the previous 503 short-circuit.erroreven when the producer state isidle, fixing the falseok.refreshProducerConnectionrejects loudly on failure and records akafka_health_connect_failedattribute.sendLinkVisitkeeps falling back to ClickHouse when Kafka is unreachable, anddisconnectProducerno longer fails if a pending connection dies during shutdown.REDPANDA_SSLorREDPANDA_SSL_ENABLEDis set tofalse(previously always on).Written for commit 56c792a. Summary will update on new commits.