Repository navigation
Conversation
Request metrics are opt-in. A bucket with one FilterId is measured into monthly_tier_1_requests and monthly_tier_2_requests; none or several is an error, and standard_storage_gb is kept. The count is the window total expressed per 30 days. Signed-off-by: Atsushi Sakashita <ats@saks.industries>
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
andreacappelletti97
left a comment
andreacappelletti97
left a comment
There was a problem hiding this comment.
Thanks, this is careful work. Collecting storage and requests separately, finding the FilterId instead of assuming it, and refusing to add filters together are all right. Local checks: go test -race on usagesync, cmd/c3x and usage, plus golangci-lint v2.15.0 on Go 1.27.3, are clean.
One change is needed before merge, plus two suggestions.
1. Required: DELETE requests aren't billable. S3 pricing says "DELETE and CANCEL requests are free", but DeleteRequests is currently added to tier 2, which overstates monthly_tier_2_requests.
- Keep it among the named metrics, so it's still subtracted from
AllRequests. - Don't add it to either tier: for example, give it its own tier value that only joins
explicit. - The fixture in
s3_requests_test.go(DeleteRequests: 5) should then expect tier 2 to be 5 lower.
2. Suggestion: a filter that covers only part of the bucket. If a bucket's only request-metrics configuration is scoped to a prefix, tag or access point, s3RequestFilter accepts it as the whole bucket and undercounts. CloudWatch can't show the scope, and reading the configuration needs the S3 API, which depguard keeps out of this package.
- Please at least record the
FilterIdused. - For example, put it in the per-resource note (
measured with request-metrics filter "X"), so someone reading the file can tellEntireBucketfromimages-only.
3. Question for discussion: --strict. Most buckets don't have request metrics, so most real runs will now end with every bucket partial, and --strict fails.
- That matches the design's rule, but it makes
--strictclose to unusable in practice. - I'd lean towards treating "this bucket has no request-metrics configuration" as an expected gap: it's still listed and the key stays unset, but it doesn't fail
--strict. A real failure (AccessDenied, throttling, several filters) still would. - What do you think? This can be a follow-up rather than part of this PR.
Your open questions
- An account-level Cost Explorer total: not on a resource address, since estimates can't use an account-wide number. Either record it in
syncedas information only, or leave the fallback out for now. I'd rather leave it out than have a value nothing reads. GetCostAndUsageWithResources: only behind an explicit opt-in flag. It needs resource-level data turned on in billing, covers only the last 14 days, and each call is billed. PlainGetCostAndUsageis the default.
Happy to merge once #1 is in. #2 would be nice in the same PR, and #3 can be a follow-up. Stage 3 (Lambda and NAT) after that sounds good.
| {"get", "GetRequests", 2}, | ||
| {"select", "SelectRequests", 2}, | ||
| {"head", "HeadRequests", 2}, | ||
| {"delete", "DeleteRequests", 2}, |
There was a problem hiding this comment.
DELETE is free in S3 pricing. Keep this metric so it's subtracted from AllRequests, but don't add it to tier 2.
| switch len(ids) { | ||
| case 0: | ||
| return "", errors.New("no S3 request metrics for this bucket; CloudWatch publishes them only for a bucket with a request-metrics configuration, which is opt-in") | ||
| case 1: |
There was a problem hiding this comment.
A single filter may be scoped to a prefix, tag or access point. Could the chosen FilterId be recorded in the result, so a partial-scope count can be spotted?
PR 2 of the staged plan in
docs/design/usage-sync.md(#97): S3 request counts from CloudWatch request metrics.What it measures
monthly_tier_1_requests=PutRequests+PostRequests+ListRequests.monthly_tier_2_requests=GetRequests+SelectRequests+HeadRequests+DeleteRequests, plus any positive gap toAllRequestsso "other" requests aren't dropped.FilterIdis discovered withListMetrics, never assumed. No request-metrics configuration, or more than one, goes undererrors. Filters are never summed.standard_storage_gb. The resource is reported aspartial, and--strictstill fails.synced, so I left that open (question below).Checked against the design
configandcloudwatchfrom the AWS SDK. The only new call is read-onlyListMetrics. depguard is unchanged.Emitsincludes the two request keys, and the catalog-key test covers them.Local checks:
go build,go vet,go test -race ./...and golangci-lint v2.15.0 are clean. In my fork I also ran this against an emulated CloudWatch (Floci) through the real AWS SDK client, and it passed. That test isn't part of this PR.Lambda and NAT (stage 3) are ready as a follow-up once this one lands.
Open questions for later stages
synced, since it can't sit on a single resource address?GetCostAndUsageWithResources(needs resource-level cost data enabled) acceptable, or onlyGetCostAndUsage?