feat(devtools): declarative queue tuning + make the DLQ alarm capable of firing - #633
Draft
d-klotz wants to merge 5 commits into
Draft
feat(devtools): declarative queue tuning + make the DLQ alarm capable of firing#633d-klotz wants to merge 5 commits into
d-klotz wants to merge 5 commits into
Conversation
Adds an optional `Definition.queue` block on each integration that lets
consumers express SQS + queue-worker tuning declaratively in their app
definition, instead of patching it in via post-deploy
`aws lambda update-event-source-mapping` / `aws sqs set-queue-attributes`
scripts.
Knobs (all optional, defaults preserved):
queue.visibilityTimeout SQS VisibilityTimeout [0..43200] default 1800
queue.messageRetentionPeriod SQS MessageRetentionPeriod [60..1209600] default 345600
queue.maxReceiveCount DLQ redrive maxReceiveCount [1..1000] default 3
queue.worker.batchSize SQS event batchSize [1..10000] default 1
queue.worker.maximumBatchingWindow SQS MaximumBatchingWindowInSeconds [0..300] default unset
queue.worker.maximumConcurrency ESM ScalingConfig.MaximumConcurrency [2..1000] default unset
queue.worker.reservedConcurrency Lambda ReservedConcurrency [0..1000] default 20
queue.worker.timeout Lambda execution timeout (s) [1..900] default 900
Implementation notes:
- All knobs optional; falls through to current defaults via `??`. Apps
that don't set `Definition.queue` produce identical output (verified
by a backward-compat test).
- maximumBatchingWindow / maximumConcurrency only emitted to the
serverless template when defined — keeps the rendered template stable
for non-opt-in consumers.
- Uses the serverless-framework input key `maximumBatchingWindow`, NOT
the longer CFN property name `maximumBatchingWindowInSeconds`. osls
and serverless@3+ reject the longer key with a schema error.
- Bounds-checked at template-generation time (consistent with
aurora-builder's `minCapacity`/`maxCapacity` validation), so out-of-range
values surface as clear errors instead of opaque CloudFormation
rejections post-package.
Downstream prerequisite for `worker.batchSize > 1`: the queue worker
handler must return `{ batchItemFailures: [...] }` for partial-batch
failures (the framework already wires `functionResponseType:
'ReportBatchItemFailures'` — same as before). With batchSize=1 this
was moot; with batchSize>1 it isn't.
Tests: 97 pass (89 existing + 8 new covering override paths, empty-config
edge cases, conditional-spread invariants, and bounds-check failures).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The generated DLQMessageAlarm watched ApproximateNumberOfMessagesVisible with Threshold 500, while createDLQObservability also attaches dlqProcessor as an SQS consumer that drains the same queue continuously. Depth therefore never accumulates and the alarm could never fire, despite its comment claiming it "fires when any message lands in the DLQ". Observed in a production Frigg app: 146 messages landed in the DLQ in one day while ApproximateNumberOfMessages read 0 throughout, and no alarm fired. Every Frigg app with a DLQ has the same blind spot. Alarm on arrival instead. The metric is NumberOfMessagesDeleted rather than NumberOfMessagesSent because SQS does not increment Sent for messages arriving via redrive — verified in production, where Sent read 0 for seven consecutive days while Received and Deleted tracked the real inflow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for friggframework-org canceled.
|
NumberOfMessagesDeleted only increments while dlqProcessor is healthy. If it is throttled or erroring — plausible at reservedConcurrency 1 under a burst — messages accumulate in the DLQ and the arrival alarm stays silent, which is the worst case to miss. Add DLQBacklogAlarm on ApproximateNumberOfMessagesVisible > 0. That is also the metric AWS recommends for DLQs; their guidance assumes nothing consumes the queue, which is why it needs pairing with an arrival alarm here rather than replacing it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…, silent knobs Three fixes from review: DLQBacklogAlarm used Statistic Maximum, which breaches on any burst larger than one dlqProcessor batch (concurrency 1, batchSize 10). It would have fired alongside DLQMessageAlarm on every ordinary burst, training people to ignore the alarm that means "consumer stalled". Minimum only breaches when the queue was never empty across the period. SQS event sources reject batchSize > 10 unless a batching window is set. The validator allowed it, so the template packaged fine and failed at deploy — exactly what the validator exists to prevent. Queue-level knobs silently did nothing when the queue is externally owned, while queue.worker.* still applied. Not mutating a queue we do not own is correct, but dropping declared config without a word is the worst failure mode for a tuning API. Warn, and run the same bounds checks on that path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
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.



Two related changes to
integration-builder, both driven by a production incident in a Frigg app whose Attio integration queue reached 268k messages.1.
Definition.queuetuning knobsSQS and queue-worker settings were hardcoded, so no app could override them:
maxReceiveCount: 3,VisibilityTimeout: 1800,reservedConcurrency: 20.Apps that needed different values patched them in post-deploy with
aws lambda put-function-concurrency/aws sqs set-queue-attributes. That drifts from the CloudFormation template, and the drift is invisible until something causes CFN to update the resource — at which point it silently reverts.We hit exactly that: a production app's template says
ReservedConcurrentExecutions: 20while the live function runs 50, and the same app's Aurora template saysMaxCapacity: 1while the cluster runs 5. Both survive only because CFN acts on template diffs, not on drift. The next change touching either resource reverts it.This exposes an optional
Definition.queueblock. All knobs optional, all existing defaults preserved via??, so apps that don't opt in render byte-identical templates (covered by a backward-compat test).queue.visibilityTimeoutVisibilityTimeoutqueue.messageRetentionPeriodMessageRetentionPeriodqueue.maxReceiveCountmaxReceiveCountqueue.worker.batchSizebatchSizequeue.worker.maximumBatchingWindowqueue.worker.maximumConcurrencyScalingConfig.MaximumConcurrencyqueue.worker.reservedConcurrencyReservedConcurrencyqueue.worker.timeoutBounds-checked at template-generation time (consistent with
aurora-builder's validation) so mistakes surface as clear errors rather than opaque CloudFormation rejections after packaging. This includes the cross-field rule that SQS rejectsbatchSize > 10without a batching window — otherwise the template packages fine and fails at deploy.maximumBatchingWindow/maximumConcurrencyare emitted only when defined, keeping rendered templates stable for non-opt-in consumers. The serverless-framework input key ismaximumBatchingWindow, not the longer CFN namemaximumBatchingWindowInSeconds— osls and serverless@3+ reject the longer key.External queues: queue-level knobs apply only to a queue this stack owns. When the queue is externally owned we must not mutate it, but
queue.worker.*still applies — an asymmetry that would otherwise be silent. That path now warns which keys were ignored and runs the same bounds checks.This half is a cherry-pick of a previously unmerged local commit, rebased onto
next. One conflict resolved:nexthas since added anappDefinitionparameter tocreateIntegrationQueue, so the signature is now(integrationName, result, appDefinition, queueConfig = {})rather than either version alone.2. The DLQ alarm could never fire
createDLQObservabilitygenerated this:The comment states the intent correctly and the code does something else. It alarms on standing depth, and the same function attaches
dlqProcessoras an SQS consumer (batchSize: 10) that drains that queue continuously. Depth never accumulates, so the threshold is unreachable.To be fair to the original: AWS explicitly recommends
ApproximateNumberOfMessagesVisiblefor DLQs. That guidance assumes the usual shape, where nothing consumes the DLQ. Frigg's does, which quietly invalidates it.Observed in production: 146 messages landed in the DLQ in a single day while
ApproximateNumberOfMessagesread 0 throughout and no alarm fired. That is why several days of silent data loss in that app went unnoticed. Every Frigg app with a DLQ has the same blind spot.Now two alarms, because a consumed DLQ needs both an arrival signal and a backlog signal:
DLQMessageAlarm— arrivals.NumberOfMessagesDeleted/Sum/> 0/TreatMissingData: notBreaching.Not
NumberOfMessagesSent, which would be just as blind. Per the SQS docs: "Messages automatically moved to a DLQ due to processing failures are not captured by this metric." Redrive is the only way messages reach a DLQ. Confirmed in production —Sentread 0 for seven consecutive days whileReceived/Deletedtracked the real inflow of 4 → 55 → 146.DLQBacklogAlarm— backlog.ApproximateNumberOfMessagesVisible/Minimum/> 0.Deletedonly increments whiledlqProcessoris healthy; if it is throttled or erroring, messages pile up and the arrival alarm stays silent — the worst case to miss.Minimumrather thanMaximumbecausedlqProcessorruns at concurrency 1 withbatchSize: 10, so any larger burst leaves depth briefly above zero;Maximumwould fire on every ordinary burst alongsideDLQMessageAlarmand train people to ignore it.Minimum > 0breaches only when the queue was never empty across the period — genuinely not draining.Deliberately not included
Raising the
maxReceiveCountdefault from 3 to 5. The production evidence supports it — DLQ inflow tracked a rate-limit storm exactly (4 → 55 → 146 → 22), the signature of retries exhausting during sustained 429s rather than a code defect. But that is policy, not mechanism: it would change behaviour for every existing Frigg app on upgrade. With the knob available, apps can set it themselves. This PR adds mechanism, fixes a bug, and changes no defaults.Testing
integration-buildersuites: 591 passing, 11 suites.Whole
packages/devtools/infrastructuretree, same environment, worktrees excluded:nextbaselineSame 18 pre-existing failures (health, networking, cloudformation-discovery, prisma-layer — all unrelated), no new ones.
integration-builder.jsis prettier-clean.integration-builder.test.jsandapp-definition.jswere already prettier-unclean onnext; deliberately not reformatted, to keep the diff free of unrelated churn.Known gaps, not addressed here
visibilityTimeout: 1.5passes bounds and CFN rejects it later.maxRecieveCounttypo is silently ignored, which is the same silent-no-op failure mode this PR fixes elsewhere. Worth a follow-up.queue.visibilityTimeout < worker.timeoutcauses in-flight redelivery and duplicate processing. Both are now user-settable independently, so this deserves a warning.InternalErrorQueue, so there is no per-integration attribution. Pre-existing, unchanged.