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

FOCUS #497: SkuPriceDetails column #503

Conversation

kk09v
Copy link
Contributor

@kk09v kk09v commented Jul 9, 2024

Introduction of KeyValueFormat columns SkuDetails and SkuPriceDetails to capture properties of a SKU ID and SKU Price ID, respectively.

Lead Maintainer: Karl

Documents:

kk09v added 6 commits July 1, 2024 00:21
* Added immutability clause per conversation on 20240703.
* Brought formatting in line with current draft of editorial guidelines
* Additional formatting changes per editorial guidelines
* Added immutability per discussion 20240709
* Updated formatting to align to current draft of editorial guidelines
More formatting per guidelines
@kk09v kk09v requested a review from a team as a code owner July 9, 2024 17:32
@kk09v
Copy link
Contributor Author

kk09v commented Jul 9, 2024

OPEN ITEMS:

  • Adding references and back links to definitions of related columns, properties, etc.
  • Populate appendix with sample of SkuPriceDetails.
  • Document conversation/reasoning in supporting content.
  • Review example JSON properties to ensure they would be relevant and are in the correct column (SkuDetails vs SkuPriceDetails).
  • Write up example use cases for these columns (comment)
  • Make determination on whether PascalCase should be included in the glossary and link to it, or if we should use the external link for PascalCase found in specification/attributes/column_naming_and_ordering.md

Action Items from TF-2 call on July 24:

  • Standardize Attribute Naming Conventions:
    Ensure that the attribute naming conventions are standardized, with a preference for using Pascal case. This action item involves reviewing existing data and documentation to align with this naming convention.
  • Finalize and Document the Definitions:
    Finalize the definitions of SKU details and SKU price details, ensuring that all team members and stakeholders have a clear understanding of what each category should include. This includes documenting these definitions in a shared document or guideline.
  • Coordinate with CSP Representatives:
    Work with representatives from different cloud service providers (CSPs) to ensure they are aligned with the proposed standards for SKU details and SKU price details. This coordination aims to facilitate uniform implementation across all providers involved.
  • Review and Approve Pull Request FOCUS #497: SkuPriceDetails column #503:
    Team members, particularly those involved in code review and approval, need to review the changes proposed in Pull Request FOCUS #497: SkuPriceDetails column #503. This includes checking for accuracy, completeness, and adherence to the agreed-upon guidelines and standards.

