fix(hubs): relax reservation price backfill gate to recover MCA rows

[Michael Flanakin (flanakin)]

🛠️ Summary

The Reservations tab in the Rate Optimization Power BI report loads empty for MCA (Microsoft Customer Agreement) deployments, with no errors. Root cause: the tmp_MissingPrices gate in the reservation price backfill requires a non-blank x_SkuOfferId, an EA-only field. MCA cost and price rows never populate an offer ID (confirmed against reporter data in #2176), so MCA rows are filtered out before the price recovery join even runs — list price stays 0, and ListCost falls back to ContractedCost, producing $0 reservation savings.

Fix: drop the offer ID requirement from the gate (isnotempty(x_SkuMeterId) and isnotempty(x_SkuOfferId) → isnotempty(x_SkuMeterId)). x_SkuOfferId stays unchanged in the reservation price lookup key — for MCA it's blank on both the cost and price side (a no-op suffix, so the key still matches), and for EA it keeps differentiating rows sharing a profile/meter (e.g. Production vs Dev/Test offers) exactly as before.

Applied identically to both IngestionSetup_v1_0.kql and IngestionSetup_v1_2.kql.

🔎 Background

• Data in Rate Optimization Report are loading fine except Reservations tab ( currently empty with no errors ) #1769 reported the empty Reservations tab for MCA. Two prior attempts closed unmerged: Change the data type for ChargePeriodStart and ChargePeriodEnd to date to prevent issues with the runningtotal measures when more than one billing profile is present. #1626 targeted a different issue (Total savings number is incorrect #1614) and was reverted per maintainer feedback; Triage and document solution for empty Reservations tab in MCA deployments #1844 was a Copilot agent attempt that shipped no code changes and mis-diagnosed this as a configuration gap rather than a code bug.
• Reservation savings underreported due to min() price selection on duplicated reservation price keys #2176 independently root-caused the same defect with much more precision — Roland Krummenacher (@RolandKrummenacher) confirmed with the reporter that x_SkuOfferId is blank on both MCA cost and price rows, and proposed this exact gate-relaxation fix in this comment. The fix was never implemented; Reservation savings underreported due to min() price selection on duplicated reservation price keys #2176 was auto-closed when fix(hubs): use max() when duplicate on-demand price rows collapse under one lookup key #2189 merged (that PR's commit trailer still said Fixes #2176 even though the PR body explicitly says it does not fix the MCA case — it only hardens min()→max() for a related but distinct collision scenario). I reopened Reservation savings underreported due to min() price selection on duplicated reservation price keys #2176 to track this properly.

✅ Verification: is "OfferId is blank for MCA" actually true?

Before shipping this, I wanted more than a reporter's screenshot behind the core premise, so I checked it against Microsoft's own schema documentation:

• Understand cost details fields scopes OfferId to account type EA, pay-as-you-go — MCA is absent.
• EA cost and usage details schema includes OfferId in every published version.
• MCA cost and usage details schema — checked all five published versions — includes it in none of them. This is the strongest evidence: the upstream MCA export has no offerId field to populate, structurally, not just as an observed data gap.
• Migrate from EA to MCA APIs lists offerID under "Retired Price Sheet API fields": "Not applicable [for MCA]. Corresponds to productOrderName in MCA."
• The Microsoft FinOps Blog: Learning FOCUS: SKUs states directly: "x_SkuOfferId is the EA offer ID for the subscription... This column is empty and not applicable for MCA subscriptions." (Note: same author as this toolkit, so treat as authoritative-but-not-independent alongside the schema docs above.)
• This repo's own docs already say the same thing in a different file: docs-mslearn/toolkit/hubs/data-model.md:680 documents x_SkuOfferId as "Source: Microsoft, EA only."

Residual risk not fully closed by documentation: whether any EA edge case (unusual charge type, legacy export version, partner-billed scenario) can also produce a blank x_SkuOfferId, which would make the relaxed gate over-match for EA rows too. No Microsoft documentation suggests this, but it isn't something docs can rule out — only live EA export data or running Tests/assets/ReservationPriceBackfillKey.kql against a real hub database can close that gap completely.

⚠️ Not included

A second, independent-looking defect — x_BillingProfileId is normalized to a bare ID in the Prices transform but assigned raw (which can be a full ARM path for MCA) in the Costs transform — was flagged during investigation but is not included here per review feedback: it's unconfirmed against real export data, and normalizing it away may not be the desired behavior (the Price Sheet's bare-ID form could itself be an export limitation rather than the canonical shape). Needs discussion before changing.

