feat: Provide support for batch scraping #2459

hkfgo · 2024-03-23T05:30:11Z

Hey Tom, hope all is well! I worked on a PR to support batch scraping, got reasonably far, but sadly got blocked due to Promitor's current deprecated SDK. I think what I've written so far is enough to get some feedback though. Would love to know if you like this overall approach, w.r.t how batch scraping is configured, how scrape definitions are batched, the definition of the batch scrape job, etc. I've attached this diagram to illustrate my proposed change.

Meanwhile, I would love some input on SDK migration. I will comment separately on that issue. I think we need to migrate over to the new SDK and make sure all existing scraper features still work, before merging in changes to support batch scraping.

Implements #2284

github-actions · 2024-03-23T05:30:21Z

Thank you for your contribution! 🙏 We will review it as soon as possible.

hkfgo · 2024-03-23T05:31:08Z

src/Promitor.Core.Scraping/Configuration/Model/Metrics/ScrapeDefinitionBatch.cs

+
+    public TimeSpan AggregationInterval{ get; }
+
+    public override int GetHashCode()


Do you think this is a sane way to implement HashCode?

Do we need one? If not, let's remove it

The idea I have is to use hash code computation to enforce batch restrictions. This part should be sane.

How I'm choosing to implement hash code is hashing a unique string representation of ScrapeDefinitionBatch. Curious what you think/can think of something better?

tomkerkhove · 2024-04-04T10:50:01Z

Thanks! Is it safe to say that the top of the diagram is current and bottom shows these changes?

tomkerkhove

Good start, added a few remarks but no concerns!

tomkerkhove · 2024-04-04T10:52:53Z

src/Promitor.Agents.Scraper/Scheduling/ResourcesScrapingJob.cs

+                    Logger.LogInformation("Batch scraping Azure Metric {AzureMetricName} for resource type {ResourceType}.", azureMetricName, resourceType);
+                    await ScheduleLimitedConcurrencyAsyncTask(tasks, () => ScrapeMetricBatched(batchScrapeDefinition), cancellationToken);
+                }
+            }

            foreach (var scrapeDefinition in scrapeDefinitions)


Do we still need it if batch is enabled?

tomkerkhove · 2024-04-04T10:53:25Z

src/Promitor.Agents.Scraper/Scheduling/ResourcesScrapingJob.cs

+        /// 1. Definitions in a batch must target the same resource type 
+        /// 2. Definitions in a batch must target the same Azure metric with identical dimensions
+        /// 3. Definitions in a batch must have the same time granularity 
+        /// 4. Batch size cannot exceed configured maximum 


Let's add link to docs here for future reference

tomkerkhove · 2024-04-04T10:55:49Z

src/Promitor.Core.Scraping/Configuration/Model/AzureMetricConfiguration.cs

@@ -45,5 +45,20 @@ public class AzureMetricConfiguration
            }
            return Dimensions?.Any(dimension => dimension.Name.Equals(dimensionName, StringComparison.InvariantCultureIgnoreCase));
        }
+
+         // A unique string to represent this Azure metric and its configured dimensions
+        public string ToUniqueStringRepresentation()


Is this to identify unique groups?

Let's make sure we have proper unit tests for this before merging later on

tomkerkhove · 2024-04-04T10:55:59Z

src/Promitor.Core.Scraping/Configuration/Model/MetricBatchScrapeConfig.cs

