Repository navigation
feat(metrics): flush pending batch on metrics ingester shutdown - #1004
Merged
AntonTyb merged 5 commits intoOct 23, 2026
Merged
Conversation
This was referenced Sep 28, 2026
There was a problem hiding this comment.
Code Review
This pull request updates the metrics ingester to flush pending messages on shutdown instead of nacking them, ensuring at-least-once delivery during termination. It introduces a bounded shutdown flush timeout, drains buffered messages non-blockingly, and updates the documentation and tests accordingly. The reviewer suggested an optimization to return early in flushOnShutdown if there are no pending messages to avoid the overhead of creating a new context.
pachopepe
reviewed
Sep 30, 2026
andrzejfudro
previously approved these changes
Oct 22, 2026
Collaborator
There was a problem hiding this comment.
LGTM but please consider addressing comments left by pachopepe marked as medium.
On SIGINT/SIGTERM the ingester used to nack its pending messages for redelivery. It now writes them in a final batch, bounded to 10s so shutdown stays inside the default Kubernetes grace period. Messages already buffered when shutdown begins are included in that batch. Messages that arrive after it are still nacked. Document the ingester, and point the ingestion gateway README at it instead of saying the Pub/Sub path has no consumer. TAG=agy CONV=c72e85d4-da3a-4185-b361-f01b12d0e3b4
…k settlement Route all batchLoop cancellation paths through a single shutdown helper that drains buffered messages and flushes them in batchSize chunks sharing a bounded 10s context detached from Run's signal cancellation. Update fakeWriter in ingester_test.go to propagate ctx.Err() and record per-call context deadlines so shutdown and in-flight write context guarantees are verified non-vacuously, and drive post-drain message arrival through pstest to assert explicit Nack settlement without Ack. Synchronize cmd/metrics_ingester/README.md for shutdown drain/chunking, terminationGracePeriodSeconds (>15s) timing budget, subscription RetryPolicy recommendation, unbounded malformed payload drops, and target -batch_interval wait semantics. Comment link: #1004 (comment) Comment link: #1004 (comment) Comment link: #1004 (comment) Comment link: #1004 (comment) Comment link: #1004 (comment) Comment link: #1004 (comment) TAG=agy CONV=0fafd02b-f3b1-4563-a73d-49ec0845afbd Change-Id: Ia80f0878c92a0c928233c40a1e60628b66bcdb0e
AntonTyb
force-pushed
the
feat/pubsub-ingester-hardening
branch
from
October 22, 2026 14:26
8869d4d to
50a4225
Compare
… separation Merge updated feat/pubsub-ingester-config and add a brief rationale in cmd/metrics_ingester/README.md explaining why metrics_ingester runs as a separate service from ingestion_gateway (ClickHouse credential/egress isolation and independent batch-insert replica scaling). Comment link: #1002 (comment) TAG=agy CONV=0fafd02b-f3b1-4563-a73d-49ec0845afbd Change-Id: I751d6c78cf1088313833074a4f95fcd0a5c7df2e
andrzejfudro
previously approved these changes
Oct 23, 2026
…tal docs Merge updated feat/pubsub-ingester-config (caa080a) into #1004, retaining #1004's shutdown-flush README details and referencing the dropped_total log field in place of the removed DroppedMessages getter. Comment link: #1002 (comment) Comment link: #1002 (comment) TAG=agy CONV=0fafd02b-f3b1-4563-a73d-49ec0845afbd Change-Id: I3ea0c7f2900179baeb5927323bb3b619331f87c5
There was a problem hiding this comment.
🤖 Incremental PR Approval
Clean rebase detected with zero semantic patch lines changed.
Preserving prior approval by @andrzejfudro on commit 128a02c.
There was a problem hiding this comment.
🤖 Incremental PR Approval
Clean rebase detected with zero semantic patch lines changed. PR touches protected files, but the approved patches are unchanged.
Preserving prior approval by @andrzejfudro on commit 128a02c.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
On SIGINT/SIGTERM the metrics ingester now writes its pending batch instead of nacking it. This PR also documents the service and updates the Ingestion Gateway README, which said the Pub/Sub path had no consumer.
Motivation & Context
Nacking on shutdown is correct but wasteful: every rollout makes Pub/Sub redeliver up to a batch of messages per pod. Writing the pending batch makes shutdown cheap.
Pub/Sub's
Receivedoesn't return until every delivered message has been settled. So the batch loop has to act on context cancellation itself, not wait for the channel to close.Key Changes
ingester.go:ctx.Done,drainBufferedpulls the messages already buffered by receive callbacks into the pending batch without blocking.flushOnShutdownthen writes that batch on a context detached from the cancelled one. The write is bounded byshutdownFlushTimeout = 10s, well inside the default 30s termination grace period.drainBufferedcmd/metrics_ingester/README.md: data flow, delivery semantics, configuration, authentication, probes and testing.cmd/ingestion_gateway/README.md: the "bring your own subscriber" warning is replaced with a pointer to the ingester.Assumptions
Verification & Testing
go build,go vet, andgo test -race -count=30 -coveron the package../.githooks/pre-commitpasses.Related Issues / Bugs
N/A
🤖 Generated with Claude Code
TAG=agy
CONV=c72e85d4-da3a-4185-b361-f01b12d0e3b4