Provide scraper for Azure Storage Queues #402
Conversation
There was a problem hiding this comment.
Thank you for your contribution! We are not there yet, but we are close!
Some remarks I have:
- You should not use the name "Azure Queue" given storage queues are not the only queues in Azure. Please use "Azure Storage Queue" instead
- It looks like you are pulling the metrics from the Azure Storage Management SDK but we prefer to use Azure Monitor REST API for this. Can you please align this? Or did you try this? The benefit of plugging into Monitor is that we get more metrics than just queues
Here is how you can determine the resource id - https://docs.microsoft.com/en-us/azure/storage/common/storage-metrics-in-azure-monitor#understanding-resource-id-for-services-in-azure-storage
...romitor.Scraper.Host/Validation/MetricDefinitions/ResourceTypes/AzureQueueMetricValidator.cs
Outdated
Show resolved
Hide resolved
...sts.Unit/Validation/Metrics/ResourceTypes/AzureQueueMetricsDeclarationValidationStepTests.cs
Outdated
Show resolved
Hide resolved
src/Promitor.Scraper.Tests.Unit/Serialization/MetricsDeclaration/YamlSerializationTests.cs
Outdated
Show resolved
Hide resolved
...romitor.Scraper.Host/Validation/MetricDefinitions/ResourceTypes/AzureQueueMetricValidator.cs
Outdated
Show resolved
Hide resolved
...romitor.Scraper.Host/Validation/MetricDefinitions/ResourceTypes/AzureQueueMetricValidator.cs
Outdated
Show resolved
Hide resolved
|
Unfortunately Azure Monitor REST API doesn't provide metrics per storage queue, only per account. That's why I used Azure Storage Management SDK. |
|
Unfortunately, you are right, thanks for letting me know! I will review the current PR asap! |
There was a problem hiding this comment.
Thanks for the changes - We are almost there!
- I would leave the duration metric out for now and stick to
MessageCount(rather thanSize) - Would love to rename our
Promitor.Integrations.AzureStorageQueueproject toPromitor.Integrations.AzureStorageas it goes beyond queues
src/Promitor.Scraper.Tests.Unit/Builders/MetricsDeclarationBuilder.cs
Outdated
Show resolved
Hide resolved
src/Promitor.Integrations.AzureStorageQueue/AzureStorageQueue.cs
Outdated
Show resolved
Hide resolved
src/Promitor.Integrations.AzureStorageQueue/AzureStorageQueue.cs
Outdated
Show resolved
Hide resolved
src/Promitor.Integrations.AzureStorageQueue/AzureStorageQueue.cs
Outdated
Show resolved
Hide resolved
src/Promitor.Integrations.AzureStorageQueue/AzureStorageQueue.cs
Outdated
Show resolved
Hide resolved
|
Actually I've noticed that we're now using |
There was a problem hiding this comment.
This looks great, thanks!
Just some final remarks:
- Remove the Duration metric for now as we'll tackle this later
- Don't forget to rename our
Promitor.Integrations.AzureStorageQueueproject toPromitor.Integrations.AzureStorageas requested before given this goes beyond queues - Fix the open comments
Other than that we are ready to proceed and merge this - Thank you for your contribution!
If I need to jump in and help, just let me know!
...mitor.Scraper.Host/Validation/MetricDefinitions/ResourceTypes/StorageQueueMetricValidator.cs
Outdated
Show resolved
Hide resolved
|
The reason why we'll skip the Duration metric is to keep this PR nice and clean and track a new issue for that metric. The current duration implementation is also not stable yet given it reports the metric as last message while we are basing us on the first message, not the last. Don't hesitate to create a new issue for this and provide the scenario you're trying to tackle and how you'd expect it to work! |
Implements #64
Proposed Changes