Skip to content

fix(providers/aws): fail loud on unparseable SP cost fields + RI utilization hours (COR-07 siblings to #1171) #18

Description

@cristim

Problem

PR LeanerCloud/cloud-commitments-cli#1236 (closes LeanerCloud/cloud-commitments-cli#1171, COR-07) made AWS RI cost-field parsing fail loud in parseAWSCostDetails (providers/aws/recommendations/parser_ri.go). The same silent-swallow pattern still exists in two sibling code paths in the same package, on equally money-affecting fields:

1. Savings Plans parser (providers/aws/recommendations/parser_sp.go)

parseOptionalFloat (lines 182-192) silently returns 0 when strconv.ParseFloat fails, then is called for every money field on every SP recommendation:

func parseOptionalFloat(field string, s *string) float64 {
    if s == nil { return 0 }
    val, err := strconv.ParseFloat(*s, 64)
    if err != nil {
        log.Printf("WARNING: failed to parse %s: %v", field, err)
        return 0          // <-- silent fallback to 0 on money path
    }
    return val
}

Callers (parseSavingsPlanDetail, lines 202-221):

  • HourlyCommitmentToPurchase -> drives RecurringMonthlyCost (× 730) and hourlyCommitment field for the recommendation
  • EstimatedMonthlySavingsAmount -> rec.EstimatedSavings (direct savings figure)
  • EstimatedSavingsPercentage -> rec.SavingsPercentage
  • UpfrontCost -> rec.CommitmentCost
  • CurrentAverageHourlyOnDemandSpend -> rec.OnDemandCost (× 730) — the denominator in effective-savings math (see issue fix(recommendations/aws): incorrect Effective Savings % — plumb on_demand_cost like Azure (#277) cloud-commitments-cli#303)
  • EstimatedAverageUtilization -> utilization signal (non-money, "0 = no signal" semantics documented, so the silent fallback is intentional here)

Same shape as COR-07: an all-upfront SP with an unparseable UpfrontCost would surface as $0 upfront -> wrong money figure feeding effective-savings math and purchase decisions, with only a WARNING log.

2. RI utilization parser (providers/aws/recommendations/utilization.go)

parseFloat (lines 142-149) silently returns 0 on parse failure for Utilization.PurchasedHours, Utilization.TotalActualHours, Utilization.UnusedHours (lines 131-138). These accumulate into riAccumulator, then buildUtilizations divides:

pct = (a.totalActualHours / a.purchasedHours) * 100

A parse failure on purchasedHours quietly understates utilization or yields 0%; a failure on totalActualHours understates the numerator. This corrupts the --target-coverage sizing input directly.

Why this is the same bug as COR-07

Memory feedback_no_silent_fallbacks.md ("no fallbacks/defaults/fabricated values on money paths; return an explicit error") covers this exact pattern. PR LeanerCloud/cloud-commitments-cli#1236 fixed RI cost fields; SP cost fields and RI utilization hours are equivalent money-path inputs in the same package.

Suggested fix

Mirror the parser_ri.go pattern: parseOptionalFloat and parseFloat return (float64, error). Callers either propagate to parseSavingsPlanDetail / GetRIUtilization (which then skip-with-warning, matching the existing RI skip in parseRecommendations for parseRecommendationDetail failures), or for utilization-signal-only fields (EstimatedAverageUtilization, AverageNumberOfInstancesUsedPerHour, AverageUtilization) keep the log-and-default semantics with a docstring noting "zero means no signal".

Acceptance

Related

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions