fix(azuremonitorreceiver): Azure Monitor receiver should not produce gaps in data points for PT1M time grains

[an-mmx]

Description

The current implementation of tracking for the last metrics fetch is highly sensitive to time, relying on time.Now(). This sensitivity causes the loss of data points due to jitter in 1-minute intervals. I recommend a change request to implement a more robust comparison with the last minute of metrics fetch.

Link to tracking issue

Fixes #37337

Testing

A new coverage introduced in unit test

Documentation

No documentation added

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[an-mmx]

/ping

[celian-garcia]

That's very interesting. Did you observe the same thing with other timegrain?
I feel like your fix would fix only the PT1M but not others.

We have a fork (using batch metric API instead of metric API in order to resolve a problem of rate limitation, anyway..) and in that fork we did something different. We remove 4 times the duration of the timegrain.
So for a PT1M, we take 4 * 60 = 240 seconds in the past. This to ensure that we'll receive some data as if we take only 60sec back, we sometimes have nothing, and nothing, and nothing. Of course it's not the exactly the same API but maybe the problem is the same.

So in the end, we're very interested in having some figures with before/after. And I think I will try your implementation with our fork.

[an-mmx]

.. Did you observe the same thing with other timegrain? I feel like your fix would fix only the PT1M but not others.

Following the scrapper logic and our changes, it should work well. The root cause of the issue was in a timedrift caused by various factors (like requests duration and processing time) and the way it was compared. This change mitigates impact of the timedrift.
Could you please share your concerns regarding other timegrains?

We have a fork (using batch metric API instead of metric API in order to resolve a problem of rate limitation, anyway..) and in that fork we did something different. We remove 4 times the duration of the timegrain. So for a PT1M, we take 4 * 60 = 240 seconds in the past. This to ensure that we'll receive some data as if we take only 60sec back, we sometimes have nothing, and nothing, and nothing. Of course it's not the exactly the same API but maybe the problem is the same.

Have you checked for these gaps in Azure App Insights? Azure doesn't store null-value data points, it might be the case: https://learn.microsoft.com/en-us/azure/azure-monitor/essentials/metrics-troubleshoot#chart-shows-dashed-line

So in the end, we're very interested in having some figures with before/after. And I think I will try your implementation with our fork.

Sure.
We collect Azure EventHub IncomingMessages metric. Here you can see a line, representing a count of data points per 1 hour collected before the fix, filtered by a single unique dimension. We can observe that we collect less than 60 datapoints per hour, and this isn't something we expect.

The next screenshot have taken after the fix. It is build on the same query. We can observe that we have reached 60 datapoints per hour. We can also observe some "spikes". These spikes is a result of otel-collector restarts. However, it isn't a problem for us.

[celian-garcia]

Well ok, I checked twice and our fork using getBatch metrics API is completely different in terms of interface. We make a batchRequest giving start and end time and the classic metrics API doesn't even have this notion actually.
This is just to take the metrics at a T time. So what you did is perfect and seems indeed to largely mitigates the issue.

LGTM in the end, I would like to approve, but I'm owner of this code since only a week and I'm not sure how can I approve it :D

[an-mmx]

Usch, I've rebased w/ main and lots of test got failed :/

[celian-garcia]

Hoping that it has some weight here, I'm approving these changes.
🙏
I've been assign to CODEOWNERS after the PR has been created :)

[crobert-1]

Hoping that it has some weight here, I'm approving these changes.

@celian-garcia: You're a code owner now, so it definitely counts. Really appreciate your input here 🙂