enhancement(aws_cloudwatch_logs sink): stop calling DescribeLogStreams for sequence tokens#25788
Conversation
…s for sequence tokens CloudWatch Logs removed the PutLogEvents sequence-token requirement in Jan 2023 and never returns InvalidSequenceToken. The sink was calling DescribeLogStreams once per batch (and again on every retry) solely to fetch that token. DescribeLogStreams is throttled account+region-wide at a low default quota (25 TPS), so large fleets writing to many streams could saturate it and stall the sink via retry amplification. Write straight to PutLogEvents without a token, and create the group/stream lazily only when PutLogEvents returns ResourceNotFoundException. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Vincent Voyer <vincent.voyer@vercel.com>
|
All contributors have signed the CLA ✍️ ✅ |
This comment has been minimized.
This comment has been minimized.
|
I have read the CLA Document and I hereby sign the CLA |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2934e24919
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self.create_missing_group { | ||
| info!( | ||
| "Log group provided does not exist; creating a new one." | ||
| ); | ||
| self.state = | ||
| State::CreateGroup(self.client.create_log_group()); |
There was a problem hiding this comment.
When the log group already exists but the target stream is missing, PutLogEvents returns ResourceNotFoundException just like it does for a missing group. This branch always prefers create_missing_group, so with create_missing_group = true and create_missing_stream = false it goes through CreateLogGroup, treats ResourceAlreadyExists as success, and then creates the stream anyway. That bypasses the create_missing_stream setting documented in src/sinks/aws_cloudwatch_logs/config.rs:121-125 and can create streams users explicitly configured Vector not to create.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f87966b. The sink now writes straight to PutLogEvents and only falls back to a single DescribeLogStreams on ResourceNotFoundException. That describe distinguishes a missing group (DescribeLogStreams returns ResourceNotFoundException) from a missing stream (it returns the existing group with no matching stream). So with create_missing_group=true, create_missing_stream=false and an existing group, the stream-missing branch now returns NoStreamsFound instead of creating the stream. Thanks for the catch.
| if self.retention_enabled { | ||
| self.state = State::PutRetentionPolicy(self.client.put_retention_policy()); | ||
| self.state = | ||
| State::PutRetentionPolicy(self.client.put_retention_policy()); | ||
| continue; |
There was a problem hiding this comment.
When the group already exists but the stream is missing, the new PutLogEvents path still enters State::CreateGroup; after CreateLogGroup returns ResourceAlreadyExists, this block now calls PutRetentionPolicy if retention is enabled. That means Vector can overwrite retention on an existing group even though Retention::enabled is documented as applying when creating a new log group (src/sinks/aws_cloudwatch_logs/config.rs:45), whereas the previous describe-first flow only reached this code after an actual group creation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in f87966b. State::CreateGroup is now only entered from the DescribeLogStreams ResourceNotFoundException branch, i.e. when the group genuinely does not exist. When the group exists but the stream is missing, the describe returns success and we go to CreateStream, never CreateGroup. The ResourceAlreadyExists fall-through here only fires on a concurrent-create race, and applying retention in that case matches the pre-existing upstream behavior (the old describe-first flow also reached PutRetentionPolicy only after deciding to create the group). So retention is not applied to groups that already existed at describe time.
With no sequence token to fetch or pass, the oneshot channel between the service and its request future is dead weight. Remove it along with the token/token_rx fields and the _token param; concurrency_limit(1) already serializes per-stream writes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Vincent Voyer <vincent.voyer@vercel.com>
…ibe-log-streams' into fix/aws-cloudwatch-logs-drop-describe-log-streams
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f48c5b13a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self.create_missing_group { | ||
| info!( | ||
| "Log group provided does not exist; creating a new one." | ||
| ); | ||
| self.state = | ||
| State::CreateGroup(self.client.create_log_group()); |
There was a problem hiding this comment.
When both creation flags are left at their defaults and the log group already exists but a dynamically named stream is new, this branch tries CreateLogGroup before CreateLogStream for every new stream. That turns normal stream creation into account/region-wide CreateLogGroup traffic; if that low-TPS API throttles, CloudwatchRetryLogic does not retry CloudwatchError::CreateGroup, so startup batches for fleets creating many streams can fail instead of backing off and creating the stream. Prefer creating the stream first when stream creation is allowed, and only fall back to creating the group if stream creation reports the group is missing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f87966b. A new stream in an existing group no longer triggers CreateLogGroup: on ResourceNotFoundException we DescribeLogStreams first, which for an existing group returns success, so we go straight to CreateLogStream. CreateLogGroup runs only when the describe itself reports the group is missing, so new-stream traffic stays on CreateLogStream and off the low-TPS CreateLogGroup. Thanks.
…ibe on ResourceNotFound Addresses review feedback. A PutLogEvents ResourceNotFoundException cannot tell a missing group from a missing stream, so unconditionally creating the group (then the stream) violated create_missing_stream=false and applied retention to preexisting groups. Fall back to a single DescribeLogStreams on ResourceNotFound to distinguish the two, restoring the original create/retention/flag semantics. Normal writes still go straight to PutLogEvents; the describe only runs when a stream is genuinely missing, not once per batch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Vincent Voyer <vincent.voyer@vercel.com>
1 participant
Problem
aws_cloudwatch_logssink callsDescribeLogStreamsbefore every batch to fetch a sequence token, and again on every retry.DescribeLogStreamsis throttled account-and-region-wide at a low default quota (25 TPS), so a fleet writing to many streams saturates it whilePutLogEvents(5000 TPS) sits idle.DescribeLogStreamscalls and the sink stalls in a self-sustaining storm.Solution
PutLogEventswith no sequence token.PutLogEventsreturnsResourceNotFoundException, then replay the batch.Implementation details
PutLogEventssequence-token requirement in January 2023 and never returnsInvalidSequenceToken, so the pre-write token fetch was dead weight: https://aws.amazon.com/about-aws/whats-new/2023/01/amazon-cloudwatch-logs-log-stream-transaction-quota-sequencetoken-requirement/DescribeLogStreamsstays only as a fallback onResourceNotFoundException, to tell a missing group from a missing stream and keep the existing create/retention/create_missing_*semantics. The per-batch describe and the sequence-token plumbing (theoneshotchannel between the service and its request future) are gone.concurrency_limit(1)) is unchanged; it could be relaxed in a follow-up now that parallelPutLogEventson one stream is allowed.