fix(aws sinks): ensure component_sent_bytes_total carries component and region labels#25915
fix(aws sinks): ensure component_sent_bytes_total carries component and region labels#25915Clee2691 wants to merge 4 commits into
Conversation
github-actions
Bot
added
the
domain: sinks
d109fe6 to
aecbef0
Compare
|
FYI: Had to force push for a rebase |
![chatgpt-codex-connector[bot]](https://cdn.github.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: d109fe6c24
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| client.send(records, stream_name).await.map(|mut r| { | ||
| // augment the response | ||
| r.events_byte_size = events_byte_size; | ||
| r.byte_size = byte_size; |
There was a problem hiding this comment.
With request_retry_partial enabled, Kinesis retry logic can retry an HTTP 200 response that contained failed records; Tower discards that first successful response and only returns the final retry response to the Driver. Since this commit disables transport-level byte metrics and records only the last response's byte_size, component_sent_bytes_total misses bytes for the initial successful attempt (or all but the last partial retry). Please accumulate bytes across retry attempts or keep transport measurement for these retrying Kinesis services.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
With request_retry_partial enabled on Kinesis Streams, Tower discards intermediate responses internally, so the Driver only sees the final retry response's byte_size and bytes from records that succeeded on earlier attempts go unrecorded.
Worth noting this class of problem also existed with the previous transport-level metrics, just in the opposite direction — AwsBytesSent was emitted for every HTTP request body, so the failed records' bytes were counted both in the original request and again in the retry, resulting in over-counting.
The fix is straightforward: carry an accumulated_bytes field on BatchKinesisRequest, compute the succeeded portion in the RetryPartial closure (original_encoded_size - retry_encoded_size), and have KinesisResponse::bytes_sent() return self.byte_size + self.accumulated_bytes. This stays localized to the Kinesis sink code with no changes to the retry infrastructure.
Since request_retry_partial defaults to false and only Kinesis Streams uses RetryPartial (Firehose does full retries), this is a narrow edge case that was already inaccurate before this PR. This could be done in a follow-up PR.
aecbef0 to
e5ead33
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5ead33b74
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| svc.oneshot(events) | ||
| .map_ok(move |_x| CloudwatchResponse { events_byte_size }) | ||
| .map_ok(move |_x| CloudwatchResponse { | ||
| events_byte_size, | ||
| byte_size, | ||
| }) |
There was a problem hiding this comment.
When a CloudWatch batch spans more than 24 hours, CloudwatchLogsSvc::process_events splits this one Driver request into multiple PutLogEvents calls, and the retry layer wraps the whole Vec<InputLogEvent>. If an early inner PutLogEvents succeeds but a later split fails or causes the whole request to be retried, this only returns a byte count after the final oneshot succeeds and only once, so bytes from successful inner attempts are no longer counted now that transport-level metrics are disabled. Track successful PutLogEvents byte counts inside the CloudWatch future or accumulate them across retries before reporting through the Driver.
Useful? React with 👍 / 👎.
1 participant
Summary
AwsBytesSentfor all Driver-based AWS sinks to eliminate duplicatecomponent_sent_bytes_totalmetrics missing component labels.label(key, value)builder onDriver; each AWS sink uses it to add aregionlabelVector configuration
Any AWS sink config
How did you test this PR?
Change Type
Is this a breaking change?
Does this PR include user facing changes?
no-changeloglabel to this PR.References
component_sent_bytes_totalbut not incomponent_sent_event_bytes_total#20356Notes
@vectordotdev/vectorto reach out to us regarding this PR.pre-pushhook, please see this template.make fmtmake check-clippy(if there are failures it's possible some of them can be fixed withmake clippy-fix)make testgit merge origin masterandgit push.Cargo.lock), pleaserun
make build-licensesto regenerate the license inventory and commit the changes (if any). More details on the dd-rust-license-tool.