Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Focused bug fix that gives traces ingestion its own Redis key namespace for rate limiting so token buckets don't bleed across ingestion types. Code is clean, logic is sound, and new tests verify the isolation. No production risk beyond the intentional behavioral change (which is the fix).
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
nodejs/src/logs-ingestion/services/logs-rate-limiter.service.test.ts:834-845
**Prefer a parameterised test for key-namespacing cases**
These two `it` blocks are the same assertion structure with different inputs (`limiterName` and expected prefix). The team prefers parameterised tests; a single `it.each` would express the same coverage more concisely and make it easy to add a third limiter in the future without duplicating the scaffold.
Reviews (1): Last reviewed commit: "fix(traces): give traces ingestion its o..." | Re-trigger Greptile |
8ba976e to
b92fb51
Compare
New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.
There was a problem hiding this comment.
Clean bug fix that isolates traces rate-limiter Redis key namespace from logs, with a straightforward mergedConfig refactor and new tests covering both the namespace isolation and config wiring. The one bot comment about parameterized tests was already addressed in the current diff.
b92fb51 to
99b0537
Compare
99b0537 to
aeb697a
Compare
|
Retaining stamphog approval — delta since last review classified as |

Problem
The
TracesIngestionConsumerextendsLogsIngestionConsumerand reuses its token-bucket rate limiter infrastructure. Before this change, both consumers wrote their per-team rate-limit state to the same Redis key namespace (@posthog/logs-rate-limiter). This meant traces traffic could deplete a team's logs rate-limit bucket (and vice versa), causing incorrect throttling across two unrelated ingestion pipelines.Additionally, the
LogsIngestionConsumerconstructor was applying overrides inconsistently — some fields usedoverrides.X ?? config.Xand others read directly fromconfig, making it easy to silently miss an override (which is exactly how the rate limiter ended up ignoring the traces-specific config values).Changes
LogsRateLimiterServicenow accepts an optionallimiterNameparameter. The Redis key prefix is derived from this name via a newbuildBaseRedisKeyhelper, so logs and traces get isolated namespaces (logs-rate-limitervstraces-rate-limiter).LogsIngestionConsumermergesconfigandoverridesinto a singlemergedConfigobject at construction time, eliminating the scatteredoverrides.X ?? config.Xpattern and ensuring all downstream services (including the rate limiter) see the correct effective config.TracesIngestionConsumerpasses'traces-rate-limiter'as the limiter name tosuper(...), giving it its own isolated per-team token buckets.How did you test this code?
New automated tests cover:
LogsRateLimiterServicekey namespacing: verifies the default prefix islogs-rate-limiterand that passing'traces-rate-limiter'produces a distinct, non-overlapping prefix.TracesIngestionConsumerrate limiter isolation: verifies the consumer's rate limiter uses thetraces-rate-limiterkey namespace and thatTRACES_LIMITER_*config values are correctly forwarded through the merged config to the rate limiter.Automatic notifications
Docs update