[Michael Flanakin (flanakin)]

🤖 [AI]

Summary

Fixes the Data Explorer dashboard savings math from #2093 Phase 1.3 so every savings tile uses one clamped basis, reconciles on screen, and never counts commitment purchases as savings.

Changes (dashboard.json)

• Clamped savings basis – Savings come from the row-level clamped columns (x_NegotiatedDiscountSavings, x_CommitmentDiscountSavings) instead of sum(ListCost) - sum(EffectiveCost).
• Derived operands – Tiles that show an equation build List and Contracted cost from the same basis, so the numbers on screen add up:
  • Contracted = Effective + Commitment savings
  • List = Contracted + Negotiated savings
  • Total savings = Negotiated + Commitment
• Purchases excluded (combined with #2352) – All 9 Summary queries use sumif(x_*Savings, x_AmortizationClass != 'Principal'). Effective cost stays unfiltered because Principal rows have EffectiveCost = 0.
• One purchase filter – Every Rate optimization tile now uses x_AmortizationClass != 'Principal'. The local re-derivations and the ChargeCategory == 'Usage' or isempty(CommitmentDiscountId) variant are gone.
• NaN guard – ESR uses iff(ListCost == 0, real(null), Savings / ListCost) on both ESR tiles and Savings breakdown by month.
• Left unchanged – The Data ingestion alignment diagnostic keeps its raw deltas on purpose.

Behavior change: ESR on the affected tiles is now (Negotiated + Commitment savings) / derived List cost. Release note is under Unreleased → FinOps hubs v16.

Compatibility with #2352

#2352 (v15) is still open and edits the same 9 queries. This PR already has the combined form (derived basis plus the Principal exclusion). When both merge, resolve conflicts on those queries by taking this PR's side.

Known gap

Non-Azure commitment purchases with an empty x_AmortizationClass aren't excluded. That should be fixed in ingestion (set the column for all providers), not per tile.

Verification

• dashboard.json is valid JSON. Only the edited query strings changed.
• Pester Lint + Unit: 6299 passed, 0 failed.
• Counterexample check: rows (100,120,80) and (100,80,60) give Negotiated 20, Commitment 60, Contracted' 200, List' 220, ESR 36.4%. Adding a 300 Principal row leaves Summary and Rate optimization both at 80.

🤖 Generated with Claude Code

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Roland Krummenacher (RolandKrummenacher)]

Reviewed the refactor and validated the assumptions by running the before/after formulas against three production hubs I have read access to (read-only queries, no identifying details below). The mechanical KQL refactor is clean, but the behavior change is materially larger than the description suggests, and it breaks the tiles that render savings as a visible equation.

Summary

The core issue is that x_TotalSavings, x_CommitmentDiscountSavings, and x_NegotiatedDiscountSavings are clamped per row. Summing them is therefore not equivalent to subtracting the aggregates — it is a different metric, one that structurally cannot net out credits, refunds, and corrections. On one hub the last-3-months total savings comes out ~40% higher after this change; on another the divergence is ~0.04%. So the blast radius is entirely tenant-dependent, which makes shipping it silently riskier, not safer.

That may well be the metric we want. But it is a product decision that belongs in #2093 with a release note, not a side effect of a "use the canonical column" refactor.

Details in the inline comments. Ordered by severity:

1. Savings-breakdown tiles now display arithmetic that does not add up — operands stayed raw, results became clamped.
2. Parts no longer sum to the whole — negotiated + commitment overstates total by ~61% on one hub.
3. Effective savings rate silently inflated — clamped numerator over raw denominator.
4. The FX guard likely regresses non-Microsoft FOCUS ingestion — and is a no-op on every Azure hub I checked.
5. The auditability claim in the description only holds for one of the two code paths.

What is right

