Skip to content

[Open Data] Fix ISF ratio scale, region coverage and culture handling, and keep retired-SKU reservations visible - #2308

Open
Roland Krummenacher (RolandKrummenacher) wants to merge 12 commits into
devfrom
RolandKrummenacher/isf-legacy-floor
Open

[Open Data] Fix ISF ratio scale, region coverage and culture handling, and keep retired-SKU reservations visible#2308
Roland Krummenacher (RolandKrummenacher) wants to merge 12 commits into
devfrom
RolandKrummenacher/isf-legacy-floor

Conversation

@RolandKrummenacher

@RolandKrummenacher Roland Krummenacher (RolandKrummenacher) commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes #2300 and #2309. Three parts: the reported symptom is fixed in the consumer, and the generator defects the investigation turned up are fixed in the data pipeline.

The published dataset goes from 1,353 to 1,363 SKUs — no row removed, ten added by a wider region sweep. Schema unchanged at three columns.

Part 1 — Reservations on retired SKUs disappeared from the reports

The reservation workbooks join the ISF open data with kind=inner. That data comes from the Catalogs API, which only returns SKUs you can still purchase — so an active reservation on a size Azure no longer sells has no row to match and is dropped from the report entirely. Not shown at zero utilization: absent. The customer keeps paying for a reservation the tool stops mentioning.

AzureOptimizationReservationsUsageV1_CL
| where ProvisioningState_s in ('Succeeded','Expiring') and todatetime(ExpiryDate_s) > now()
...
| join kind=inner ( ISFGroups ) on $left.SKUName_s == $right.ArmSKUName

All seven ISF joins in reservations-usage.json become leftouter, with the group and ratio falling back to the SKU itself:

| extend ISFGroup = coalesce(ISFGroup, SKUName_s), Ratio = coalesce(Ratio, 1.0)

The coalesce is not optional. A plain leftouter is worse than today: Ratio and ISFGroup come back null, TotalReservedQuantity_s * Ratio becomes null, and summarize ... by ISFGroup collapses every unmatched reservation into a single null bucket with an empty utilization column.

Ratio = 1 is correct here. Util7Days_s comes from Azure and already accounts for instance size flexibility, so the percentage is right as-is. The ratio only converts between group units, a no-op for a group of one.

reservations-potential.json and benefits-simulation.json keep kind=inner. They model what to buy, and there restricting to purchasable SKUs is correct.

Known limitation. Drilldowns fall back to the consumed size while the overview falls back to the reserved size. For a reservation on Standard_DS4_v2 running DS3_v2 underneath, those don't meet — which sizes belong to a retired group is exactly what the ISF data no longer carries. A drilldown on an unmatched reservation therefore shows usage of the identical SKU only. Better than nothing, not a full restoration.

Part 2 — Three generator defects

Region coverage. The sweep used a hardcoded list of 26 regions, so SKUs that launch in only a handful of regions never entered the dataset, and the list had to be hand-maintained. It now enumerates every physical region from the ARM locations API — 63 today. Note regionType is nested under metadata, not at the top level, and 46 of the 109 entries are logical groupings (global, unitedstates, europe) that are not valid catalog scopes. An empty enumeration fails the run rather than sweeping a partial set. Measured: 7.9 minutes for 63 regions × 3 reserved types, against a 60-minute timeout.

Culture handling. Ratios were parsed and written with the current culture:

culture TryParse("2.1") Export-Csv writes
en-US 2.1 "2.1"
de-DE 21 "2,1"
fr-CH fails, row dropped "2,1"

CI runs on ubuntu-latest and was never affected, but the README documents running the generator by hand, so a contributor in a comma-decimal locale would have published a corrupt file — "2,1" breaks the externaldata(... Ratio:double) joins and both Power BI partitions typed as number. Both parse sites and the write path now pin InvariantCulture.

Zero ratios. The API returns azure_redis_cache_isolated_i100 with a ratio of 0, and benefits-simulation.json divides by Ratio. Non-positive ratios are dropped, with a unit test asserting the invariant on the published file.

Part 3 — Ratios were on the wrong scale

