Concurrent fetch of azure metricdefinitions and batchApi usage

[MichaelKatsoulis]

The changes affect azure monitor and relevant metricsets. The list of metricsets affected are:

• monitor
• container_registry
• container_instance
• container_service
• compute_vm
• compute_vm_scaleset
• database_account
• storage_account

A new configuration parameter is introduced enable_batch_api of type boolean.
If set to false(default) nothing changes in the way the metrics are collected for these metricsets.

If set to true:

• The metric definitions of resources are collected asynchronously and write the results in a channel.
• The channel is read and when the number of definitions collected reach 50 (batch API limit)
• The metrics definitions are grouped based on criteria(1) and the azure BatchAPI is used to retrieve
  metrics of multiple resources with one api call.

1. Grouping criteria are

• Namespace
• SubscriptionID
• Location
• Names
• TimeGrain
• Dimensions

Proposed commit message

• WHAT: Introduce enable_batch_api parameter for concurrent fetching of azure metric definitions and metric values collection using Batch Api
• WHY: Helps mitigating scalability problems

Checklist

• My code follows the style guidelines of this project
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have made corresponding change to the default configuration files
• I have added tests that prove my fix is effective or that my feature works
• I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Disruptive User Impact

Author's Checklist

• [ ]

How to test this PR locally

Related issues

• Relates Improve Azure Monitor scalability and performance #38624

Use cases

Screenshots

Logs

[mergify]

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b concurrent-fetch-of-azure-metricdefinitions upstream/concurrent-fetch-of-azure-metricdefinitions
git merge upstream/main
git push upstream concurrent-fetch-of-azure-metricdefinitions

[mergify]

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @MichaelKatsoulis? 🙏.
For such, you'll need to label your PR with:

• The upcoming major version of the Elastic Stack
• The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

• backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit

[mergify]

backport-8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

[zmoog]

Microsoft.DocumentDb/databaseAccounts (1 resource)

resource type: Microsoft.DocumentDb/databaseAccounts
resource count: 1 resource
versions tested:

• 8.17.1 (branch 8.17)
• 9.0.0 (branch MichaelKatsoulis:concurrent-fetch-of-azure-metricdefinitions)

Activity:

• I created one "Azure Cosmos DB for NoSQL", with Provisioned throughput (default settings)
• I set up the standard Metricbeat database account module

# x-pack/metricbeat/modules.d/azure.yml
- module: azure
  metricsets:
  - database_account
  enabled: true
  period: 300s
  client_id: '${AZURE_CLIENT_ID:""}'
  client_secret: '${AZURE_CLIENT_SECRET:""}'
  tenant_id: '${AZURE_TENANT_ID:""}'
  subscription_id: '${AZURE_SUBSCRIPTION_ID:""}'
  refresh_list_interval: 600s

• 8.17.1 and 9.0.0 are creating the same metrics (cardinality and values).

UPDATE: I didn't build the right version, I'm re-testing 9.0.0

8.17.1

9.0.0

• Data collected regularly: yes

Issues

(1) Timegrain for azure.database_account.create_account.count is empty

In version 8.17.1, the timegrain for this field is PT5M.

(2) The azure.database_account.service_availability.avg (timegrain PT1H) is missing

Version 9.0.0 always collects 7 documents with PT5M, while version 8.17.1 collect 7 documents PT5M + 1 document PT1H during the first iteration and again every 60 mins.

Is 9.0.0 missing the PT1H document on the first iteration? Waiting for the next iteration to double-check.

After 75 mins, no azure.database_account.service_availability.avg field with PT1H.

UPDATE: tested by @MichaelKatsoulis

I managed to collect azure.database_account.service_availability.avg field with PT1H with the PR code. The problem is that the API requests metric values for metrics ServiceAvailability and ReplicationLatency for Average aggregation. When values for both metrics are requested, service_availability.avg is always nil. If we remove the ReplicationLatency and we just request values for ServiceAvailability the service_availability.avg is returned ok! Still do not know the reason of that.

[zmoog]

UPDATE: I built the wrong version, I'm re-testing 9.0.0 with Microsoft.DocumentDb/databaseAccounts (1 resource) and I'll update the previous comment.

My apologies for the noise.

[zmoog]

Microsoft.KeyVault/vaults (10 resources)

resource type: Microsoft.KeyVault/vaults
resource count: 10 resources
versions tested:

• 8.17.1 (branch 8.17)
• 9.0.0 (branch MichaelKatsoulis:concurrent-fetch-of-azure-metricdefinitions)

Activity:

• I set up a custom Metricbeat config using the Azure Monitor metricset to target the key vaults

- module: azure  
  metricsets:  
    - monitor  
  enabled: true  
  period: 60s  
  client_id: '${AZURE_CLIENT_ID:""}'
  client_secret: '${AZURE_CLIENT_SECRET:""}'
  tenant_id: '${AZURE_TENANT_ID:""}'
  subscription_id: '${AZURE_SUBSCRIPTION_ID:""}'
  refresh_list_interval: 600s  
  resources:  
  - resource_query: "resourceType eq 'Microsoft.KeyVault/vaults'"  
    resource_group:  
    - "mbranca-az-scalability-kv-r10"    
    metrics:  
      - dimensions:  
          - name: ActivityType  
            value: '*'  
          - name: ActivityName  
            value: '*'  
          - name: StatusCode  
            value: '*'  
          - name: StatusCodeClass  
            value: '*'  
        ignore_unsupported: true  
        name:  
          - ServiceApiLatency  
          - Availability  
          - ServiceApiResult  
        namespace: Microsoft.KeyVault/vaults  
        timegrain: PT1M  
      - dimensions:  
          - name: ActivityType  
            value: '*'  
          - name: ActivityName  
            value: '*'  
        ignore_unsupported: true  
        name:  
          - ServiceApiHit  
        namespace: Microsoft.KeyVault/vaults  
        timegrain: PT1M  
      - dimensions:  
          - name: ActivityType  
            value: '*'  
          - name: ActivityName  
            value: '*'  
          - name: TransactionType  
            value: '*'  
        ignore_unsupported: true  
        name:  
          - SaturationShoebox  
        namespace: Microsoft.KeyVault/vaults  
        timegrain: PT1M

Notes:

When the key vaults are unused (like in this resource group), they only generates a subset of metrics:

• Availability
• API Hits
• API Results.

8.17.1

In progress.

I can see the three metrics (Availability, API Hits, API Results), grouped in two documents. So 2 documents x 10 resources = 20 documents per iteration:

9.0.0

In progress.

First iterations are okay. I get the same number of documents (20) as 8.17.1 and same values.

Still checking, but this case looks good.

[zmoog]

@MichaelKatsoulis, I found a couple of issues relate to timegrain in the Microsoft.DocumentDb/databaseAccounts (1 resource) test.

[zmoog]

Microsoft.ContainerRegistry/registries (1 resource)

resource type: Microsoft.ContainerRegistry/registries
resource count: 1 resource
versions tested:

• 8.17.1 (branch 8.17)
• 9.0.0 (branch MichaelKatsoulis:concurrent-fetch-of-azure-metricdefinitions)

Activity:

• I set up a custom Metricbeat config using the Azure Monitor metricset to target the key vaults

- module: azure
  metricsets:
  - container_registry
  enabled: true
  period: 300s
  client_id: '${AZURE_CLIENT_ID:""}'
  client_secret: '${AZURE_CLIENT_SECRET:""}'
  tenant_id: '${AZURE_TENANT_ID:""}'
  subscription_id: '${AZURE_SUBSCRIPTION_ID:""}'
  refresh_list_interval: 600s

Since we had issue with PT1H metrics, I tried another metricset with this timegrain.

8.17.1

After one iteration, 8.17.1 collected:

• 1 document with PT5M every 5 minutes
• 1 document with PT1H every 60 minutes

9.0.0

After one iteration, 8.17.1 collected:

• 1 document with PT5M every 5 minutes
• 1 document with PT1H every 60 minutes

Conclusion

✅ With the recent code changes 8.17.1 and 9.0.0 yield the same outcome.

Metrics docs

• https://learn.microsoft.com/en-us/azure/azure-monitor/reference/supported-metrics/microsoft-containerregistry-registries-metrics

[zmoog]

The performance gains from the new sdk/monitor/query/azmetrics package with batch API are extremely compelling.

I would love to simplify the internal structure, but I am also okay with going with the PR as-is, collect customer feedback, and switch to the batch API in the next release.

I added a few non-blocking comments for things we may want to address before merging.

[zmoog]

Can we also add storage to the list, or remove it because the metricset supports all the metricsets, right?

[MichaelKatsoulis]

Isn't storage in this list?

[shabbywu]

metricStores only work on a single goroutine, so no mutex is needed?

[shabbywu]

In another points, if MetricStore need mutex, here should have an bug when another goroutine AddMetric into store.

bug for array copy will not share the resize action, see playground

But for now, MetricStore is only work in single goroutine, so it work well for now, right?