• The refactor itself is correct KQL. The union/pivot tiles properly re-sum the already-renamed columns in the second summarize.
• Dropping the redundant local extend re-derivations in 3b3f0a58 and 0341b3e4 is a genuine simplification — the latter's were entirely dead code.
• Removing now-unused ListCost/ContractedCost from summarize lists (e.g. 2d7b6447) is a real efficiency win on wide scans.
• Leaving the Summary of list, contracted, and effective cost alignment diagnostic on raw unclamped deltas is the right call, and the reasoning in the description is sound.

Two things not introduced here, but now load-bearing

The two clamp formulas disagree. HubSetup_v1_2.kql:169-171 uses iff(ContractedCost < EffectiveCost, real(0), ...); IngestionSetup_v1_2.kql:706-708 uses iff(isempty(ListCost) or ListCost == 0 or ListCost - EffectiveCost < 0.0001, real(0), ...). Pre-existing, but this PR makes 21 tiles depend on those columns, so a hub with mixed v1.0/v1.2 data will now get two different clamping semantics inside a single displayed number. Worth aligning while we are here.

Dashboard/hub version coupling. I verified via getschema that a hub running a current released version does not yet expose the x_*Savings columns on Costs_v1_2() — they come from this branch. Fine for a normal template deploy, but importing the new dashboard.json against a not-yet-upgraded hub now produces hard query failures on all 21 tiles rather than degraded output. Probably worth a release-note line.

Recommendation

Blocking on 1-3: either keep raw subtraction in the tiles that render savings as a visible equation, or derive the displayed operands from the clamped relationship so the math closes. 4 should be narrowed to a currency-equality guard before merge. 5 is a follow-up.

Happy to take the tile fixes if that is useful.

[Roland Krummenacher (RolandKrummenacher)]

Re-reviewed at 0e05f98. The KQL revert resolves the two threads it touches — replied there, no objection. The PR is now a pure dashboard.json change, +21/-21.

The six dashboard threads are untouched by this commit and all still apply, so keeping changes requested. Consolidating them here, since they're one root cause rather than six independent problems.

Root cause: the change swaps raw subtractions for sum(x_*Savings) on the result side of each tile, while leaving sum(ListCost) / sum(ContractedCost) / sum(EffectiveCost) raw on the operand side. The x_*Savings columns clamp negatives at row level, so the two sides no longer reconcile.

Tile	Symptom, measured on a production hub
Savings breakdown, monthly (e17346d1) + daily (f1ef29df)	the three equations the tile literally displays don't hold: +25.9%, +53.8%, +40.2%
Total / Negotiated / Commitment (line 3074)	components exceed the total they decompose by ~61%
Stacked savings area chart (line 2876)	stack no longer sums to ListCost, overshoots by a tenant-dependent amount
Effective Savings Rate (e1bc2d51 + daily twin)	clamped numerator over raw denominator: ESR +40% relative, one month +230%

One decision settles all four: either keep the raw subtractions on these tiles, or derive the displayed cost operands from the clamped columns so the arithmetic closes. I don't have a strong preference as long as each tile is internally consistent — a user reading two numbers and their stated difference shouldn't see a third number that isn't the difference.

ESR is the one I'd treat as separate from the arithmetic question: it's a headline KPI, and a step change of that size across an upgrade boundary makes the month-over-month series non-comparable. If the clamped definition is the one we want, that needs a release note rather than just a formula change.

Happy to re-measure whichever direction you pick.

[Brett Wilson (MSBrett)]

Requesting changes. I re-reviewed the current head (0e05f98b) from the base diff, traced the savings fields to their source definitions, checked all 21 changed queries, and independently reproduced the arithmetic failures below.

The replacement of aggregate subtraction with sums of row-level clamped savings is not semantics-preserving for visuals that still rely on exact accounting relationships:

1. The monthly and daily savings-summary tiles display false equations (dashboard.json:3086 and :3092). They still show raw aggregate values for List, Contracted, and Effective cost, followed by subtraction and equals signs, but the displayed results now sum independently clamped row values. Whenever a row is clamped, the displayed operands no longer produce the displayed result.