+{
+    public class MetricBatchScrapeConfig
+    {
+        public bool Enabled { get; set; }


Let's be explicit

Suggested change

public bool Enabled { get; set; }

public bool Enabled { get; set; } = false;

tomkerkhove · 2024-04-04T11:04:33Z

src/Promitor.Core.Scraping/Configuration/Model/Metrics/BatchScrapeDefinition.cs

+            Guard.NotNull(groupedScrapeDefinitions, nameof(groupedScrapeDefinitions));
+            Guard.NotNull(groupedScrapeDefinitions, nameof(scrapeDefinitionBatchProperties));


Let's guard for things that have no entries

tomkerkhove · 2024-04-04T11:04:40Z

src/Promitor.Core.Scraping/Configuration/Model/Metrics/BatchScrapeDefinition.cs

+            Guard.NotNull(groupedScrapeDefinitions, nameof(groupedScrapeDefinitions));
+            Guard.NotNull(groupedScrapeDefinitions, nameof(scrapeDefinitionBatchProperties));


Suggested change

Guard.NotNull(groupedScrapeDefinitions, nameof(groupedScrapeDefinitions));

Guard.NotNull(groupedScrapeDefinitions, nameof(scrapeDefinitionBatchProperties));

Guard.NotNull(groupedScrapeDefinitions, nameof(groupedScrapeDefinitions));

Guard.NotNull(scrapeDefinitionBatchProperties, nameof(scrapeDefinitionBatchProperties));

tomkerkhove · 2024-04-04T11:05:12Z

src/Promitor.Core.Scraping/Configuration/Model/Metrics/ScrapeDefinition.cs

@@ -94,5 +94,14 @@ public class ScrapeDefinition<TResourceDefinition> where TResourceDefinition : c
            }
            return AzureMetricConfiguration?.Aggregation?.Interval;
        }
+
+        public ScrapeDefinitionBatchProperties buildPropertiesForBatch() { 


Massive nit

Suggested change

public ScrapeDefinitionBatchProperties buildPropertiesForBatch() {

public ScrapeDefinitionBatchProperties BuildScrapingBatchInfo() {

tomkerkhove · 2024-04-04T11:05:40Z

src/Promitor.Core.Scraping/Configuration/Model/Metrics/ScrapeDefinitionBatch.cs

+
+    public TimeSpan AggregationInterval{ get; }
+
+    public override int GetHashCode()


Do we need one? If not, let's remove it

tomkerkhove · 2024-04-04T11:05:48Z

src/Promitor.Core.Scraping/Configuration/Model/Metrics/ScrapeDefinitionBatch.cs

+    /// <summary>
+    /// Equality comparison override in case of hash collision
+    /// </summary>
+    public override bool Equals(object obj)


Same here, let's remove if we don't need it

tomkerkhove · 2024-04-04T11:15:40Z

src/Promitor.Core.Scraping/Scraper.cs

+                foreach (int i in Enumerable.Range(0, scrapedMetricResult.Count))
+                {
+                    var scrapedMetricResult = scrapedMetricResult[i];
+                    var scrapeDefinition = scrapeDefinitions[i];
+                    LogMeasuredMetrics(scrapeDefinition, scrapedMetricResult, aggregationInterval);
+
+                    await _metricSinkWriter.ReportMetricAsync(scrapeDefinition.PrometheusMetricDefinition.Name, scrapeDefinition.PrometheusMetricDefinition.Description, scrapedMetricResult);
+
+                    await ReportScrapingOutcomeAsync(scrapeDefinition, isSuccessful: true, isBatchJob: true) ;
+                }


We may need to fan this out to running tasks in parallel for every result, but can do that in a later update

hkfgo · 2024-05-09T22:47:20Z

Hey I'm revisiting this PR now since SDK migration is almost across the finish line. Do you know if the batch scraping API is available in sovereign clouds and what URLs we should use? We are particularly interested in the US Gov cloud. I can't find any information online(Azure doesn't really mentioned the existence of sovereign clouds to begin with)

tomkerkhove · 2024-05-16T07:22:29Z

Do you know if the batch scraping API is available in sovereign clouds and what URLs we should use?

I do not know, sorry! What is the URL for public cloud?

tomkerkhove · 2024-05-21T13:11:59Z

US Government cloud : .metrics.monitor.azure.us
- Token audience: https://metrics.monitor.azure.us
China cloud : .metrics.monitor.azure.cn
- Token audience: https://metrics.monitor.azure.cn

hkfgo · 2024-05-30T05:53:16Z

https://metrics.monitor.azure.cn

Awesome! How did you find this out?

tomkerkhove · 2024-05-30T07:04:37Z

I reached out to the team :D

tomkerkhove · 2024-06-01T06:58:38Z

@hkfgo Do you know if you'll have time to finalize this PR next week? I'd like to cut a release next week.

hkfgo · 2024-06-01T21:45:14Z

@hkfgo Do you know if you'll have time to finalize this PR next week? I'd like to cut a release next week.

We should plan for batch scraping to be on the release after this coming one. There are still a decent amount of work left and I'm not able to work on this full time.

I do have time to get SDK migration done though. In that case we should have something major-ish to release :)

tomkerkhove · 2024-06-03T18:42:43Z

That's OK - Thank you!

hkfgo added 6 commits March 18, 2024 17:51

define batch scrape definition model

163713d

implement skeleton batch scraping flow

d0f4c73

complete mapping logic between YAML and scraper domain

2552197

start grouping logic implementation via compound keys

975eef7

flesh out definiion batching logic

40289da

implement scraper batch function + hashcode/equals for grouping

e9e385a

hkfgo requested a review from tomkerkhove as a code owner March 23, 2024 05:30

trafico-bot bot added the 🔍 Ready for Review Pull Request is not reviewed yet label Mar 23, 2024

hkfgo commented Mar 23, 2024

View reviewed changes

implement scraper batch function + hashcode/equals for grouping

d6fe4ff

tomkerkhove marked this pull request as draft April 4, 2024 10:50

tomkerkhove changed the title ~~draft: enable batch scraping~~ feat: Provide support for batch scraping Apr 4, 2024

tomkerkhove reviewed Apr 4, 2024

View reviewed changes

Merge branch 'master' into feat/enable-batch-scraping

55b88f9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Provide support for batch scraping #2459

feat: Provide support for batch scraping #2459

hkfgo commented Mar 23, 2024

github-actions bot commented Mar 23, 2024

hkfgo Mar 23, 2024

tomkerkhove Apr 4, 2024

hkfgo May 30, 2024

tomkerkhove commented Apr 4, 2024

tomkerkhove left a comment

tomkerkhove Apr 4, 2024

tomkerkhove Apr 4, 2024

tomkerkhove Apr 4, 2024

tomkerkhove Apr 4, 2024

tomkerkhove Apr 4, 2024

tomkerkhove Apr 4, 2024

tomkerkhove Apr 4, 2024

tomkerkhove Apr 4, 2024

tomkerkhove Apr 4, 2024

tomkerkhove Apr 4, 2024

hkfgo commented May 9, 2024

tomkerkhove commented May 16, 2024

tomkerkhove commented May 21, 2024

hkfgo commented May 30, 2024

tomkerkhove commented May 30, 2024

tomkerkhove commented Jun 1, 2024

hkfgo commented Jun 1, 2024

tomkerkhove commented Jun 3, 2024


		public TimeSpan AggregationInterval{ get; }

		public override int GetHashCode()

	public bool Enabled { get; set; }
	public bool Enabled { get; set; } = false;

		Guard.NotNull(groupedScrapeDefinitions, nameof(groupedScrapeDefinitions));
		Guard.NotNull(groupedScrapeDefinitions, nameof(scrapeDefinitionBatchProperties));

	public ScrapeDefinitionBatchProperties buildPropertiesForBatch() {
	public ScrapeDefinitionBatchProperties BuildScrapingBatchInfo() {

feat: Provide support for batch scraping #2459

Are you sure you want to change the base?

feat: Provide support for batch scraping #2459

Conversation

hkfgo commented Mar 23, 2024

github-actions bot commented Mar 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomkerkhove commented Apr 4, 2024

tomkerkhove left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkfgo commented May 9, 2024

tomkerkhove commented May 16, 2024

tomkerkhove commented May 21, 2024

hkfgo commented May 30, 2024

tomkerkhove commented May 30, 2024

tomkerkhove commented Jun 1, 2024

hkfgo commented Jun 1, 2024

tomkerkhove commented Jun 3, 2024