Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify FOCUS conversion instructions #1153

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

flanakin
Copy link
Collaborator

πŸ› οΈ Description

Update FOCUS conversion docs to clarify cost and unit handling.

Fixes #1125

πŸ“‹ Checklist

πŸ™‹β€β™€οΈ 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)
  • ➑️ Will add log in a future PR (feature branch PRs only)
  • ❎ Log not needed (small/internal change)

πŸ“– Did you update documentation?

  • βœ… Public docs in docs (required for dev)
  • βœ… Internal dev docs in src (required for dev)
  • ➑️ Will add docs in a future PR (feature branch PRs only)
  • ❎ Docs not needed (small/internal change)

Copy link

@brianwyka brianwyka left a comment

Choose a reason for hiding this comment

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

I've commented on some unchanged docs as well. Hope this is okay!

@@ -57,35 +62,33 @@ The following mapping is assuming you have all amortized cost rows and only comm
| CommitmentDiscountType | BenefitId | If BenefitId contains "/microsoft.capacity/" (case-insensitive), `Reservation`; if contains "/microsoft.billingbenefits/", `Savings Plan`; otherwise, null |
| ConsumedQuantity | Quantity | If ChargeType == "Usage", then Quantity; otherwise, null |
| ConsumedUnit | UnitOfMeasure | If ChargeType == "Usage", then map using [Pricing units data file](../../_reporting/data/README.md#-pricing-units); otherwise, null |
| ContractedCost | UnitPrice * Quantity | Map UnitOfMeasure using [Pricing units data file](../../_reporting/data/README.md#-pricing-units) and divide Quantity by the PricingBlockSize |
| ContractedCost | UnitPrice * Quantity / x_PricingBlockSize (lookup) | Map UnitOfMeasure to x_PricingBlockSize using [Pricing units data file](../../_reporting/data/README.md#-pricing-units) |

Choose a reason for hiding this comment

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

x_PricingBlockSize is the column name in FOCUS dataset, but inside the reference data CSV file, it is the column name is PricingBlockSize.

Copy link
Collaborator Author

@flanakin flanakin Nov 23, 2024

Choose a reason for hiding this comment

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

Given the mapping doesn't happen in this column, what do you think about only calling out the division here and leaving the mapping for the x_PricingBlockSize column?

Suggested change
| ContractedCost | UnitPrice * Quantity / x_PricingBlockSize (lookup) | Map UnitOfMeasure to x_PricingBlockSize using [Pricing units data file](../../_reporting/data/README.md#-pricing-units) |
| ContractedCost | UnitPrice * Quantity / focus:x_PricingBlockSize | Note that x_PricingBlockSize requires a mapping. See column notes for details. |

| ListCost | EA: Not available<br>MCA: PaygCostInBillingCurrency | None |
| ListUnitPrice | EA: PayGPrice<br>MCA: PayGPrice \* ExchangeRate | None |
| PricingCategory | PricingModel | If "OnDemand", then `Standard`; if "Spot", then `Dynamic`; if "Reservation" or "Savings Plan", then `Committed`; otherwise, null |
| PricingQuantity | Quantity | Map UnitOfMeasure using [Pricing units data file](../../_reporting/data/README.md#-pricing-units) and divide Quantity by the PricingBlockSize<sup>2</sup> |
| PricingUnit | UnitOfMeasure | Map using [Pricing units data file](../../_reporting/data/README.md#-pricing-units) |
| PricingQuantity | Quantity / x_PricingBlockSize (lookup) | Map UnitOfMeasure to x_PricingBlockSize using [Pricing units data file](../../_reporting/data/README.md#-pricing-units)<sup>1</sup> |

Choose a reason for hiding this comment

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

Same as before, please use PricingBlockSize.

Copy link
Collaborator Author

@flanakin flanakin Nov 23, 2024

Choose a reason for hiding this comment

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

Same comment as before...

Suggested change
| PricingQuantity | Quantity / x_PricingBlockSize (lookup) | Map UnitOfMeasure to x_PricingBlockSize using [Pricing units data file](../../_reporting/data/README.md#-pricing-units)<sup>1</sup> |
| PricingQuantity | Quantity / focus:x_PricingBlockSize | Note that x_PricingBlockSize requires a mapping. See column notes for details. |

| ProviderName | `Microsoft` | None |
| PublisherName | PublisherName | None |
| RegionId | focus:RegionName | Lowercase and remove spaces |
| RegionName | ResourceLocation | Map using [Regions data file](../../_reporting/data/README.md#-regions)<sup>3</sup> |
| RegionName | ResourceLocation | Map ResourceLocation to RegionName using [Regions data file](../../_reporting/data/README.md#-regions)<sup>2</sup> |

Choose a reason for hiding this comment

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

Clarify that the OriginalValue column in the reference data file is what is used to map from ResourceLocation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch! Do you think this is clear enough? πŸ€”

Suggested change
| RegionName | ResourceLocation | Map ResourceLocation to RegionName using [Regions data file](../../_reporting/data/README.md#-regions)<sup>2</sup> |
| RegionName | ResourceLocation | Map ResourceLocation (OriginalValue) to RegionName using [Regions data file](../../_reporting/data/README.md#-regions)<sup>2</sup> |

| ServiceCategory | ResourceType | Map using [Services data file](../../_reporting/data/README.md#-services) |
| ServiceName | ResourceType | Map using [Services data file](../../_reporting/data/README.md#-services) |
| ResourceType | SingularDisplayName (lookup) | Map ResourceType to SingularDisplayName using [Resource types data file](../../_reporting/data/README.md#-resource-types) |
| ServiceCategory | ServiceCategory (lookup) | Map ResourceType to SerivceCategory using [Services data file](../../_reporting/data/README.md#-services) |

Choose a reason for hiding this comment

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

I believe for the Services reference data file, we need to map based on ConsumedService and ResourceType, based on this documentation - https://microsoft.github.io/finops-toolkit/data#%EF%B8%8F-services. It's not unique just by ResourceType.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes and no. I need to go look at the updated mapping we use internally, but there are only 1 or 2 services that need a ConsumedService mapping. The rest are all ResourceType. I have a task on my backlog to merge Services and ResourceTypes and just call out the exceptions separately. Any thoughts on how you'd prefer to see that here?

| ServiceName | ResourceType | Map using [Services data file](../../_reporting/data/README.md#-services) |
| ResourceType | SingularDisplayName (lookup) | Map ResourceType to SingularDisplayName using [Resource types data file](../../_reporting/data/README.md#-resource-types) |
| ServiceCategory | ServiceCategory (lookup) | Map ResourceType to SerivceCategory using [Services data file](../../_reporting/data/README.md#-services) |
| ServiceName | ServiceName (lookup) | Map ResourceType to SerivceName using [Services data file](../../_reporting/data/README.md#-services) |

Choose a reason for hiding this comment

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

Same here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See previous comment

docs/_docs/focus/convert.md Show resolved Hide resolved
| PricingQuantity | Quantity | Map UnitOfMeasure using [Pricing units data file](../../_reporting/data/README.md#-pricing-units) and divide Quantity by the PricingBlockSize<sup>2</sup> |
| PricingUnit | UnitOfMeasure | Map using [Pricing units data file](../../_reporting/data/README.md#-pricing-units) |
| PricingQuantity | Quantity / x_PricingBlockSize (lookup) | Map UnitOfMeasure to x_PricingBlockSize using [Pricing units data file](../../_reporting/data/README.md#-pricing-units)<sup>1</sup> |
| PricingUnit | DistinctUnits (lookup) | Map UnitOfMeasure to DistinctUnits using [Pricing units data file](../../_reporting/data/README.md#-pricing-units) |
| ProviderName | `Microsoft` | None |
| PublisherName | PublisherName | None |
| RegionId | focus:RegionName | Lowercase and remove spaces |

Choose a reason for hiding this comment

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

Here, it mentions focus:RegionName. Should the value also be looked up from the Regions reference data file via the ResourceLocation value and mapped to RegionId?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could. The reason I used the RegionName reference is because doing another lookup takes more time. This is just faster and aligns with the general guidance we have from the ARM team about how to handle location IDs.

Choose a reason for hiding this comment

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

How does one obtain focus:RegionName? It's not a column. Confused on that.

| ProviderName | `Microsoft` | None |
| PublisherName | PublisherName | None |
| RegionId | focus:RegionName | Lowercase and remove spaces |
| RegionName | ResourceLocation | Map using [Regions data file](../../_reporting/data/README.md#-regions)<sup>3</sup> |
| RegionName | ResourceLocation | Map ResourceLocation to RegionName using [Regions data file](../../_reporting/data/README.md#-regions)<sup>2</sup> |
| ResourceId | ResourceId | None |
| ResourceName | ResourceName | None |

Choose a reason for hiding this comment

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

I believe this can be obtained from the ResourceId, but let me know if I am wrong:

regexp_extract(ResourceId, '.*/([^/]+)$', 1)

Take the last segment of the / delimited value.

Copy link
Collaborator Author

@flanakin flanakin Nov 23, 2024

Choose a reason for hiding this comment

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

It can, but it's not that simple. Nested resources technically have a name that includes all levels in the resource hierarchy. So a SQL db (under a server) would be "MyServer/MyDatabase".

I forgot that this isn't in the MCA dataset today πŸ€” Doing the "right" thing is a bit onerous and probably wouldn't be clear here. If I remember correctly from doing this in KQL, it requires a few lines of code. Your approach is simpler, but I hate to rely on regular expressions. What do you think about this:

Suggested change
| ResourceName | ResourceName | None |
| ResourceName | EA: ResourceName<br>MCA: split(ResourceId, "/")[-1] | Azure resource names include multiple levels (e.g., "SqlServerName/SqlDbName"), which requires more processing. This is a simplified approach to only use the last, most-specific segment. |

Not sure if the [-1] reference will be clear to everyone since I'm only aware of 2 platforms that use that πŸ€” Maybe the text is good enough? (Although that should probably be in a footnote and not in the "transform" column.)

docs/_docs/focus/convert.md Outdated Show resolved Hide resolved
@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention πŸ‘‹ Issue or PR needs to be reviewed by the author or it will be closed due to no activity and removed Needs: Review πŸ‘€ PR that is ready to be reviewed labels Nov 22, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Review πŸ‘€ PR that is ready to be reviewed and removed Needs: Attention πŸ‘‹ Issue or PR needs to be reviewed by the author or it will be closed due to no activity labels Nov 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Micro PR πŸ”¬ Very small PR that should be especially easy for newcomers Needs: Review πŸ‘€ PR that is ready to be reviewed Skill: Documentation Documentation updates Tool: FinOps guide Implementing FinOps guide
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Documentation Improvement] Convert Cost Management data to FOCUS
3 participants