The Catalogs API leaves ratios unnormalized: only 52 of its 211 groups have a smallest SKU of 1. Microsoft's documentation calls this outBS Series starts at 0.25, Ddsv5 Series at 2 — and publishes a normalization step of its own. The retired isfratioblob.csv this dataset replaced was normalized, so #2222 silently changed the scale under every consumer.

That matters because the Optimization Engine converts quantities into units of a group's smallest SKU — AvgRIsUsedInSmallestRatio is the variable's own name — and multiplying by a ratio that doesn't start at 1 inflates the absolute figures by a per-group constant. Utilization percentages carry the factor on both sides and were never wrong; the displayed quantities were.

It also makes Part 1 coherent. An unmatched reservation falls back to Ratio = 1, which is implicitly "smallest SKU = 1"; next to an unnormalized group that would put two rows of one table in different units.

Each group is now normalized. -Normalize is replaced by -Raw, so the switch names the departure from the default rather than the default itself.

Parity with the retired file is close but not total, and the gap is worth understanding rather than glossing. Of the 1,291 rows the two files share, 1,223 now agree. The 68 that don't split cleanly:

groups rows why
Different anchor 12 52 The API no longer returns the historically smallest SKU, so normalization anchors on a different one. BS Series High Memory is down to Standard_B20ms alone, which normalizes to 1 rather than 40.2 — internally consistent, since no other SKU of that group remains.
Different proportions 11 16 The API and the retired file disagree on the proportions themselves, not the unit. All dedicated host groups, 5-10% apart. The API is the newer source.

Measured impact on consumers: Power BI is unaffectedx_CommitmentDiscountFlexRatio appears in no DAX measure and no report visual, only in linguistic metadata; the visuals use the flexibility group. The Optimization Engine's absolute quantities change, which is the point.

What this deliberately does not do

An earlier revision backfilled 1,220 retired SKUs from the ratio file the API replaced. That was dropped. Those rows would be frozen forever — nothing can refresh or correct them once the source blob is gone — inside a dataset whose contract is a weekly refresh from the authoritative API. Fixing the consumer avoids carrying dead reference data.

Fixes #2309 as part of this.

