JITSU-88 fix(ingest): silent success on quota block + stream-id metric label#1413
JITSU-88 fix(ingest): silent success on quota block + stream-id metric label#1413absorbb wants to merge 2 commits into
Conversation
…JITSU-88)
Two changes preparing ingest for the JITSU-88 usage-quota block, which
works by setting throttle=100 on a workspace.
1. Throttled events now return a success response instead of HTTP 402.
V1 blocking must be silent — the client should see no ingestion error
(events are still preserved in backup and simply not delivered to
destinations). sendToRotor writes {"ok":true} (or the tracking gif for
the pixel endpoint) and returns the throttle marker, which still drives
the SKIPPED events-log status, the `throttled` metric and the
dead-letter copy. In the batch handler a throttled event counts toward
okEvents instead of failing the batch. The classic handler's trailing
success write is now guarded by !Written() — gin appends body writes
rather than guarding them, so this also fixes a pre-existing double-body
write on every sendToRotor error path there.
2. IngestHandlerRequests now labels `slug` with metricsId (the resolved
stream id) instead of DefaultString(loc.Slug, loc.Domain), which was
ambiguously a slug or a domain. Throttled counts are always
post-resolution, so blocked-event volume can now be read per stream from
bulkerapp_handler_ingest{status="throttled"} and mapped to a workspace.
NOTE: this changes the `slug` label across all ingest statuses (success
and error too) — Grafana panels keying on the old domain values need a
look before deploy.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![jitsu-code-review[bot]](https://avatars.githubusercontent.com/in/3391383?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the ingest throttling and metrics-label changes across router handlers. One potential correctness regression noted inline: throttled non-pixel requests now return JSON {"ok":true} unconditionally, which can break clients expecting endpoint-specific 200 response bodies (for example plaintext/gif-style beacons or CORS preflight-sensitive integrations). Please confirm this intentional behavior across all ingest endpoints.
| if c.FullPath() == "/api/px/:tp" { | ||
| c.Data(http.StatusOK, "image/gif", appbase.EmptyGif) | ||
| } else { | ||
| c.JSON(http.StatusOK, gin.H{"ok": true}) |
There was a problem hiding this comment.
Potential regression: this now returns {"ok":true} for every non-pixel throttled endpoint. Previously throttled responses were produced by ResponseError(...), which may have preserved endpoint-specific response shapes/headers. Could this break clients that parse expected 200 bodies for classic/batch/ingest variants? If silent-success is required, would preserving each handler’s normal success response format be safer?
|
Added
The flag gates both the single-event response ( This gives us a per-deployment escape hatch without another code change if we decide 402 is preferable after watching the silent rollout. |
| if c.FullPath() == "/api/px/:tp" { | ||
| c.Data(http.StatusOK, "image/gif", appbase.EmptyGif) | ||
| } else { | ||
| c.JSON(http.StatusOK, gin.H{"ok": true}) |
There was a problem hiding this comment.
Potential bug: for non-pixel endpoints this throttle branch now always writes {"ok":true} when ERROR_ON_THROTTLE=false. In ClassicHandler, successful responses include destination lists (async, tag) when available, and some clients may rely on those fields. Returning a different success schema only for throttled requests can cause subtle client-side regressions. Could we preserve each endpoint’s normal success payload (or at least Classic’s shape) while still treating the event as silently skipped?
… response Adds ERROR_ON_THROTTLE, default true — preserves the pre-JITSU-88 behavior where a fully-throttled (quota-blocked) event is rejected with HTTP 402. Merging this PR therefore changes no default behavior; the silent block from the previous commit becomes opt-in (ERROR_ON_THROTTLE=false): the event is accepted (HTTP 200) and preserved in backup but not delivered to destinations. The flag gates both the single-event response (sendToRotor) and the batch aggregate (throttled events are errors when erroring, ok when silent). Internal accounting — SKIPPED events-log status, `throttled` metric, dead-letter copy, backup — is identical in both modes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
66f114d to
f9d0b4e
Compare
|
Correction: |
There was a problem hiding this comment.
Reviewed the ingest throttling and metrics-label changes across , , and handler call sites in this PR range ().\n\nI focused on correctness, user-visible behavior, and security-sensitive paths (throttle branching, response writing, and metrics attribution).\n\nNo additional actionable issues found beyond the existing discussion threads.
There was a problem hiding this comment.
Reviewed the ingest throttling and metrics-label changes across config.go, router.go, and handler call sites in this PR range (da3912b..f9d0b4e).
I focused on correctness, user-visible behavior, and security-sensitive paths (throttle branching, response writing, and metrics attribution).
No additional actionable issues found beyond the existing discussion threads.
1 participant
Bulker side of
JITSU-88(PR 2 of the plan). Prepares ingest for the usage-quota block, which works by settingthrottle=100on a workspace. Must be deployed before the billing cron enables the 105% block.1. Throttled events return a success response (not HTTP 402)
V1 blocking is silent — the client must see no ingestion error. Per the issue's V1 requirement ("stop sending event to destination and not have error on ingestion side") and the decision on the Linear thread.
sendToRotornow responds200 {"ok":true}(or the tracking gif for/api/px/:tp) on throttle instead of402 {"error":...}. The event is still preserved in backup (produced before the throttle check) and simply not delivered to destinations. The returned throttle marker is unchanged, so all internal accounting is preserved:bulkerapp_handler_ingest{status="throttled"}metricstatus: "SKIPPED"Batch handler: a throttled event counts toward
okEventsinstead of appending toerrors, so a blocked batch returnsok: true. Classic handler: the trailing{"ok":true}write is now guarded by!c.Writer.Written()— gin appends body writes rather than guarding them, so this also fixes a pre-existing double-body write ({"error":...}{"ok":true}) on everysendToRotorerror path there.2.
IngestHandlerRequestsslug label → stream idThe
sluglabel was filled withDefaultString(loc.Slug, loc.Domain)— ambiguously a slug or a domain. It now usesmetricsId, which is the resolvedstream.Stream.Id(falling back toUNKNOWNonly for pre-resolution errors). Throttle rejections are always post-resolution, so blocked-event volume can be read per stream frombulkerapp_handler_ingest{status="throttled"}and mapped to a workspace — this is how the billing cron computes{blocked_count}.sluglabel across all ingest statuses (success + error too), not just throttled. Grafana panels / alerts keying on the old domain-styleslugvalues need a look before this deploys.Testing
go build+go vetclean. No automated test added — the ingest package has no HTTP-handler test harness (existing tests cover only pure helpers like batch dedup), so a response-level test would need scaffolding that doesn't exist. Behavior verified by tracing all five handlers (ingest/s2s, pixel, classic return clean single success responses; batch reports success in the aggregate; funcs has no throttle path).Related
quotaStatusin/api/billing/settings+ console banners, legacyset-throttleremoval.🤖 Generated with Claude Code