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.

[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.

[Michael Flanakin (flanakin)]

🤖 [AI][Claude] PR Update Summary

Addressed: 1 of 5 open thread(s)

• ✅ Implemented: 1 (test assertions anchored on fixture literals, not header text)
• 🤔 Needs discussion: 0
• ❓ Questions: 0
• Left for maintainer: 4 (gate reservation-scoping, join fan-out dedupe check, and extending the harness to model month/key regressions — all judgment calls on production pricing/backfill logic, intentionally not auto-applied)

Ran the full HubsReservationPriceBackfill.Tests.ps1 suite (10/10 passed) and sanity-checked the new regex correctly rejects the pre-fix gate text.

[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.

[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.

[Roland Krummenacher (RolandKrummenacher)]

Re-review — PR #2263 (head 2cb5d32)

My blocker is resolved. The gate in both IngestionSetup_v1_0.kql and IngestionSetup_v1_2.kql now admits rows without an offer ID only when CommitmentDiscountCategory == 'Usage'. Every other row keeps the old behavior, so reported cost outside reservation usage doesn't change. The harness header now states what it covers, and the new fixture shows that a non-reservation MCA row stays outside the gate. Approving.

Non-blocking follow-ups

1. x_BillingProfileId asymmetry on the Actual/Amortized path: For FOCUS exports, the cost side carries x_BillingProfileId as the bare ID, and Prices_transform_v1_2 reduces the price side to the bare ID too, so the keys match. On the Actual/Amortized path, x_BillingProfileId = BillingProfileId (lines 1223 and 1444) keeps the raw value. If that value arrives as an ARM path, the lookup key still won't match. This isn't a regression from this PR, so please track it in a separate issue.
2. Fixes vs Ref: Please change Fixes #1769 and Fixes #2176 to Ref unless the reporters ingest FOCUS exports. Otherwise both issues close on merge even if their data goes through the path in item 1.