Action Items from the Maintainer's call on August 5th:

  • [TF1-#497-#503] Karl is waiting for group’s feedback. If someone disagrees with the current proposal, they should present a competing alternative.

Action Items from TF-2 call on August 7th:

  • [TF2-#503] Karl: Update the PR to reflect the introduction of a single SKU-related column and prepare to discuss it in abstract at the members' meeting.
  • [TF2-#503] Draft the survey/voting text for Slack to gather feedback on the necessity and scope of a second column.
  • [TF2-#503] Tim: Collect additional customer feedback to help inform future decisions on column additions.
  • General: Ensure all discussions and decisions are documented for future reference and clarity.

Action Items from TF-2 on Aug 14 call:

  • [TF2-#503] Karl to complete the rephrasing and wordsmithing.
  • [TF2-#503] All team members to review the PR and provide feedback before the next meeting.

Action Items, Members', Aug 15:

[Maintainers-#503 ] Karl to address any final comments and maintainers to review and approve the PR.

Action Items, Maintainers call, Aug 19:

  • [Maintainer-##503] Karl will conduct a poll to determine the approach to SKU price details and finalize the PR accordingly.

@jpradocueva jpradocueva linked an issue Jul 9, 2024 that may be closed by this pull request
1 task
@jpradocueva jpradocueva added the P1 label Jul 9, 2024
@jpradocueva jpradocueva added this to the v1.1 milestone Jul 9, 2024
@kk09v
Copy link
Contributor Author

kk09v commented Jul 9, 2024

Additional Discussion points:

  • Evaluation of SHOULD vs MUST normative statements
  • Clarifications
    • Do we need clarification that the immutability of properties clause means that flags (e.g. "is_current") and other things like validity dates should not be included?
    • Do we need to define what is meant by "property"
    • Should we further clarify that property refers to key and value unless otherwise specified?
    • How do we explain the sorts of things we want included without relying on examples?
  • Phrasing: list of words/phrases that might have a better alternative
    • "all resources"
    • "properties"

@ijurica
Copy link
Contributor

ijurica commented Jul 10, 2024

SKU - SKU Price relationship

Is the following statement correct?

... A given value of SkuPriceId MUST be associated with one and only one SkuId, except in cases of commitment discount flexibility. ...

When we wrote the SkuPriceId specification, it seemed to be the case, but I now have doubts. It would be beneficial to examine sample cost records to verify or contradict this.

Sample charge records

Karl has already noted the need to include sample charge records (beyond sample SKU Details and SKU Price Details values). Here are some key use cases I believe we should address:

  • flat-rate pricing,
  • tier-based pricing,
  • volume-based pricing,
  • commitment discount flexibility,
  • free tier,
  • etc.

Note: It is very likely that the terms used are not the most appropriate (for internal use only). We should probably address those pricing strategies/models/vegetables 🙂 at some point in the glossary or appendix.

kk09v added 2 commits July 11, 2024 14:17
Correct ConsumedUnit to ConsumedQuantity
Correct ConsumedUnit to ConsumedQuantity
@kk09v
Copy link
Contributor Author

kk09v commented Jul 22, 2024

Here are some of the use cases (phrased as questions I'd want to be able to easily answer) I had in my mind when I was creating these dimensions. I do reference specific CSP offerings in many of these because each CSP has a different approach to pricing/billing which may make it easier or harder to answer a question.

How many cores of D-series (Azure) VMs do we have running in each region?
How much of my GCS bill is for storage of data versus access to the data (e.g. operations, network, etc.)?
How much total "disk" storage do we have provisioned?
How much do we spend on GPUs?
What portion of our N-series VM usage is Spot instances?

Specific to SkuPriceDetails
Why am I paying more for this resource than that resource (which look the same to me, as a user)?

@ijurica
Copy link
Contributor

ijurica commented Jul 24, 2024

@kk09v I've noticed that you've started preparing sample dataset.

I feel like we should prepare more sample records before we close this PR. We should probably choose a couple of records from each CSP. As mentioned before, here are some use case scenarios that I believe we should cover.

  • flat-rate pricing,
  • tier-based pricing,
  • volume-based pricing,
  • commitment discount flexibility,
  • free tier,
  • etc.

Not sure if this helps, but here it is - in this Google spreadsheet, I've extracted some Azure-EA sample records (focusing on a subset of FOCUS and Pricing columns). As mentioned before, I've used sample data provided in the azure-finops-toolkit. In one of the sheets, I've also provided a list of columns available in the FOCUS Dataset, EA-Pricing, and Public Azure Pricing.

@robmartin33
Copy link

“…which are not captured in another FOCUS column.” Implies that “Region” in Irena’s example would not be in SKU Details.

Are we losing some compatibility over time (constructing a query against multiple versions of FOCUS data in a single database would have to look in a column for new data, but parse SKU Details in older data), clarity (we recreate the problem we have today having to know what collection of columns and SKU Details makes up a unique SKU), and flexibility if we take some things out because they are in columns? As opposed to allowing each vendor to specify HOW THEY define their SKU fully (while still filling out the columns correctly also).

If all of the SKU Details are in for any SKU, including the data that make that SKU unique, we know they are all always all the attributes.

I see Udam's point about this being an easy button that might slow down column standardization, but it also does simplify reporting for practitioners in certain use cases wouldn't it?

If I'm getting this all wrong, please let me know. Happy to take a call.

@kk09v
Copy link
Contributor Author

kk09v commented Jul 25, 2024

@robmartin33

“…which are not captured in another FOCUS column.” Implies that “Region” in Irena’s example would not be in SKU Details.

Are we losing some compatibility over time (constructing a query against multiple versions of FOCUS data in a single database would have to look in a column for new data, but parse SKU Details in older data), clarity (we recreate the problem we have today having to know what collection of columns and SKU Details makes up a unique SKU), and flexibility if we take some things out because they are in columns? As opposed to allowing each vendor to specify HOW THEY define their SKU fully (while still filling out the columns correctly also).

If all of the SKU Details are in for any SKU, including the data that make that SKU unique, we know they are all always all the attributes.

I see Udam's point about this being an easy button that might slow down column standardization, but it also does simplify reporting for practitioners in certain use cases wouldn't it?

If I'm getting this all wrong, please let me know. Happy to take a call.

Region (in particular) is tricky because Region could be a property of a SKU (in the Azure example, the SKU is D4v3/D4sv3 in East US), could be a property of a SKU Price (where the same SKU deployed in different regions has a different price), or could be a property of a resource (a single SKU/SKU Price in GCP applies to "Americas" where the resource could be deployed any of 4 regions). In my example with the Microsoft data, I faked a property called PricingRegion as an example of what could be put into SkuPriceDetails. I used the value "US East" from the x_SkuRegion column which actually differs from the value in the Region column "East US"

This specific case aside, to your question of "What happens if a new dimension is created for something currently in SkuDetails?" The value SHOULD NOT be removed from the SkuDetails because that would violate the immutability clause in the normative section. The clause you're referencing is not in the normative section; it was intended to signal more about what data should be included rather than what should not be included. I'm trying to avoid someone implementing this reading these columns and asking themselves, "Does this require me to repeat PricingCategory, PricingUnit, etc. in SkuPriceDetails?" The answer to that question is no, but if they decide to do so, that's fine because there's nothing indicating the MUST NOT nor SHOULD NOT repeat those as key/value pairs here.

kk09v added 4 commits July 25, 2024 00:16
This likely doesn't meet the criteria for being included in the appendix due to being well defined elsewhere, but I'm including it for discussion. PascalCase is also referenced in specification/attributes/column_naming_and_ordering.md with a link to an external definition https://techterms.com/definition/pascalcase
Additionally, reformatted examples as PascalCase
@kk09v
Copy link
Contributor Author

kk09v commented Jul 25, 2024

I added the PascalCase clause to both and reformatted the examples as PascalCase, per the discussion in TF2 today.

I also added PascalCase to the glossary, though it may not meet our criteria for being included because it's well defined externally. The only other place it's referenced in FOCUS is in specification/attributes/column_naming_and_ordering.md where there's a link to an external site. This was added to the glossary for discussion since I could go either way on including it. If we decide to keep it, we'd link PascalCase to the glossary in these column definitions, otherwise we'll remove that addition from the glossary. Though it could be argued that PascalCase should belong as an attribute, I don't believe that's warranted at this point.

Copy link
Contributor

@jpradocueva jpradocueva left a comment

Choose a reason for hiding this comment

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

Although the group has not finalized the editorial guidelines, they have agreed not to use bold formatting for normative keywords like "MUST" and "SHOULD."

specification/columns/skudetails.md Outdated Show resolved Hide resolved
specification/columns/skudetails.md Outdated Show resolved Hide resolved
specification/columns/skudetails.md Outdated Show resolved Hide resolved
specification/columns/skudetails.md Outdated Show resolved Hide resolved
specification/columns/skudetails.md Outdated Show resolved Hide resolved
specification/columns/skupricedetails.md Outdated Show resolved Hide resolved
specification/columns/skupricedetails.md Outdated Show resolved Hide resolved
specification/columns/skupricedetails.md Outdated Show resolved Hide resolved
specification/columns/skupricedetails.md Outdated Show resolved Hide resolved
specification/columns/skupricedetails.md Outdated Show resolved Hide resolved
@flanakin flanakin self-requested a review August 27, 2024 19:54
@ijurica ijurica self-requested a review August 28, 2024 20:31
@jpradocueva
Copy link
Contributor

jpradocueva commented Aug 28, 2024

Action Items from TF-2 call on Aug 28:

  • [TF2-#503] Karl - @kk09v : To incorporate suggestions from the team and finalize the wording in PR FOCUS #497: SkuPriceDetails column #503.
  • [TF2-#503] Karl - @kk09v : To ensure the final document is consistent, including the use of asterisks for bullet points.
  • [TF2-#503] Karl - @kk09v : To add relevant examples and supporting content in a separate follow-up PR.

Copy link
Contributor

@udam-f2 udam-f2 left a comment

Choose a reason for hiding this comment

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

nit: I would reorder these so the more definitive statements (MUST) aren't spread out throughout in between other less definitive requirements

specification/columns/skupricedetails.md Outdated Show resolved Hide resolved
specification/columns/skupricedetails.md Outdated Show resolved Hide resolved
specification/columns/skupricedetails.md Outdated Show resolved Hide resolved
kk09v and others added 3 commits August 28, 2024 22:53
Replaced _italic_ with *italic*
Replaced
* bullet
with 
- bullet
@kk09v
Copy link
Contributor Author

kk09v commented Aug 29, 2024

nit: I would reorder these so the more definitive statements (MUST) aren't spread out throughout in between other less definitive requirements

The ordering has gotten flipped around a few times. The general feedback was that grouping clauses together by topic was easier to read and understand than grouping them by level of normativity. I'm not aware of where we take a stand on this in the design guidelines or similar, but happy to conform to guidelines as needed.

@kk09v
Copy link
Contributor Author

kk09v commented Aug 29, 2024

@AWS-ZachErdman can you take a look at lines 3 and 13 to make sure I've incorporated your feedback appropriately?

@jpradocueva
Copy link
Contributor

Action Items from Members' call on Aug 29:

  • [Members-#503] Karl - @kk09v : Finalize editorial changes and ensure consistent language throughout the document.

@kk09v
Copy link
Contributor Author

kk09v commented Sep 2, 2024

Changed verbiage on lines 10 and 11 to reconcile a conflict between the bullet and sub-bullets.

@jpradocueva
Copy link
Contributor

Action Item from Maintainers' call on Sep 2nd:

@kk09v
Copy link
Contributor Author

kk09v commented Sep 4, 2024

Updates since last week for 20240905 meeting:

  • Added nullability clauses to make it explicit.
  • Made changes to clarify how two clauses relate to one another:
    • The properties (both keys and values) contained in the SkuPriceDetails column MUST be shared across all charges having the same SkuPriceId, subject to the below provisions.
    • Additional properties (key-value pairs) MAY be added to SkuPriceDetails going forward for a given SkuPriceId

@jpradocueva
Copy link
Contributor

Action Items from TF-2 call on Sep 4:

  • [TF2-#503] Karl, @kk09v : Make final edits on capitalization and null conditions before the next review.

@flanakin flanakin dismissed their stale review September 5, 2024 18:41

Changes were made

@jpradocueva
Copy link
Contributor

Approved by the Members group on Sep 5.

jpradocueva added a commit that referenced this pull request Sep 5, 2024
This change is dependent on #503 being merged as it links to
SkuPriceDetails. This change to SkuPriceId solves for a case where a
provider does not have a SkuPriceId but wants to include
SkuPriceDetails. This case was introduced when there was a change in
scope of #503 to not include SkuDetails.

Changes:
- Reformatted normative to bullets
- Added option (MAY) to repeat SkuId as SkuPriceId when there is no
SkuPriceId, as long as other conditions are not violated.

---------

Co-authored-by: Udam Dewaraja <[email protected]>
Co-authored-by: Joaquin <[email protected]>
@jpradocueva jpradocueva merged commit 31803cb into FinOps-Open-Cost-and-Usage-Spec:working_draft Sep 5, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: W.I.P
Development

Successfully merging this pull request may close these issues.

SKU Price Details