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

Compute best practices documentation update #1088

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

Conversation

nteyan
Copy link
Contributor

@nteyan nteyan commented Nov 4, 2024

πŸ› οΈ Description

This update to the documentation page introduces new queries designed to retrieve detailed information about compute resources. The motivation behind these changes is to provide users with the tools needed to identify cost-saving opportunities and optimize resource usage. By leveraging these queries, users can gain insights into their compute resource allocation, enabling more efficient and cost-effective management.

πŸ“· Screenshots

πŸ“‹ Checklist

πŸ”¬ How did you test this change?

  • 🀏 Lint tests
  • 🀞 PS -WhatIf / az validate
  • πŸ‘ Manually deployed + verified
  • πŸ’ͺ Unit tests
  • πŸ™Œ Integration tests

πŸ™‹β€β™€οΈ 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)

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Review πŸ‘€ PR that is ready to be reviewed Skill: Documentation Documentation updates Tool: FinOps guide Implementing FinOps guide labels Nov 4, 2024
@nteyan nteyan removed Needs: Review πŸ‘€ PR that is ready to be reviewed Tool: FinOps guide Implementing FinOps guide labels Nov 4, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the Tool: FinOps guide Implementing FinOps guide label Nov 4, 2024
@nteyan nteyan added the Needs: Review πŸ‘€ PR that is ready to be reviewed label Nov 4, 2024
@nteyan nteyan changed the title Compute best practices updates Compute best practices documentation update Nov 4, 2024

### Query: AKS Cluster
### List of cost recommendations for Compute
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to track "Advisor" as a service under every category? It feels like that should only fall under General, right? At least, unless we see some very targeted action that we can recommend based on the service-specific queries, right?

| where tostring (properties.category) has "Cost"
| where properties.shortDescription.problem has "underutilized"
| where properties.impactedField has "Compute" or properties.impactedField has "Container" or properties.impactedField has "Web"
| project AffectedResource=tostring(properties.resourceMetadata.resourceId),Impact=properties.impact,resourceGroup,AdditionaInfo=properties.extendedProperties,subscriptionId,Recommendation=tostring(properties.shortDescription.problem)
Copy link
Collaborator

Choose a reason for hiding this comment

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

There should be spaces around =, after commas, and long lines should be broken up on multiple lines.


<h4>Category</h4>

Resource management
Cost optimization

<h4>Query</h4>

```kql
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: We should establish some KQL language standards. If you happen to know of any, we should document them in the wiki. No worries if you haven't.

AKSname = name
advisorresources
| where type == "microsoft.advisor/recommendations"
| where tostring (properties.category) has "Cost"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
| where tostring (properties.category) has "Cost"
| where tostring(properties.category) has "Cost"

| where type == "microsoft.advisor/recommendations"
| where tostring (properties.category) has "Cost"
| where properties.shortDescription.problem has "underutilized"
| where properties.impactedField has "Compute" or properties.impactedField has "Container" or properties.impactedField has "Web"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move each or to a new line so it's more readable online. Web should be in the Web category and not in Compute according to FOCUS service categories.

Suggested change
| where properties.impactedField has "Compute" or properties.impactedField has "Container" or properties.impactedField has "Web"
| where properties.impactedField has "Compute"
or properties.impactedField has "Container"

docs/_docs/best-practices/compute/compute.md Outdated Show resolved Hide resolved
Comment on lines +114 to +128
Resources | where type == "microsoft.compute/virtualmachines"
| extend osDiskId= tostring(properties.storageProfile.osDisk.managedDisk.id)
| join kind=leftouter(resources
| where type =~ 'microsoft.compute/disks'
| where properties !has 'Unattached'
| where properties has 'osType'
| project OS = tostring(properties.osType), osSku = tostring(sku.name), osDiskSizeGB = toint(properties.diskSizeGB), osDiskId=tostring(id)) on osDiskId
| join kind=leftouter(Resources
| where type =~ 'microsoft.compute/disks'
| where properties !has "osType"
| where properties !has 'Unattached'
| project sku = tostring(sku.name), diskSizeGB = toint(properties.diskSizeGB), id = managedBy
| summarize sum(diskSizeGB), count(sku) by id, sku) on id
| project vmId=id, subscriptionId, resourceGroup, OS, location, osDiskId, osSku, osDiskSizeGB, DataDisksGB=sum_diskSizeGB, diskSkuCount=count_sku
| sort by diskSkuCount desc
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Make sure the queries are formatted correctly.
  2. You're using a case-insensitive type filter on some lines and case-sensitive on the first where. Which is correct?
  3. Let's always use single instead of double quotes.
  4. Make sure table names are cased consistently. You have "resources" and "Resources".
  5. Doesn't KQL use order by instead of sort by?
Suggested change
Resources | where type == "microsoft.compute/virtualmachines"
| extend osDiskId= tostring(properties.storageProfile.osDisk.managedDisk.id)
| join kind=leftouter(resources
| where type =~ 'microsoft.compute/disks'
| where properties !has 'Unattached'
| where properties has 'osType'
| project OS = tostring(properties.osType), osSku = tostring(sku.name), osDiskSizeGB = toint(properties.diskSizeGB), osDiskId=tostring(id)) on osDiskId
| join kind=leftouter(Resources
| where type =~ 'microsoft.compute/disks'
| where properties !has "osType"
| where properties !has 'Unattached'
| project sku = tostring(sku.name), diskSizeGB = toint(properties.diskSizeGB), id = managedBy
| summarize sum(diskSizeGB), count(sku) by id, sku) on id
| project vmId=id, subscriptionId, resourceGroup, OS, location, osDiskId, osSku, osDiskSizeGB, DataDisksGB=sum_diskSizeGB, diskSkuCount=count_sku
| sort by diskSkuCount desc
Resources
| where type == 'microsoft.compute/virtualmachines'
| extend osDiskId= tostring(properties.storageProfile.osDisk.managedDisk.id)
| join kind=leftouter(
Resources
| where type =~ 'microsoft.compute/disks'
| where properties !has 'Unattached'
| where properties has 'osType'
| project
OS = tostring(properties.osType),
osSku = tostring(sku.name),
osDiskSizeGB = toint(properties.diskSizeGB),
osDiskId=tostring(id)
) on osDiskId
| join kind=leftouter(
Resources
| where type =~ 'microsoft.compute/disks'
| where properties !has "osType"
| where properties !has 'Unattached'
| project
sku = tostring(sku.name),
diskSizeGB = toint(properties.diskSizeGB),
id = managedBy
| summarize sum(diskSizeGB), count(sku) by id, sku
) on id
| project
vmId = id,
subscriptionId,
resourceGroup,
OS,
location,
osDiskId,
osSku,
osDiskSizeGB,
DataDisksGB = sum_diskSizeGB,
diskSkuCount = count_sku
| order by diskSkuCount desc

@@ -152,6 +174,87 @@ resources

<br>

### Query: Virtual machine scale set details
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be in the VMSS section? (Unless I missed it.)


<br>

## App Service plans
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move this to the Web category.


<br>

## Azure Kubernetes Service
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move this back to the top. Services should be sorted alphabetically.

@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 5, 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 6, 2024
@flanakin flanakin added this to the Guide - Build-out milestone Nov 7, 2024
@flanakin flanakin 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 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Attention πŸ‘‹ Issue or PR needs to be reviewed by the author or it will be closed due to no activity Skill: Documentation Documentation updates Tool: FinOps guide Implementing FinOps guide
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants