fix(hubs): stop price transform row fan-out and fix eligibility scoping #2246
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,16 +69,32 @@ Prices_transform_v1_2() | |
| | extend x_IngestionTime = ingestion_time() | ||
| ); | ||
| // | ||
| // Commitment discount eligibility per meter, used below. Sourced from the | ||
| // CommitmentDiscountEligibility open-data table (a snapshot of the Azure Retail Prices API) | ||
| // rather than derived from `prices` itself: Prices_raw can land as multiple parquet-snappy | ||
| // files per export, each triggering a SEPARATE update policy invocation (see #1625), so a | ||
| // meter's Reservation/SavingsPlan row and its Consumption row can be visible in different | ||
| // invocations. Deriving eligibility from `prices` only sees whichever rows happen to be in the | ||
| // current invocation; CommitmentDiscountEligibility is ingested as a whole table and doesn't | ||
| // have that gap. Dedupe with take_any() even though MeterId is expected to be unique in the | ||
| // source data -- defends against a future duplicate row silently fanning out the lookup below. | ||
| // MeterId is lowercased by the generator (Update-CommitmentDiscountEligibility.ps1); the | ||
| // pricesheet's MeterId/MeterID casing isn't documented/guaranteed, and `lookup on` only | ||
| // supports case-sensitive equality, so both sides are normalized to lowercase for the join. | ||
| let commitmentEligibility = CommitmentDiscountEligibility | ||
| | extend tmp_MeterId = tolower(MeterId) | ||
|
Collaborator
There was a problem hiding this comment. Not a problem — flagging pre-emptively so this doesn't get bounced on a false positive. This
|
||
| | summarize take_any(x_CommitmentDiscountSpendEligibility), take_any(x_CommitmentDiscountUsageEligibility) by tmp_MeterId; | ||
|
Collaborator
There was a problem hiding this comment. Blocking — this regresses Azure Government and Azure China hubs to 100% "Not Eligible".
Today's The description lists "unmatched meter defaults to Not Eligible" as preserved behavior, which is mechanically true, but that was a rare tail case before and becomes the only outcome for entire clouds. Options as I see them:
The first looks strictly better to me — it also covers the new-meter freshness gap you flagged in the description, since a meter the weekly refresh hasn't picked up yet would fall back to the pricesheet-derived answer instead of silently reading "Not Eligible". |
||
| // | ||
| // Copy list/base/contracted prices from on-demand SKUs | ||
| prices | ||
| | where x_SkuPriceType == 'SavingsPlan' | ||
| // If we use join, specify the shuffle key | ||
| // TODO: Compare join vs. lookup perf -- | join kind=leftouter hint.strategy=shuffle (prices | where x_SkuPriceType == 'Consumption' | summarize take_any(ListUnitPrice), take_any(ContractedUnitPrice), take_any(x_BaseUnitPrice) by tmp_SavingsPlanKey) on tmp_SavingsPlanKey | ||
| // The dimension side must be unique per tmp_SavingsPlanKey (meter+product+SKU+tier+offer, no region/currency): | ||
| // `distinct` over the price columns does not guarantee that when the same key has rows with | ||
| // different prices (e.g. multi-region exports), so it can fan out matching SavingsPlan rows. | ||
| // `summarize take_any(...) by tmp_SavingsPlanKey` guarantees exactly one row per key. | ||
| | lookup kind=leftouter (prices | where x_SkuPriceType == 'Consumption' | summarize take_any(ListUnitPrice), take_any(ContractedUnitPrice), take_any(x_BaseUnitPrice) by tmp_SavingsPlanKey) on tmp_SavingsPlanKey | ||
|
Collaborator
There was a problem hiding this comment. Substantive — this fixes the row count but can silently assign the wrong price. The fan-out is real and That matters because a hub can ingest pricesheets for multiple billing accounts and profiles into one Before: loud and detectable — row counts exceed The fix that matches the diagnosis is widening the key — adding If widening the key is out of scope here, I'd rather see that stated as a deliberate decision with a follow-up issue than left as an arbitrary pick. Minor, same line — the semi-join prefilter is gone. The old dimension side had | where x_SkuMeterId in ((prices | where x_SkuPriceType == 'SavingsPlan' | distinct x_SkuMeterId)) |
||
| | extend ListUnitPrice = coalesce(ListUnitPrice, ListUnitPrice1) | ||
| | extend ContractedUnitPrice = coalesce(ContractedUnitPrice, ContractedUnitPrice1) | ||
| | extend x_BaseUnitPrice = coalesce(x_BaseUnitPrice, x_BaseUnitPrice1) | ||
|
|
@@ -92,11 +108,12 @@ Prices_transform_v1_2() | |
| '' | ||
| ) | ||
| // | ||
| // Calculate commitment discount eligibility from the open-data snapshot; unmatched meters default to not eligible | ||
| | extend tmp_MeterId = tolower(x_SkuMeterId) | ||
| | lookup kind=leftouter (commitmentEligibility) on tmp_MeterId | ||
| | extend x_CommitmentDiscountSpendEligibility = iff(isnotempty(x_CommitmentDiscountSpendEligibility) and x_SkuPriceType != 'ReservedInstance', x_CommitmentDiscountSpendEligibility, 'Not Eligible') | ||
| | extend x_CommitmentDiscountUsageEligibility = coalesce(x_CommitmentDiscountUsageEligibility, 'Not Eligible') | ||
| | project-away tmp_MeterId | ||
| // | ||
| // TODO: Implement x_CommitmentDiscountNormalizedRatio | ||
| | extend x_CommitmentDiscountNormalizedRatio = real(null) | ||
|
|
||
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.
Blocking — local hubs will silently report every meter as "Not Eligible".
devgainedIngestionSetup_OpenDataExternal.kqlin #2187 ("Run FinOps hubs on your own hardware"), which merged after this branch forked — so it isn't visible in this diff. It's the local/emulator stand-in that populates the open-data tables, built into thefinops-hub-local-opendata.kqlrelease artifact via.build.configand consumed byInitialize-FinOpsHubLocal.ps1:181.This PR adds a fifth open-data table, creates it here, and populates it only through the new
Update CommitmentDiscountEligibility in ADXpipeline activity. Local hubs never run Data Factory. So the table exists, empty, the lookup inPrices_transform_v1_0/v1_2misses every meter, and both eligibility columns come out'Not Eligible'for 100% of rows — with no error anywhere.The fix is one line added to
IngestionSetup_OpenDataExternal.kql, mirroring the four already there:This will surface when you merge
devin to clear the changelog conflict, but it's worth catching deliberately rather than discovering the empty table later.Minor, same area — Fabric upgrade path.
ingestion_InitScriptsandingestion_VersionedScriptsare bothif (useAzure)(app.bicep:357/376), so Fabric users run these scripts by hand.Prices_transform_v1_2()now referencesCommitmentDiscountEligibility, so a Fabric user who reruns only the versioned script against an existing eventhouse hits a function-validation failure on a table that doesn't exist yet. Worth an explicit "run the infra script first" note in the upgrade guidance.