fix(hubs): relax reservation price backfill gate to recover MCA rows #2263
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| # Copyright (c) Microsoft Corporation. | ||
| # Licensed under the MIT License. | ||
|
|
||
| <# | ||
| Regression coverage for the MCA reservation list-price backfill fix (#1769 / #2176): | ||
| the price backfill gate must not require x_SkuOfferId. x_SkuOfferId is EA-only -- for MCA it is blank on | ||
| both the cost and price side (confirmed against reporter data in #2176), so gating on it excluded every | ||
| MCA row from the price backfill before the join was even attempted. x_SkuOfferId stays in the lookup key | ||
| unchanged: for MCA it is blank on both sides (a no-op suffix), and for EA it keeps differentiating rows | ||
| (e.g. Production vs Dev/Test offers) exactly as before. | ||
|
|
||
| NOTE: a separate, unconfirmed hypothesis -- that x_BillingProfileId also needs normalizing to match between | ||
| the Costs and Prices transforms, because MCA cost rows may carry a full ARM billing-profile path where the | ||
| Prices transform normalizes to a bare ID -- is intentionally NOT included here. It wasn't raised or needed | ||
| in the reporter-confirmed #2176 investigation, and normalizing away the ARM path may not be the desired | ||
| fix (Prices' bare-ID form could itself be a Price Sheet export limitation rather than the canonical shape). | ||
| Needs review before changing. | ||
|
|
||
| Per-row behavioral coverage lives in the executable harness | ||
| Tests/assets/ReservationPriceBackfillKey.kql (PASS = 0 returned rows on any Kusto database). | ||
| #> | ||
|
|
||
| Describe 'HubsReservationPriceBackfill' { | ||
|
|
||
| BeforeDiscovery { | ||
| $repoRoot = (Resolve-Path "$PSScriptRoot/../../../..").Path | ||
| $scriptsPath = Join-Path $repoRoot 'src/templates/finops-hub/modules/Microsoft.FinOpsHubs/Analytics/scripts' | ||
| $ingestionFiles = @('IngestionSetup_v1_0.kql', 'IngestionSetup_v1_2.kql') | ForEach-Object { | ||
| @{ Name = $_; FullName = (Join-Path $scriptsPath $_) } | ||
| } | ||
| } | ||
|
|
||
| BeforeAll { | ||
| $repoRoot = (Resolve-Path "$PSScriptRoot/../../../..").Path | ||
| $harnessPath = Join-Path $repoRoot 'src/powershell/Tests/assets/ReservationPriceBackfillKey.kql' | ||
| } | ||
|
|
||
| Context 'Missing-price gate' { | ||
|
|
||
| It 'Should not require x_SkuOfferId to attempt the backfill: <Name>' -ForEach $ingestionFiles { | ||
| $content = Get-Content -Path $FullName -Raw | ||
| $content.Contains('isnotempty(x_SkuMeterId) and isnotempty(x_SkuOfferId)') | Should -BeFalse -Because 'x_SkuOfferId is EA-only and blank for every MCA row (cost and price side alike, confirmed in #2176); gating on it excludes every MCA row from the price backfill before the join is even attempted (#1769, #2176)' | ||
| } | ||
|
|
||
| It 'Should still require x_SkuMeterId to attempt the backfill: <Name>' -ForEach $ingestionFiles { | ||
| $content = Get-Content -Path $FullName -Raw | ||
| $content.Contains('isnotempty(x_SkuMeterId)') | Should -BeTrue -Because 'the meter ID is still required to identify the on-demand price to recover' | ||
| } | ||
|
|
||
| It 'Should leave x_SkuOfferId in the reservation price lookup key: <Name>' -ForEach $ingestionFiles { | ||
| $content = Get-Content -Path $FullName -Raw | ||
| $content.Contains('x_SkuMeterId, x_SkuOfferId))') | Should -BeTrue -Because 'the key is unchanged by this fix: for MCA x_SkuOfferId is blank on both sides (a no-op suffix), and for EA it keeps differentiating rows (e.g. Production vs Dev/Test offers) as before' | ||
| } | ||
| } | ||
|
|
||
| Context 'Equivalence harness' { | ||
|
|
||
| It 'Should have the reservation price backfill key harness asset' { | ||
| Test-Path $harnessPath | Should -BeTrue | ||
| } | ||
|
|
||
| It 'Should assert expected divergence per row' { | ||
| $harness = Get-Content -Path $harnessPath -Raw | ||
| $harness | Should -Match '\| where \(old_matches != new_matches\) != expectedDivergent' -Because 'the harness must return only rows that violate the recorded old-vs-new expectation' | ||
| } | ||
|
|
||
| It 'Should cover an MCA blank offer ID case' { | ||
| $harness = Get-Content -Path $harnessPath -Raw | ||
| $harness | Should -Match 'MCA' -Because 'the fixture set must exercise the blank-offer-ID case that silently broke MCA reservation pricing' | ||
| } | ||
|
|
||
| It 'Should cover an EA offer ID differentiation case for regression safety' { | ||
| $harness = Get-Content -Path $harnessPath -Raw | ||
| $harness | Should -Match 'EA' -Because 'EA already matched before the fix; the fixture set must prove the fix does not regress it, including the case where offer ID legitimately differentiates two rows' | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| //====================================================================================================================== | ||
| // Reservation price backfill gate regression harness (#1769 / #2176) | ||
| // | ||
| // Verifies, per row, whether a cost row backfills its list/contracted price from the Prices table, before and after | ||
| // the fix. A row backfills only if (a) the missing-price gate lets it attempt the join, and (b) its | ||
| // tmp_ReservationPriceLookupKey matches a price row's key: | ||
| // | ||
| // old gate: isnotempty(x_SkuMeterId) and isnotempty(x_SkuOfferId) | ||
| // new gate: isnotempty(x_SkuMeterId) -- x_SkuOfferId requirement dropped | ||
| // | ||
| // key (unchanged by this fix): tolower(strcat(x_BillingProfileId, month, x_SkuMeterId, x_SkuOfferId)) | ||
| // | ||
| // Confirmed against reporter data in #2176: for MCA, x_SkuOfferId is blank on BOTH the cost row (FOCUS | ||
| // pass-through) and the price row (Price Sheet pass-through) -- it was never a key-matching problem, only a | ||
| // gate problem. Relaxing the gate lets the (still offer-ID-suffixed) key match, because the blank offer ID is | ||
| // an identical no-op suffix on both sides. For EA, x_SkuOfferId stays populated and continues to differentiate | ||
| // rows sharing a profile/meter (e.g. Production vs Dev/Test offers) exactly as before. | ||
| // | ||
| // NOTE: this harness intentionally does not model x_BillingProfileId format (ARM path vs bare ID). Whether the | ||
| // Costs and Prices transforms need to agree on that format is a separate, unconfirmed question -- see the test | ||
| // file docstring. | ||
| // | ||
| // How to run: paste into any Kusto database (ADX or Fabric eventhouse; no table access required). | ||
| // - PASS = the query returns 0 rows. | ||
| // | ||
| // expectedDivergent = true rows are the intended behavior change: MCA rows that now backfill after the fix. | ||
| // expectedDivergent = false rows assert everything else is unchanged: EA rows backfilled before and still do | ||
| // (including where offer ID legitimately differentiates two rows), and rows with no corresponding price | ||
| // (different meter/profile) stay unmatched either way. | ||
| //====================================================================================================================== | ||
|
|
||
| let cases = datatable( | ||
| label:string, | ||
| billingProfileId:string, | ||
| priceBillingProfileId:string, | ||
| meterId:string, | ||
| priceMeterId:string, | ||
| offerId:string, // cost-side x_SkuOfferId; blank for MCA (mapped from OfferId, which MCA exports never populate) | ||
| priceOfferId:string, // price-side x_SkuOfferId; blank for MCA (pass-through from the Price Sheet export) | ||
| expectedDivergent:bool | ||
| ) | ||
| [ | ||
| // EA: offer ID populated on both sides -- gate passes and key matches before; must still backfill after. | ||
| 'EA offer ID present, matching meter', 'ea-profile-1', 'ea-profile-1', 'meter-1', 'meter-1', 'OFFER-EA-1', 'OFFER-EA-1', false, | ||
| // EA: same profile and meter, but genuinely different offer IDs (e.g. Production vs Dev/Test) -- the key must | ||
| // keep separating these both before and after the fix; confirms leaving x_SkuOfferId in the key still works. | ||
| 'EA same profile/meter, different offer (Prod vs Dev/Test)', 'ea-profile-1', 'ea-profile-1', 'meter-1', 'meter-1', 'OFFER-EA-PROD', 'OFFER-EA-DEVTEST', false, | ||
| // MCA: offer ID blank on both cost and price rows (confirmed in #2176). Old gate excludes the row outright; | ||
| // new gate passes and, since the blank offer ID is an identical no-op suffix on both sides, the key matches. | ||
| 'MCA blank offer ID on both sides', 'mca-profile-1', 'mca-profile-1', 'meter-2', 'meter-2', '', '', true, | ||
| // Genuinely different meter within the same MCA profile: must stay unmatched under both old and new (gate | ||
| // failure under old, key mismatch under new) -- proves the fix does not backfill unrelated rows. | ||
| 'MCA different meter, same profile', 'mca-profile-1', 'mca-profile-1', 'meter-2', 'meter-3', '', '', false, | ||
| // Genuinely different billing profile: must not match under either, even post-fix. | ||
| 'EA different profile entirely', 'ea-profile-1', 'ea-profile-2', 'meter-1', 'meter-1', 'OFFER-EA-1', 'OFFER-EA-1', false, | ||
| ] | ||
| // old: gate requires a non-empty offer ID | ||
| | extend old_gate = isnotempty(meterId) and isnotempty(offerId) | ||
| | extend old_costKey = tolower(strcat(billingProfileId, meterId, offerId)) | ||
| | extend old_priceKey = tolower(strcat(priceBillingProfileId, priceMeterId, priceOfferId)) | ||
| | extend old_matches = old_gate and old_costKey == old_priceKey | ||
| // new: gate drops the offer ID requirement; key itself is unchanged | ||
| | extend new_gate = isnotempty(meterId) | ||
| | extend new_costKey = tolower(strcat(billingProfileId, meterId, offerId)) | ||
|
Collaborator
There was a problem hiding this comment.
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 |
||
| | extend new_priceKey = tolower(strcat(priceBillingProfileId, priceMeterId, priceOfferId)) | ||
| | extend new_matches = new_gate and new_costKey == new_priceKey | ||
| | where (old_matches != new_matches) != expectedDivergent | ||
| | project label, old_matches, new_matches, expectedDivergent | ||
| | order by label asc | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two "must cover" assertions can never fail.
Should -Match 'MCA'here andShould -Match 'EA'at line 74 run against the whole harness file, and both strings occur in its comment header —MCAat lines 16 and 29,EAat lines 19 and 30 — all well above thedatatable. 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-fixisnotempty(x_SkuMeterId) and isnotempty(x_SkuOfferId), so it passes against pre-fix and post-fix code alike.