2. The Total / Negotiated / Commitment summary is no longer a valid decomposition (dashboard.json:3074). Independently clamped negotiated and commitment savings are not additive and can sum to more than total savings.

3. The stacked Cost + savings chart no longer reconciles to list cost (dashboard.json:2876). It stacks raw effective cost with both independently clamped savings categories, so the stack can overstate the implied cost without discounts.

A minimal counterexample confirms all three failures: for rows (List, Contracted, Effective) = (100,120,80) and (100,80,60), the raw negotiated difference is 0 while summed clamped negotiated savings is 20; negotiated plus commitment savings is 80 while total savings is 60; and the stacked value is 220 against list cost of 200.

The clamped Effective Savings Rate formula is not inherently invalid—it matches the repository's savings-summary-report.kql precedent—but it is a material KPI-definition change rather than a mechanical refactor. If retained for ESR and standalone savings visuals, the changed semantics and historical discontinuity should be explicitly approved and documented in release notes.

Please either keep aggregate subtraction for visuals that present reconciled equations/components, or redesign those visuals and labels so the independently clamped metrics are not presented as arithmetic identities. Also update the PR title/body now that the FX change has been reverted and remove the claim that the savings portion was verified clean while these blocking threads remain unresolved.

[Roland Krummenacher (RolandKrummenacher)]

Re-reviewed at 0c226999. All three items from my last round are implemented as described: Cost summary is on the derived basis and reconciles with Savings summary beside it; every displayed "Total savings" is now sum(neg) + sum(comm) (no x_TotalSavings left on screen); Savings breakdown by month derives List cost and ESR the same way as the ESR tiles. I re-ran the counterexample across all tiles and the Rate optimization page reconciles end to end (220 / 200 / 140 → 20 / 60 / 80, ESR 36.4%). The merge from v15-prep only touched markdown in dashboard.json.

Two things still need to be settled before this can go in, details inline:

1. Commitment purchases still count as savings on the Summary page, which conflicts with #2352.
2. Rate optimization uses three different filters for purchase rows.

Minor: the PR title and description are stale (FX fallback, x_TotalSavings, "6 tiles").

Given how visible these numbers are, I'll also walk through the deployed dashboard against a live hub (purchase month, non-USD, EA and MCA) before approving.

[Roland Krummenacher (RolandKrummenacher)]

Re-reviewed at 5f764371. All four items from my last round are implemented as described:

• All 9 Summary queries use sumif(x_*Savings, x_AmortizationClass != 'Principal'), and Effective cost stays unfiltered.
• Every Rate optimization tile filters with x_AmortizationClass != 'Principal'. The local re-derivations in 3b3f0a58 and 0341b3e4 are gone, and so is the ChargeCategory == 'Usage' or isempty(CommitmentDiscountId) filter. Removing the re-derivation is safe because ingestion fills x_AmortizationClass when an export doesn't provide it.
• The stale comment in dcc1f533 is fixed.
• The changelog entry is under Unreleased → FinOps hubs v16, with the correct tile names.

The tiles that still use plain sum(x_*Savings) are fine: d7f31381 and 13cad52d already filter to ChargeCategory == 'Usage', and 3886b5cd only shows Effective cost. One nit inline. LGTM.

[Michael Flanakin (flanakin)]

🤖 [AI] Merged origin/dev (conflicts were stale v15-prep docs; took dev, then re-added the Unreleased v16 changelog entry). Added HubsDashboardSavings.Tests.ps1: guards against raw-aggregate savings, the legacy purchase filter, missing Principal exclusion on Summary queries, and ESR zero-list-cost guard. Brett Wilson (@MSBrett) your 3 blocking points (false equations, decomposition, stack) were addressed by the derived-operand basis in earlier commits (verified by Roland Krummenacher (@RolandKrummenacher), who approved at 5f76437); please re-review.