Test plan

  • Invoke-Pester ./src/powershell/Tests/Unit/* ./src/powershell/Tests/Lint/*6,010 passed, 0 failed (4 pre-existing skips)
  • Generator run end-to-end against the Catalogs API: 63 regions, 7.9 min
  • Verified against v15: 0 rows removed, 10 added; ratios rescaled per group by normalization
  • Verified against the retired isfratioblob.csv: 1,223 of 1,291 shared rows agree, the rest classified above
  • Every published group has a smallest ratio of 1, asserted by a unit test
  • 22 unit tests for the generator, including all four region-enumeration paths and the de-DE round trip
  • Open Reservations usage against a subscription holding a reservation on a retired size and confirm it now appears with a correct utilization percentage
  • Confirm reservations on current sizes are unchanged — same groups, quantities and percentages

The workbook change is reasoned, not verified. No test in this repo executes workbook KQL, and I have no Optimization Engine workspace with reservation data to run it against. The join semantics and the null behaviour that motivates the coalesce want confirming by someone who does.

🤖 Generated with Claude Code

The Catalogs API only returns SKUs that can still be purchased, so the
instance size flexibility dataset dropped every retired series that active
reservations still cover (Av2, D, DS, Dv2, Dv3, Ev3, F, G, H, LS, NC, NV
and others). The Optimization Engine joins ISF with kind=inner, so those
reservations were silently missing from its reports.

- Backfill the retired groups from the ratio file the API replaced:
  1,353 -> 2,573 SKUs and 211 -> 318 flexibility groups. Purely additive
  against v15 -- no row removed and no published ratio changed.
- Merge additively instead of overwriting, so a SKU the API stops
  returning is carried forward rather than dropped, and fail the run if
  the dataset would shrink.
- Enumerate every physical region from the ARM locations API (63) instead
  of sweeping a hardcoded list of 26, which omitted SKUs that launch in
  only a handful of regions. regionType is nested under metadata, and the
  logical groupings it marks are not valid catalog scopes.
- Reconcile the ratio scale. The retired file normalized each group so its
  smallest SKU was 1 while the API reports vCPU counts, and because Azure
  retires individual sizes the API covers only part of 44 of the 204
  shared groups, so a per-SKU merge left single groups holding both units.
  Backfilled rows were converted onto the API scale once, and
  Assert-IsfScale now fails the run if a group drifts off it again.
- Drop non-positive ratios. The API returns one, and the benefits
  simulation workbook divides by Ratio.

Fixes #2300

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@microsoft-github-policy-service microsoft-github-policy-service Bot added the Needs: Review 👀 PR that is ready to be reviewed label Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

⚠️ Open Data CI task failed: https://github.com/microsoft/finops-toolkit/actions/runs/34264566963

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The generator uses culture-dependent [double]::TryParse(...) for ratios, which can misparse decimal values in non-dot locales and jeopardize dataset correctness.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes gaps in the Instance Size Flexibility (ISF) open-data pipeline by making the generated dataset additive (carrying forward retired-but-still-relevant SKUs), expanding region coverage via ARM locations enumeration, and enforcing invariants around ratio scale and non-positive ratios to prevent downstream consumer errors.

Changes:

  • Merge Catalogs API results over the previously published ISF CSV keyed by ArmSkuName, and fail the run if the dataset would shrink.
  • Enumerate all physical Azure regions from the ARM locations API when -Location is not provided, replacing the hardcoded region list.
  • Drop non-positive ratios, add scale-consistency validation (Assert-IsfScale), expand unit tests, and update documentation/changelog to reflect the new behavior.
File summaries
File Description
src/scripts/Update-InstanceSizeFlexibility.ps1 Implements additive merge, ARM-based physical region enumeration, ratio filtering, and scale validation logic.
src/powershell/Tests/Unit/Update-InstanceSizeFlexibility.Tests.ps1 Adds unit tests for additive merge behavior, zero-ratio filtering, scale mismatch detection, and region enumeration paths.
src/open-data/README.md Updates dataset behavior documentation (additive merge, region coverage, ratio scale notes, zero-ratio handling).
src/open-data/InstanceSizeFlexibility.csv Backfills and expands the published ISF dataset with retired SKUs and additional groups.
docs-mslearn/toolkit/open-data.md Updates Microsoft Learn documentation to reflect additive dataset behavior, region coverage, and ratio scale handling.
docs-mslearn/toolkit/changelog.md Adds changelog entries describing the fixes and dataset growth.
.github/workflows/opendata-instance-size-flexibility.yml Updates workflow PR body text to align with additive/merge semantics and expected diffs.
Review details

Suppressed comments (2)

src/scripts/Update-InstanceSizeFlexibility.ps1:337

  • This ratio parse also defaults to the current culture. Since the published CSV uses . for decimals, cultures that expect , can misparse or reject values, which would affect the duplicate-group detection and merge inputs. Use InvariantCulture for deterministic behavior.
    {
        $ratio = 0.0
        if (-not [double]::TryParse($row.Ratio, [ref]$ratio)) { continue }
        if ($ratio -le 0) { continue }

src/scripts/Update-InstanceSizeFlexibility.ps1:346

  • Same culture-dependent parsing issue here: using the current culture can mis-handle decimal ratios in the CSV (which are written with .). Use InvariantCulture to avoid silently changing which rows are retained during the merge.
    foreach ($row in $rows)
    {
        $ratio = 0.0
        if (-not [double]::TryParse($row.Ratio, [ref]$ratio)) { continue }
        if ($ratio -le 0) { continue }
  • Files reviewed: 7/7 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/scripts/Update-InstanceSizeFlexibility.ps1 Outdated
Comment thread docs-mslearn/toolkit/changelog.md Outdated
@RolandKrummenacher

Copy link
Copy Markdown
Collaborator Author

The ratio-scale follow-up flagged in the description is now tracked separately in #2309, so it doesn't hold up this PR. That one is about the Optimization Engine's math; this PR leaves every published ratio unchanged.

Roland Krummenacher and others added 2 commits September 8, 2026 20:56
The ratios were described as absolute vCPU counts. That holds for many
D/E-series groups by coincidence -- their smallest SKU has 2 vCPUs, so
normalizing against it yields a factor of 2 -- but not in general:

- 799 of 1,164 parseable VM SKUs match their vCPU count; 365 do not.
- Standard_B2as_v2 has 2 vCPUs and a ratio of 16; Standard_B16als_v2 has
  16 and a ratio of 113.4.
- azure_managed_redis_balanced_b1000 has a ratio of 1248.
- 156 of the returned ratios are not integers.

What is actually verifiable is that the API leaves ratios unnormalized:
only 52 of its 211 groups start at 1, against 432 of 433 in the retired
files. That is what the merge and the scale check rely on, so the wording
now states it and no longer names a unit.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@RolandKrummenacher

Copy link
Copy Markdown
Collaborator Author

Depends on merge order with #2298, which adds src/powershell/Private/Get-OpenDataInstanceSizeFlexibility.ps1 — generated from this CSV at its pre-change 1,353 rows.

This PR takes the CSV to 2,573 rows, so that generated file has to be rebuilt. It is deliberately not included here: both PRs would otherwise add the same new file and collide.

Plan: let #2298 merge first, then merge dev into this branch and run Build-OpenData.ps1 -PowerShell -Name InstanceSizeFlexibility so the regenerated file lands with the data it describes.

Note that Open Data CI passing on this PR does not mean the file was regenerated — on dev the workflow uses git commit -a, which skips the still-untracked file. #2298 fixes that with git add -A.

Hub KQL is unaffected: Build-OpenData.ps1 -Hubs uses an allowlist that excludes this dataset, and the committed OpenDataFunctions.kql contains no ISF data.

Roland Krummenacher and others added 2 commits September 8, 2026 21:40
Ratios were parsed and written with the current culture. On a comma-decimal
machine that corrupts the dataset in both directions:

- de-DE parses "2.1" as 21, so a ratio is silently ten times too large.
- fr-CH fails the parse outright, so the row is dropped.
- Export-Csv writes the value back as "2,1", which breaks every consumer:
  the Optimization Engine's externaldata(... Ratio:double) and the Power BI
  partitions typed as number.

CI runs on ubuntu-latest and is unaffected, but the README documents running
the generator by hand, so a contributor in a comma-decimal locale would
publish a corrupt file. Both parse sites and the write path now pin
InvariantCulture, with a regression test that drives the generator under
de-DE and asserts the round trip.

Also corrects the SKU count in the changelog: it still said 2,574 from
before the zero-ratio row was dropped, against 2,573 actually published.

Reported by Copilot review on #2308 (the parse side; the write side turned
up while verifying it).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… fixes

Reverts the backfill of retired SKUs and everything it required: the
additive merge, the shrink guard, Import-IsfCsv, Assert-IsfScale and the
one-time migration onto the API scale. The generator publishes exactly
what the Catalogs API returns again, and aborts without touching the file
if any scope fails.

The premise behind the backfill does not hold up. The dataset would have
carried 1,220 rows sourced from a file Microsoft has retired, frozen
forever because nothing can refresh or correct them, in a dataset whose
contract is a weekly refresh from the authoritative API. Reservations on
retired SKUs are better handled where they surface -- see #2300 for the
Optimization Engine side.

What remains are three independent defects, all of which concern the
purchasable SKUs the API does return:

- Region coverage: the sweep used a hardcoded list of 26 regions, so SKUs
  that launch in only a few regions were missing. It now enumerates every
  physical region from the ARM locations API (63 today). regionType is
  nested under metadata, and the logical groupings it marks are not valid
  catalog scopes.
- Culture: ratios were parsed and written with the current culture, so a
  comma-decimal machine read "2.1" as 21 or rejected it, and wrote "2,1"
  back into the published file.
- Zero ratios: the API returns one, and the benefits simulation workbook
  divides by Ratio.

The published dataset goes from 1,353 to 1,363 SKUs -- no row removed, ten
added by the wider region sweep.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@RolandKrummenacher Roland Krummenacher (RolandKrummenacher) changed the title [Open Data] Restore retired SKUs in instance size flexibility and reconcile the ratio scale [Open Data] Fix region coverage, culture handling and zero ratios in the instance size flexibility generator Sep 9, 2026
Roland Krummenacher and others added 2 commits September 9, 2026 18:07
The reservation workbooks join the instance size flexibility open data with
kind=inner. The Catalogs API that feeds it only returns SKUs that can still
be purchased, so an active reservation on a retired size has no row to match
and is dropped from the report entirely -- not shown at zero, absent. The
customer keeps paying for a reservation the tool stops mentioning.

Reservations run one or three years and outlive the sellability of their
size, so this is reachable in normal use.

All seven ISF joins in reservations-usage.json become leftouter, with the
group and ratio falling back to the SKU itself:

    | extend ISFGroup = coalesce(ISFGroup, SKUName_s), Ratio = coalesce(Ratio, 1.0)

Without that fallback a plain leftouter is worse than the current behaviour:
Ratio and ISFGroup come back null, every unmatched reservation collapses into
one null bucket, and the utilization column reads empty.

Ratio 1 is correct for these rows. Util7Days_s comes from Azure and already
accounts for flexibility, so the percentage is right; the ratio only converts
between group units, which is a no-op for a single-SKU group.

reservations-potential.json and benefits-simulation.json keep kind=inner.
They model what to buy, where restricting to purchasable SKUs is correct.

Also points the changelog entries for the culture and zero-ratio fixes at
this PR rather than #2300, which they have nothing to do with.

Note the workbook KQL is not executed by any test in this repo, so this
change is reasoned rather than verified.

Fixes #2300

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@RolandKrummenacher Roland Krummenacher (RolandKrummenacher) changed the title [Open Data] Fix region coverage, culture handling and zero ratios in the instance size flexibility generator [Open Data] Fix ISF generator defects and keep retired-SKU reservations visible in the Optimization Engine Sep 9, 2026
The Catalogs API leaves ratios unnormalized -- only 52 of its 211 groups
have a smallest SKU of 1, which Microsoft's documentation calls out (BS
Series starts at 0.25, Ddsv5 Series at 2) alongside a normalization step of
its own. The retired isfratioblob.csv this dataset replaced was normalized,
so #2222 silently changed the scale under every consumer.

That matters because the Optimization Engine converts quantities into units
of a group's smallest SKU -- AvgRIsUsedInSmallestRatio is the variable's
own name -- and multiplying by a ratio that doesn't start at 1 inflates the
absolute figures by a per-group constant. Utilization percentages carry the
factor on both sides and were never wrong; the displayed quantities were.

Normalizing loses nothing: ratios are only meaningful within their group,
and both forms carry identical proportions.

It also makes the leftouter fix in this PR coherent. An unmatched
reservation falls back to Ratio 1, which is implicitly "smallest SKU = 1";
next to an unnormalized group that would put two rows of one table in
different units.

Against the retired file, 1,223 of the 1,291 rows they share now agree.
The 68 that don't split cleanly:

- 12 groups / 52 rows where the API no longer returns the historically
  smallest SKU, so normalization anchors on a different one. BS Series High
  Memory is down to Standard_B20ms alone, which normalizes to 1 rather than
  40.2. Internally consistent, since no other SKU of that group remains.
- 11 groups / 16 rows, all dedicated host, where the API and the retired
  file disagree on the proportions themselves rather than the unit. The API
  is the newer source.

-Normalize is replaced by -Raw, so the switch names the departure from the
default rather than the default itself.

Fixes #2309

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@RolandKrummenacher Roland Krummenacher (RolandKrummenacher) changed the title [Open Data] Fix ISF generator defects and keep retired-SKU reservations visible in the Optimization Engine [Open Data] Fix ISF ratio scale, region coverage and culture handling, and keep retired-SKU reservations visible Sep 9, 2026
Comment thread docs-mslearn/toolkit/changelog.md Outdated
Comment thread src/optimization-engine/views/workbooks/reservations-usage.json

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description should be updated to remove the "Fixes #2309" statement, because it does not address the so called ratio scale issue.

- It covers Virtual Machines, Redis Cache, and Dedicated Host across every physical Azure region.
- `ArmSkuName` is unique across the file and can be used as a join key on its own.
- Ratios are the raw Microsoft values within each group (the smallest SKU isn't always `1`).
- Ratios are normalized so the smallest SKU in each flexibility group has a ratio of `1`. Compare ratios only within a flexibility group.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure whether generating a CSV whose ISF ratios are inconsistent with what Cost Management exports report in the RINormalizationRatio property of the AdditionalInfo column is a good idea.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not seeing the full picture, but once I address #2309 I'll have a better idea.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 [AI][Claude Code] PR Review

Summary: Strong, well-reasoned PR. The generator fixes (invariant culture, region enumeration, non-positive ratio drop, normalization) are correct and well tested, and the leftouter + coalesce pattern is the only safe form here — KQL strings have no null, so leftouter yields "" and coalesce (documented as "first non-null, or non-empty for string") handles both the string and the double case where isnull() would silently fail. Verified locally: 23 unit tests pass (the description says 22), all 3,593 lint tests pass, and the published CSV holds every claimed invariant — 1,363 rows, 213 groups, every group normalized to a smallest ratio of 1, ArmSkuName globally unique, no non-positive ratios, invariant decimal formatting. The Power BI claim also checks out: x_CommitmentDiscountFlexRatio appears only in linguistic metadata, in no DAX measure or visual.

One blocker: the fix stops three joins short of complete, and the reported symptom survives in one of them.

🚫 Blockers (1)

  1. reservations-usage.json has 10 ISF joins, not 7. Three were already leftouter before this PR and still have no coalesce guard — including "Unused Reservations over time (by VM count)" (line 1093), which multiplies by a null Ratio and drops retired-SKU reservations exactly as #2300 describes.

⚠️ Should fix (3)

  1. The Optimization Engine changelog section now has two separate - **Fixed** blocks, the new one placed before - **Added**.
  2. The instance size flexibility sample table in open-data.md shows rows that exist nowhere in the published file.
  3. leftouter widens the two ISF-grouped tables to every reservation type, with dead-end drilldowns — an undocumented scope change.

💡 Suggestions (1)

  1. The ARM locations call has no retry, unlike every Catalogs call.

Not verified, and I couldn't verify it either: no test in this repo executes workbook KQL, so the runtime behavior of these queries is reasoned, not run. Everything I checked above is static analysis plus the Pester suites and the published CSV.

Comment thread src/optimization-engine/views/workbooks/reservations-usage.json Outdated
Comment thread src/optimization-engine/views/workbooks/reservations-usage.json Outdated
Comment thread docs-mslearn/toolkit/changelog.md Outdated
Comment thread docs-mslearn/toolkit/open-data.md
Comment thread src/scripts/Update-InstanceSizeFlexibility.ps1 Outdated
… types

Addresses review feedback on #2308.

- Add the coalesce guard to the three ISF joins that were already leftouter
  before this PR and so never appeared in the diff. Without it, a reservation
  on a retired SKU still contributed nothing to "Unused Reservations over time
  (by VM count)" with Use ISF = Yes, collapsed into a single mislabeled series
  in the by-SKU cost chart, and showed a blank ISF group in the reservations
  grid. All ten ISF joins are now leftouter and guarded.
- Scope the unmatched-SKU fallback to the reserved resource types that have
  instance size flexibility, matching the types the ISF dataset covers. Types
  without it keep their prior behavior in the Use ISF = Yes views and remain
  fully listed in the Use ISF = No views.
- Extract Invoke-ArmRequest so region enumeration retries transient failures
  like every other ARM call in the generator. It is the first call the weekly
  run makes and a prerequisite for all of them, so a single 429 or 5xx failed
  the whole refresh before it fetched a single catalog page.
- Merge the duplicate Optimization Engine "Fixed" block, replace the instance
  size flexibility sample table with rows that exist in the file, and fold the
  new dataset's fix bullets into nested notes under "Added".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Conflict was ms.date in docs-mslearn/toolkit/changelog.md only, resolved to
today per AGENTS.md. Both sides' entries are kept: the FinOps hubs ADF trigger
fix from #2291 and the Optimization Engine and open data entries from this PR.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@RolandKrummenacher

Copy link
Copy Markdown
Collaborator Author

Review feedback addressed — ready for another look

Everything from the 9 Sep review is in, in 6fcf79f8, plus a dev merge in d4b8e31e. Six threads resolved, one left open deliberately. All checks green.

Michael Flanakin (@flanakin) — your approval on 9 Sep 20:19 predates the agent review you posted at 23:58 the same evening, so it doesn't cover the blocker or anything below. Re-approval would be appreciated.

What changed

Finding Resolution
🚫 Ten ISF joins, three unguarded coalesce guard added at 1060, 1093 and 1488. All ten are now leftouter + guarded. Ratio is only coalesced where it's actually referenced; 1060's 'Canceled RIs' branch is preserved, and a KQL comment there records why coalesce and not isnull
⚠️ innerleftouter widened scope Scoped. let IsfResourceTypes = dynamic(['VirtualMachines', 'RedisCache', 'DedicatedHost']) + where ResourceType in~ (IsfResourceTypes) in the two AzureOptimizationReservationsUsageV1_CL subqueries
⚠️ Duplicate - **Fixed** block Merged into the existing one; the section reads Added → Changed → Fixed
⚠️ Sample table rows don't exist Replaced with three Virtual Machines Dadsv6-series Linux rows, verbatim from the CSV in this PR, anchored at ratio 1 so the sample demonstrates the normalization note below it
💡 No retry on the first ARM call Extracted Invoke-ArmRequest; both call sites use it, message wording unchanged, new test covers a 503 during region enumeration
nit: don't document fixes to a new dataset The instance size flexibility - **Fixed** block is gone. Three nested bullets under Added now cover what differs from the retired isfratioblob.csv

The one judgment call worth a second look

I scoped the fallback rather than documenting the widening. Two things that weren't in the original analysis pushed it that way:

Nothing is hidden either way. Both affected grids sit in groups gated on UseISF == 'Yes'. Each has a twin gated on UseISF == 'No' — "Reservation Usage Details" and "Unused Reservations Details" — with no ISF join at all, already listing every reserved resource type. Scoping restores the pre-PR split rather than dropping anything from the workbook.

The dead-end drilldown is confirmed, not inferred. The Use ISF = No drilldown carries an explicit fallback for rows with no ServiceType:

| extend VMSize = tostring(parse_json(AdditionalInfo_s).ServiceType)
| extend ConsumedSize = iif(isnotempty(VMSize), VMSize, strcat(MeterSubCategory_s, ' ', MeterName_s))

That line exists because those rows are real. The ISF drilldown filters on ISFGroup == '{selectedISFGroup:value}' before reaching its copy of it, and ISFGroup is coalesce(ISFGroup, VMSize) — so for exactly those rows it's empty and matches nothing.

Hélder Pinto (@helderpinto)

Two things for you, since you're the only real-data check this PR has:

  • The scoping landed after the build you tested. The Use ISF = Yes views no longer pick up SQL Database / Cosmos DB / App Service reservations. The retired-VM-SKU path you verified is unchanged.
  • Your RINormalizationRatio thread is the one I left open on purpose, since you tied it to [AOE] Reservation workbooks show absolute quantities on the wrong ratio scale #2309. If the ratio scale here turns out to conflict with what Cost Management reports in AdditionalInfo, that's worth settling before this ships rather than after.

Verification

  • 833/833 Pester tests locally after the dev merge (24 generator including the new retry test, 445 KQL join-kind, 144 KQL operator, 220 ADF trigger)
  • Workbook JSON parses; prettier clean; PSScriptAnalyzer shows no new findings
  • dev merge conflict was ms.date only, resolved to today per AGENTS.md; both sides' changelog entries kept

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs: Review 👀 PR that is ready to be reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[AOE] Reservation workbooks show absolute quantities on the wrong ratio scale InstanceSizeFlexibility incomplete

5 participants