Also not included: a lower-severity issue where the kql-based Power BI dataset is missing Prices/ReservationDetails/ReservationTransactions tables that the storage dataset has, while RateOptimization.Report/report.json still references a Prices page. Tracked as a fast-follow.

🧪 Testing

• New HubsReservationPriceBackfill.Tests.ps1 (content assertions on both KQL files) + ReservationPriceBackfillKey.kql executable harness (old-vs-new gate/key behavior across EA and MCA fixture rows), following the existing HubsContractedCostGuard pattern.
• Full unit suite: 2269/2269 passed, 0 failed.
• Hand-traced all harness fixtures against both old and new logic before trusting the harness.

Fixes #1769
Fixes #2176

🔬 How did you test this change?

• 🤏 Lint tests
• 🤞 PS -WhatIf / az validate
• 👍 Manually deployed + verified
• 💪 Unit tests
• 🙌 Integration tests

🙋‍♀️ Do any of the following that apply?

• 🚨 This is a breaking change.
• 🤏 The change is less than 20 lines of code.

📑 Did you update docs/changelog.md?

• ✅ Updated changelog (required for dev PRs)

📖 Did you update documentation?

• ✅ Public docs in docs (required for dev)
• ✅ Internal dev docs in src (required for dev)
• ❎ Docs not needed (small/internal change)

🤖 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)]

Review — PR #2263

The gate change is correct and the evidence behind "x_SkuOfferId is EA-only" is more than sufficient — the MCA dataset schema having no offerId field at all is the right kind of proof, and I appreciate that the residual-risk section says plainly what documentation can't rule out. Dropping the offer ID from the gate while leaving it in the key is the minimal, safe shape of this fix. No objection to the change itself.

Two things before this merges.

1. Does this actually fix #1769 end-to-end, or only unblock the gate?

The fix's premise is that once the gate lets MCA rows through, tmp_ReservationPriceLookupKey matches on the price side. That holds for the offer ID (blank on both sides, identical no-op suffix). It does not obviously hold for the first component of the key.

Prices_transform_v1_2 normalizes the billing profile ID out of ARM-path form:

// IngestionSetup_v1_2.kql:65
| extend x_BillingProfileId = iff(BillingProfileId startswith '/', split(BillingProfileId, '/')[-1], coalesce(BillingProfileId, EnrollmentNumber))

ActualCosts_transform_v1_2 and AmortizedCosts_transform_v1_2 assign it raw (:1217, :1438), and those feed Costs_raw via update policy (:1299-1307), which is what Costs_transform_v1_2 then keys on at :419. So on the C360 Actual/Amortized ingestion path, an MCA row whose BillingProfileId arrives as a full ARM path produces a cost-side key of /providers/microsoft.billing/…<month><meter> against a price-side key of <guid><month><meter>. The relaxed gate lets the row into the join and the join still misses — ListUnitPrice stays 0 and the Reservations tab stays empty.

This is the same asymmetry the PR body defers as "flagged during investigation but not included … needs discussion," and I agree it shouldn't be fixed blind. But it's load-bearing for the claim this PR makes: if the reporter in #1769 is on that path, this merges as a fix that changes nothing for them, and the Fixes #1769 / Fixes #2176 trailers auto-close both issues on merge.

Two ways out, either is fine with me:

• Confirm which export path the #2176 reporter's data came through. If it's a FOCUS export (x_BillingProfileId passed through from the FOCUS column rather than the C360 mapping), the profile IDs agree and this fix is complete for them — worth one line in the PR body saying so.
• Or downgrade the trailers to Ref #1769 / Ref #2176 and let the issues stay open until the profile-ID question is settled, so we don't repeat the #2189 pattern where a trailer closed #2176 for a fix that the PR body itself said didn't cover the MCA case.

