feat(ingestion): add common ingestion pipeline builder prototype - #70800
Conversation
|
|
Reviews (1): Last reviewed commit: "chore(ingestion): handle batch hook side..." | Re-trigger Greptile |
jose-sequeira
left a comment
There was a problem hiding this comment.
Approved, overall change seems good to unify code. Think some improvements to testing across the pipelines can be made as well as other smallr call outs
| @@ -0,0 +1,489 @@ | |||
| import { Message } from 'node-rdkafka' | |||
There was a problem hiding this comment.
No tests for the massive class we added. We should add a test file for it as most pipelines do
| * committed chunk-level transform. The block's start type stays existential — | ||
| * it lives only in the closures — so stages don't carry it as a type parameter. | ||
| */ | ||
| interface PreTeamChain<TInput, TContext, ROut extends string, CBatch, TCurrent> { |
There was a problem hiding this comment.
The two chain interfaces plus their four factory functions (pendingPreTeamChain/committedPreTeamChain and pendingTeamChain/committedTeamChain) implement the exact same pending/committed coalescing logic which leads to a good amount of duplicated code with only the type parameters changed (BatchContext<TContext> vs TeamAwareContext<TContext>, SubpipelineTransform vs TeamSubpipelineTransform).
Could we generalize and then provide type aliases?
Chain<TBuilderContext, TTransform> and then alias based on parameters?
| import { newBatchingPipeline } from '~/ingestion/framework/builders' | ||
| import { PipelineConfig } from '~/ingestion/framework/result-handling-pipeline' | ||
|
|
||
| export interface ClientWarningsPipelineConfig { |
There was a problem hiding this comment.
consumer.test.ts in clientwarnings mocks this entire file, so none of this gets tested
The common builder's pipelines handle their own side effects via handleSideEffects, so BatchResult.sideEffects is always empty for drivers. Remove the misleading scheduling loops and just drain results.
Covers the builder's composition behavior with mock steps: sequential block coalescing (pre-team and team-aware), pipeChunk closing blocks, team resolution drops, DLQ/redirect/warning routing, side effects handled inside the pipeline (scheduled or awaited inline), batch hook context and ordering, and concurrentlyPerGroup with gather.
…ivers Pipeline test drivers used to mirror the consumers and schedule BatchResult.sideEffects; now that the pipelines handle their own side effects, the drivers assert every drained batch surfaces none. The batching-pipeline integration harness gains the hook-level handleSideEffects the real joined pipeline now has.
…peline Drives the real pipeline (previously only exercised through a consumer test that mocks it): $$client_ingestion_warning events emit a warning for the resolved team, other events DLQ, and no side effects leak to the driver.
The pre-team and team-aware phases duplicated the pending/committed chain logic with only the start type and element context changed. Collapse them into one generic Chain with two factories; the phase names remain as type aliases over their instantiations.
…lder The builder's stage contract is enforced by the type system (no team-dependent steps before resolveTeam, no body steps before parseMessage, redirects limited to declared outputs). Guard it with @ts-expect-error assertions so a refactor that loosens the generics fails the typecheck instead of compiling silently.
…n harness The harness intentionally mirrors driver-side side effect scheduling; keep it as it was.
… rely on
The joined pipeline uses compose and multi-step concurrentlyPerGroup
subpipelines, error tracking uses resolveTeam({ wrap }) and chunk-step
retries. Extend the builder tests to lock those in: cross-group
concurrency (gated, deadlocks on regression), sequential multi-step
processing within a group, compose stage boundaries, the wrap decorator
replacing the resolve step, and retry options reaching chunk steps.
The sequential-block and chunk-barrier tests relied on microtask ordering of synchronous steps. Hold one element inside a step with a deferred gate and assert nothing else ran while it was held, so a regression to concurrent processing fails the negative assertion rather than depending on scheduler timing.
Fake every duration-bearing API so nothing depends on wall-clock time, keeping setImmediate real as the settle primitive (bare microtask rounds advance chains one continuation per round and never reach the park point). The fake clock strengthens two tests: awaitSideEffects now proves the drain is blocked until the parked effect fires, and the retry test proves the second attempt waits for a production-sized backoff that elapses instantly.
Problem
Every Kafka-fed ingestion pipeline in nodejs (analytics, AI, error tracking, heatmaps, client warnings, session replay) hand-assembles the same skeleton: beforeBatch hooks, messageAware, header parsing, pre-parse restrictions, body parse + team resolution, lifting the team into context, a teamAware processing block, ingestion warning handling, result handling, side effect handling, afterBatch flush hooks. Only the middle differs per product. The boilerplate is duplicated in every pipeline, the invariants (warnings before results, side effects last) rely on convention, and the deep callback nesting makes it hard to see at a glance what a pipeline actually does.
Changes
Adds a common ingestion pipeline builder (
nodejs/src/ingestion/common/common-ingestion-pipeline.ts) that owns the skeleton and reads as a flat recipe. Phase markers carry the structure; there is no user-side nesting except batch hooks and grouping:Semantics:
.pipe()calls coalesce into one sequential per-element block; any chunk-level call (.pipeChunk,.gather,.concurrentlyPerGroup,.compose) closes the block. Block boundaries fall out of where the chunk operations sit, which is how the hand-written pipelines were already shaped..parseHeaders()/.parseMessage()/.resolveTeam()pipe the shared steps..resolveTeam()also lifts the team into the element context and opens the team-aware scope: everything after it runs insidehandleIngestionWarnings. It takes an optionalwrapto decorate the resolution step (error tracking wraps it in topHog metrics)..build()wraps the whole thing inmessageAwareand bundleshandleResults,handleSideEffects, and the batching hooks. The built pipeline owns all of its side effects: the beforeBatch/afterBatch hook pipelines are wrapped in side-effect handling too (via a new single-itemSideEffectHandlingProcessorandPipelineBuilder.handleSideEffects, mirroring the chunk-level one), soBatchResult.sideEffectsalways comes out empty and drivers only drain results. AnawaitSideEffectsconfig flag (default false, i.e. schedule on the promise scheduler) applies to element results and hooks alike..parseMessage()), and redirect kinds are declared up front via theROuttype parameter so the outputs must cover every redirect a step can produce..concurrentlyPerGroupmirrors the framework method (within-group ordering stays visible at the call site), and.compose(fn)is the escape hatch for existing subpipeline functions likecreatePostTeamPreprocessingSubpipeline, whose generics are widened to accept mid-chain builders.any; the coalescing block's start type stays existential inside closures.Five pipelines are rewired onto the builder: client warnings, heatmaps, joined analytics (keeps its per-distinct-id
concurrentlyPerGroupand post-team subpipeline), AI (keeps its topHog metric wrappers and per-step retries), and error tracking. Two mechanical enablers rode along: the schema-enforcement toggle moved intocreateValidateEventSchemaStep(a no-op step when disabled beats conditional chain composition), and the post-team subpipeline generics were widened.Note
Error tracking previously built a bare chunk pipeline driven through
ChunkPipelineUnwrapper; it is now aBatchingPipelinelike the other consumers, with passthrough batch hooks andconcurrentBatches: 1, and its custom team-liftfilterMapbecame a plainmessageBytesstep. ItscreateErrorTrackingPipeline/runErrorTrackingPipelinesignatures are unchanged, so the existing suites verify the conversion.Note
Sequential-block boundaries shift slightly versus the hand-written versions (e.g. post-team-resolution validation now runs inside the team-aware scope, so its warnings route per-team even for dropped elements). Per-element step order is unchanged everywhere; the end-to-end suites pass unchanged, including all 22 ingestion-consumer snapshots.
Session replay is the one pipeline left (custom team resolution via
TeamServicerather thanTeamManager); a doc-test chapter for the builder is also still pending.How did you test this code?
Automated only, no manual testing:
pnpm build(tsc across nodejs) and eslint on the touched filessrc/ingestion/ingestion-consumer.test.ts(57 tests, 22 snapshots, end-to-end joined pipeline),src/ingestion/pipelines/ai/(806 tests),src/ingestion/pipelines/errortracking/(146 tests, including the end-to-end pipeline and consumer suites, driven through the unchangedcreateErrorTrackingPipeline/runErrorTrackingPipelineAPI),src/ingestion/pipelines/heatmaps/(42 tests),src/ingestion/pipelines/clientwarnings/validate-event-schema.test.ts: a step built withenabled: falsepasses through an event that violates an enforced schema — guards the flag plumbing, which no existing test covered; the enabled-path cases already existedAutomatic notifications
Docs update
None yet, prototype. The framework's doc-test chapters (
nodejs/src/ingestion/framework/docs/) get a builder chapter if this graduates.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Built with Claude Code. Skills used:
ingestion-pipeline-doctor-nodejs,writing-tests. The API went through three shapes on this branch: staged callbacks per phase (preParse/resolveTeam/perTeam), then a collapse of the post-resolution stage, then the current flat recipe — the flat form was chosen because the linear spine is the point of the template and the callback nesting added no information. Key implementation decisions: sequential-block coalescing with the block's start type kept existential inside closures (cast-free),ROutfixed at the factory so redirect coverage is checked where outputs are supplied, the post-team subpipeline generics widened instead of adding a special builder hook, and error tracking converted to aBatchingPipeline(rather than giving the builder a second, unwrapped build path) to keep all consumers on one pipeline shape.