Repository navigation
Commit 30e8912
authored
* feat(providers/aws): add purchasecfg helper to bound purchase-path SDK retries
Introduce providers/aws/internal/purchasecfg.NewConfig which returns a copy
of the caller's aws.Config with RetryMaxAttempts=2 and http.Client.Timeout=15s.
Worst-case wall clock: 2 * 15s = 30s, well inside the 300s Lambda budget.
Recommendation-collection clients (recommendations.NewClient) are explicitly
NOT touched; they call Cost Explorer and need the higher default retry budget.
Adds five unit tests covering: retry cap, HTTP timeout, immutability of the
base config, region preservation, and the wall-clock bound invariant.
Closes #683. Refs #667 #668 #632 #681.
* fix(purchases): wire purchasecfg into all 7 purchase-path SDK constructors
Switch EC2, RDS, ElastiCache, MemoryDB, OpenSearch, Redshift, and SavingsPlans
NewClient calls to use purchasecfg.NewConfig(cfg) so every purchase-path SDK
client gets RetryMaxAttempts=2 and http.Client.Timeout=15s.
OpenSearch and Redshift also pass the tightened config to their STS client
(used for ARN construction / tagging on the purchase path).
Recommendation-collection clients (recommendations.NewClient) are not touched;
they use their own constructor and deliberately keep the SDK default retry
budget for Cost Explorer calls.
All 694 tests pass. go build ./... clean.
Closes #683. Refs #667.
* fix(terraform): bump Lambda timeout 60 -> 300s in all three env tfvars
Raise lambda_timeout from 60s to 300s in github-dev, github-staging, and
github-prod tfvars. The SDK retry tightening (purchasecfg, max 30s wall clock)
combined with the increased Lambda budget means transient AWS API slowness
either completes within 30s or surfaces a clear error to the user -- no more
silent context deadline exceeded from the Lambda itself expiring mid-retry.
Lambda Function URL has no upstream timeout ceiling, so 300s is safe.
Closes #683. Refs #667 #681.
* feat(purchase): cap per-rec execution at 30s via context.WithTimeout
Each recommendation in processPurchaseRecommendations now runs under a
30s per-rec deadline derived from context.WithTimeout. This matches the
purchasecfg hard limits (2 retries * 15s HTTP timeout) so a single hung
rec (SDK retry storm, STS hang, or pagination hang) cannot exhaust the
Lambda budget and starve the remaining recs in the same execution.
On timeout, a purchase[<id>]: per-rec deadline 30s exceeded log line
fires so CloudWatch pinpoints which rec hung and what the elapsed time
was, without requiring a second failure to reproduce.
Closes part of issue #683.
* feat(purchase/api): add approve handler entry/exit diagnostic logging
Adds purchase[<exec-id>]: tagged timing logs at every stage of the
approval path so CloudWatch can pinpoint exactly where a future hang
occurs:
- approvePurchaseViaSession: entry (auth=session) + elapsed on exit
- ApproveExecution: entry (auth=token) + elapsed on exit for both the
preflight-reject path and the full execute path
- ApproveAndExecute: entry + status-transition elapsed + execute elapsed
Actor emails are masked to ***@Domain before logging (PII rule).
Part of issue #683 diagnostic logging extension.
* feat(purchase): add timing logs to resolveAccountProvider and sub-resolvers
Adds purchase[resolveAccountProvider/resolveAWSProvider/resolveAzureProvider/
resolveGCPProvider]: entry + elapsed-on-exit logs so CloudWatch can show
whether a hang is in credential resolution (STS AssumeRole, OIDC token
exchange) vs the cloud API call itself.
Each log includes provider, account ID, auth mode, and elapsed time.
Error paths also log before returning so a failed resolution is visible
in the trace even if the caller swallows the error in an aggregator.
Part of issue #683 diagnostic logging extension.
* feat(providers): add STS GetCallerIdentity and Azure subscriptions timing logs
Instruments the two network calls that ValidateCredentials makes during
provider construction so CloudWatch distinguishes a credential-validation
hang from the subsequent offering-search or purchase hang:
- providers/aws/provider.go: purchase[STS]: GetCallerIdentity starting /
returned in <elapsed> / failed after <elapsed>
- providers/azure/provider.go: purchase[Azure]: subscriptions.NewListPager/
NextPage starting / returned in <elapsed> / failed after <elapsed>
Both use the purchase[...]: prefix for grep-ability. Profile/subscription
context is logged but no tokens, key material, or full identities.
Part of issue #683 diagnostic logging extension.
* feat(providers/aws/services): add per-page timing logs to findOfferingID in all 7 AWS service clients
Adds purchase[<exec-id>]: tagged per-pagination-page timing logs to the
findOfferingID loop in each of the 7 AWS purchase-path service clients
(EC2, RDS, ElastiCache, MemoryDB, OpenSearch, Redshift, SavingsPlans).
Each log line records:
- Entry: service, instance/node type, term, payment option
- Per page: page number, offering count returned, page elapsed
- Hit: page number where match was found, total elapsed
- Exhausted: total pages scanned, total elapsed (no-match path)
The exec-id is threaded from PurchaseCommitment(opts.ExecutionID) into
findOfferingID so all lines share the same purchase[<uuid>]: prefix and a
single CloudWatch Logs Insights query can reconstruct the full pagination
trace for any hanging execution.
ValidateOffering and GetOfferingDetails callers pass "" (rendered as
"no-exec") since they run outside the purchase flow.
Part of issue #683 diagnostic logging extension.
* feat(execution): add per-goroutine entry/exit timing in FanOutWithConcurrency
Adds purchase[fan-out]: entry + elapsed-on-exit log lines to each
goroutine launched by FanOutWithConcurrency. Both the account fan-out
(multi-account execution) and the per-rec fan-out (processPurchaseRec-
ommendations) go through FanOutWithConcurrency, so one addition covers
both call sites.
Logs:
- purchase[fan-out]: goroutine starting for account=<id>
- purchase[fan-out]: goroutine for account=<id> completed in <elapsed>
- purchase[fan-out]: goroutine for account=<id> failed after <elapsed>: <err>
Together with the per-rec ctx.WithTimeout and findOfferingID page logs,
a CloudWatch Logs Insights query on purchase[ now reconstructs the full
timing tree for a stuck execution: fan-out goroutine start -> credential
resolution -> STS -> service client -> findOfferingID pagination ->
PurchaseCommitment.
Part of issue #683 diagnostic logging extension.
* fix(purchase/tests): fix per-rec context.WithTimeout breaking mock matchers
Move context.WithTimeout from the fan-out closure (processPurchaseRecommendations)
into executeSinglePurchase where it belongs -- the budget governs the cloud API
call chain, not the fan-out bookkeeping.
Update all GetServiceClient and PurchaseCommitment mock.On() setups in
execution_test.go, coverage_extra_test.go, and manager_test.go to match
on deadline-carrying contexts via:
mock.MatchedBy(func(c context.Context) bool { _, ok := c.Deadline(); return ok })
This positively asserts the 30s per-rec timeout is propagated to every
cloud SDK call rather than silently accepting any context (mock.Anything).
Also fix the savingsplans/client_test.go build error: findOfferingID gained
an execID string parameter in commit 5 but the direct test call was not
updated (only the PurchaseCommitment path was updated).
All 180 purchase package tests and 308 AWS service tests now pass.
Part of issue #683 diagnostic logging extension.
* fix(purchase): address CodeRabbit review on PR #684
Three changes per CR pass-1 triage:
1. Actionable (execution.go): distinguish context.DeadlineExceeded from
context.Canceled in the post-PurchaseCommitment error log. The previous
branch logged "per-rec deadline 30s exceeded" for any non-nil recCtx.Err(),
including parent cancellations -- mislabeling Canceled as a timeout makes
diagnostic triage harder. Logic extracted into logRecCtxErr helper to keep
executeSinglePurchase cyclomatic complexity within the gocyclo limit.
2. Nitpick (execution_test.go): replace mock.Anything with a deadline-aware
mock.MatchedBy on the first arg of all 12 CreateAndValidateProvider
expectations. The 30s per-rec budget is set before the factory call, so
all three mocked stages (factory, GetServiceClient, PurchaseCommitment)
now assert deadline propagation end-to-end.
3. Nitpick (purchasecfg/config.go + config_test.go): preserve the caller's
*http.Client when one is provided in base config. NewConfig now shallow-
copies the client and only overrides Timeout, keeping any custom Transport,
Jar, or CheckRedirect. Falls back to a fresh client when base.HTTPClient is
nil or a non-*http.Client implementation. Adds
TestNewConfig_PreservesCustomTransport to pin the contract.
* test(purchase): tighten ctx matcher to assert per-rec 30s budget (CR #684)
CodeRabbit on PR #684 (review 4348606803) noted that the 53 mock matchers
using `_, ok := c.Deadline(); return ok` only assert *some* deadline is
set -- they would still pass if `executeSinglePurchase` silently reused
the parent's longer deadline instead of its own
`context.WithTimeout(ctx, 30*time.Second)` wrap.
Tighten the contract: introduce `hasPerRecDeadline(max time.Duration)` in
`execution_test.go` and replace all 53 sites across `execution_test.go`,
`coverage_extra_test.go`, and `manager_test.go` with
`mock.MatchedBy(hasPerRecDeadline(30*time.Second))`. The matcher now
positively asserts the remaining deadline is in (0, 30s], so removing or
loosening the wrap fails the test.
* refactor(aws/services): extract findOfferingID helpers to fit gocyclo budget
memorydb: extract scanMemoryDBOfferingPage (per-page offering scan with
mismatch guard) and isLastMemoryDBPage (terminal-page check) from
findOfferingID, mirroring the helpers already in the ec2 package.
ec2: extract buildEC2QueryFromRec (details type-assertion + payment-option
conversion + query assembly) from findOfferingID into a standalone method.
Both findOfferingID functions now sit at cyclomatic complexity ≤10.
No behaviour change -- pure extraction verified by 72 passing unit tests.
1 parent fd33b50 commit 30e8912
26 files changed
Lines changed: 676 additions & 185 deletions
File tree
- internal
- api
- execution
- purchase
- providers
- aws
- internal/purchasecfg
- services
- ec2
- elasticache
- memorydb
- opensearch
- rds
- redshift
- savingsplans
- azure
- terraform/environments/aws
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
360 | 360 | | |
361 | 361 | | |
362 | 362 | | |
| 363 | + | |
| 364 | + | |
| 365 | + | |
363 | 366 | | |
364 | 367 | | |
365 | 368 | | |
| |||
376 | 379 | | |
377 | 380 | | |
378 | 381 | | |
379 | | - | |
| 382 | + | |
380 | 383 | | |
381 | 384 | | |
| 385 | + | |
| 386 | + | |
382 | 387 | | |
383 | 388 | | |
384 | 389 | | |
| 390 | + | |
| 391 | + | |
385 | 392 | | |
386 | 393 | | |
387 | 394 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
| 11 | + | |
11 | 12 | | |
12 | 13 | | |
13 | 14 | | |
| |||
92 | 93 | | |
93 | 94 | | |
94 | 95 | | |
| 96 | + | |
| 97 | + | |
95 | 98 | | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
96 | 107 | | |
97 | 108 | | |
98 | 109 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
22 | 22 | | |
23 | 23 | | |
24 | 24 | | |
25 | | - | |
| 25 | + | |
| 26 | + | |
26 | 27 | | |
27 | 28 | | |
28 | 29 | | |
| |||
50 | 51 | | |
51 | 52 | | |
52 | 53 | | |
| 54 | + | |
| 55 | + | |
53 | 56 | | |
54 | 57 | | |
55 | 58 | | |
56 | | - | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
57 | 90 | | |
58 | 91 | | |
59 | 92 | | |
| |||
108 | 141 | | |
109 | 142 | | |
110 | 143 | | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
111 | 147 | | |
112 | 148 | | |
| 149 | + | |
| 150 | + | |
113 | 151 | | |
114 | 152 | | |
| 153 | + | |
115 | 154 | | |
116 | 155 | | |
117 | 156 | | |
118 | 157 | | |
119 | 158 | | |
120 | | - | |
| 159 | + | |
121 | 160 | | |
122 | 161 | | |
123 | 162 | | |
124 | 163 | | |
125 | 164 | | |
126 | 165 | | |
127 | | - | |
128 | | - | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
129 | 174 | | |
130 | 175 | | |
131 | 176 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
4 | 4 | | |
5 | 5 | | |
6 | 6 | | |
| 7 | + | |
7 | 8 | | |
8 | 9 | | |
9 | 10 | | |
| |||
285 | 286 | | |
286 | 287 | | |
287 | 288 | | |
288 | | - | |
| 289 | + | |
289 | 290 | | |
290 | 291 | | |
291 | 292 | | |
| |||
582 | 583 | | |
583 | 584 | | |
584 | 585 | | |
585 | | - | |
| 586 | + | |
586 | 587 | | |
587 | 588 | | |
588 | 589 | | |
| |||
630 | 631 | | |
631 | 632 | | |
632 | 633 | | |
633 | | - | |
634 | | - | |
| 634 | + | |
| 635 | + | |
635 | 636 | | |
636 | 637 | | |
637 | 638 | | |
| |||
679 | 680 | | |
680 | 681 | | |
681 | 682 | | |
682 | | - | |
683 | | - | |
684 | | - | |
| 683 | + | |
| 684 | + | |
| 685 | + | |
685 | 686 | | |
686 | 687 | | |
687 | 688 | | |
| |||
732 | 733 | | |
733 | 734 | | |
734 | 735 | | |
735 | | - | |
736 | | - | |
737 | | - | |
| 736 | + | |
| 737 | + | |
| 738 | + | |
738 | 739 | | |
739 | 740 | | |
740 | 741 | | |
| |||
788 | 789 | | |
789 | 790 | | |
790 | 791 | | |
791 | | - | |
792 | | - | |
793 | | - | |
| 792 | + | |
| 793 | + | |
| 794 | + | |
794 | 795 | | |
795 | 796 | | |
796 | 797 | | |
| |||
845 | 846 | | |
846 | 847 | | |
847 | 848 | | |
848 | | - | |
849 | | - | |
850 | | - | |
| 849 | + | |
| 850 | + | |
| 851 | + | |
851 | 852 | | |
852 | 853 | | |
853 | 854 | | |
| |||
1083 | 1084 | | |
1084 | 1085 | | |
1085 | 1086 | | |
1086 | | - | |
1087 | | - | |
| 1087 | + | |
| 1088 | + | |
1088 | 1089 | | |
1089 | 1090 | | |
1090 | | - | |
| 1091 | + | |
1091 | 1092 | | |
1092 | 1093 | | |
1093 | 1094 | | |
| |||
1221 | 1222 | | |
1222 | 1223 | | |
1223 | 1224 | | |
1224 | | - | |
1225 | | - | |
| 1225 | + | |
| 1226 | + | |
1226 | 1227 | | |
1227 | 1228 | | |
1228 | | - | |
| 1229 | + | |
1229 | 1230 | | |
1230 | 1231 | | |
1231 | 1232 | | |
| |||
0 commit comments