Given the history on this specific bug — two prior closed-unmerged attempts and one incorrect auto-close — I'd rather over-communicate the scope than have #1769 close a third time without the reporter seeing data.

2. Test harness has two gaps worth a few minutes

Neither blocks, but both weaken the regression net that's the main reason to add these files at all.

The harness doesn't model the month component. The header comment documents the key as tolower(strcat(x_BillingProfileId, month, x_SkuMeterId, x_SkuOfferId)), but the modeled keys omit it entirely:

| extend old_costKey = tolower(strcat(billingProfileId, meterId, offerId))

Harmless for the current fixtures (all implicitly same-month), but the comment claims coverage the code doesn't have, and a future month-boundary bug would pass. Either add a month column to the datatable or say in the header that month is deliberately held constant.

One assertion can never fail. In HubsReservationPriceBackfill.Tests.ps1:

$content.Contains('isnotempty(x_SkuMeterId)') | Should -BeTrue

isnotempty(x_SkuMeterId) is a substring of the old isnotempty(x_SkuMeterId) and isnotempty(x_SkuOfferId) too, so this passes against pre-fix and post-fix code alike. If the intent is "the meter ID requirement survived," it needs to anchor on the full post-fix line.

Coordination note

Draft #2246 touches the same price-transform region and is currently conflicting. Whichever of the two lands first, the other will need a merge from dev — worth deciding the order deliberately rather than discovering it.

[Roland Krummenacher (RolandKrummenacher)]

Re-review — PR #2263 (head 20a8475)

Re-reviewed after the 08-26 merge from dev. The direction is still right and I still have no objection to it: x_SkuOfferId really is EA-only, and the old gate did exclude every MCA row before the join was attempted.

Requesting changes on one finding that's new since 08-18: the relaxed gate isn't scoped to reservation rows, so it changes reported cost for a much wider population than the fix targets, including rows that find no price at all. Details inline.

New in this pass

1. Gate is not reservation-scoped — blocker. Rows that enter the join and miss now get contracted/list imputed from effective, where they previously passed through untouched.
2. Join fan-out newly exposed to MCA — pre-existing for EA, but worth one uniqueness check before this widens its blast radius.
3. Two "must cover" assertions can never fail — they match the harness comment header, not the fixtures.
4. Harness can't detect a lookup-key regression — the old and new key expressions are byte-identical.

Still open from 08-18

20a8475 is a merge from dev, so nothing has changed in response to the previous round. These stand:

• The x_BillingProfileId ARM-path vs bare-GUID asymmetry, and whether Fixes #1769 / Fixes #2176 should be downgraded to Ref so the issues don't auto-close on a fix the reporter may not see.
• The harness omitting the month component while its header documents it.
• $content.Contains('isnotempty(x_SkuMeterId)') passing against pre-fix code too.

Item 1 is the only one I'd hold the merge for; 3 is a two-line fix that can ride along with it, and 2 needs one query against real price data to settle. Happy to turn a re-review around quickly once the gate scoping is decided.

[Roland Krummenacher (RolandKrummenacher)]

Blocker — this gate isn't scoped to reservation rows.

Dropping isnotempty(x_SkuOfferId) is right for the reservation case, but the surrounding predicate has no reservation filter, so the selected population is now every Microsoft row with a zero/empty list or contracted unit price. Those rows previously failed the gate and passed through untouched via union (allCosts | where not(tmp_MissingPrices)) at line 489. They now enter the join — and, critically, the post-join case chains — whether or not the join finds a price.

For a row that enters and misses the lookup, every case falls through to its last branch, because PricingUnit1 and x_PricingBlockSize1 are null on a leftouter miss:

• L445/447 → ContractedUnitPrice := x_EffectiveUnitPrice
• L457 → ListUnitPrice := ContractedUnitPrice
• L467 → ContractedCost := EffectiveCost (this condition is now true, since ContractedUnitPrice was just set to x_EffectiveUnitPrice above)
• L480 → ListCost := ContractedCost

So a row that recovers no price at all still emerges with contracted and list values imputed from effective, where before it stayed 0. That is a change to reported cost, not a no-op, and it lands on a considerably wider population than reservations.

This weighs more given the x_BillingProfileId asymmetry I raised on 08-18 and this PR defers: if cost-side keys are ARM paths while price-side keys are bare GUIDs, the miss path isn't the edge case — it's the common outcome, and the net effect of the PR becomes "impute contracted/list from effective across MCA" rather than "recover list prices for MCA reservations".

Two things I'd want before this merges:

1. Scope the gate to reservation rows, or gate the imputation branches on a successful join (e.g. isnotempty(ContractedUnitPrice1)), so a miss leaves the row exactly as it was.
2. If the imputation is intended, state it in the changelog — the current entry reads as a narrowly targeted reservation fix.

[Roland Krummenacher (RolandKrummenacher)]

Same issue as the v1_2 script — see my comment on IngestionSetup_v1_2.kql:416.

The v1_0 predicate differs only in the missing-price test (ListUnitPrice == 0 or ContractedUnitPrice == 0, without the isempty(...) arms), so the population and the post-join fall-through behavior are the same. Whatever scoping lands in v1_2 needs to land here too, and the two should stay identical.

[Roland Krummenacher (RolandKrummenacher)]

(Anchoring here — the lines I mean are 421–426, just outside the diff.)

The leftouter right side is summarized by tmp_ReservationPriceLookupKey, x_PricingBlockSize, PricingUnit at line 426, so it is not unique per lookup key. Any key carrying more than one (x_PricingBlockSize, PricingUnit) combination fans the cost row out into several output rows, all of which land in the union at line 489 — double-counted BilledCost and EffectiveCost.

This is pre-existing for EA and not introduced by this PR, but this PR is what exposes every MCA row to it, so it's worth one check before merge rather than after: confirm that (key, x_PricingBlockSize, PricingUnit) collapses to a single row per key in real Prices_final_v1_2 data. If it does, ship it and file a follow-up for the dedupe. If it doesn't, this inflates billed cost for MCA and the right side needs reducing to one row per key first.

[Roland Krummenacher (RolandKrummenacher)]

These two "must cover" assertions can never fail.

Should -Match 'MCA' here and Should -Match 'EA' at line 74 run against the whole harness file, and both strings occur in its comment header — MCA at lines 16 and 29, EA at lines 19 and 30 — all well above the datatable. I checked: delete every fixture row and both tests still pass.

Anchor on fixture literals instead ('mca-profile-1', 'OFFER-EA-DEVTEST') so the assertions pin the rows they claim to cover.

Related, and still open from 08-18: $content.Contains('isnotempty(x_SkuMeterId)') at line 47 has the same shape — it is a substring of the pre-fix isnotempty(x_SkuMeterId) and isnotempty(x_SkuOfferId), so it passes against pre-fix and post-fix code alike.

[Roland Krummenacher (RolandKrummenacher)]

new_costKey here and old_costKey at line 62 are byte-identical, as are new_priceKey (68) and old_priceKey (63).

That is faithful to the fix — the key genuinely doesn't change — but it means the gate is the only variable the harness can move, so the harness cannot fail if the lookup key itself regresses. The unit test at line 50 ("Should leave x_SkuOfferId in the reservation price lookup key") leans on exactly the property the harness isn't exercising.

Together with the month-segment gap I raised on 08-18 and two more — no x_SkuPriceType == 'Consumption' filter, and no fixture where one key maps to several price rows (see my note on the join fan-out) — the harness documents the fix more than it regression-tests it. Either extend it to vary the key, or trim the header comment so it doesn't claim coverage the fixtures don't provide.

[microsoft-github-policy-service]

@Michael Flanakin (@flanakin), thanks for the PR. It looks like you forgot to indicate whether the documentation was updated as part of the PR. Please edit the PR to select (x) the appropriate items in the checklist. This PR will be blocked until one of the values is selected.

[microsoft-github-policy-service]

@Michael Flanakin (@flanakin), thanks for the PR. It looks like you forgot to indicate whether the documentation was updated as part of the PR. Please edit the PR to select (x) the appropriate items in the checklist. This PR will be blocked until one of the values is selected.