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

fix: Improve AzureMonitorScraper Error Handling for Time Series Missing Requested Dimension Value #2345

Merged

Conversation

hkfgo
Copy link
Contributor

@hkfgo hkfgo commented Jul 12, 2023

Fixes #2331

At Axon, we observed that Azure Monitor may, from time to time, return time series without the requested dimension. The previous try catch logic is at the metric level, ensuring that failure to scrape one metric will not affect all metrics; however, there are many time series within a metric, and failure to process one time series will result in a metric gap for that metric. I am adding a try-catch when looping over time series to prevent that from happening.

I am very new to this repo though, and couldn't find my way around how to write tests for this. Some help would be much appreciated.

@hkfgo hkfgo requested a review from tomkerkhove as a code owner July 12, 2023 20:15
@CLAassistant
Copy link

CLAassistant commented Jul 12, 2023

CLA assistant check
All committers have signed the CLA.

@github-actions
Copy link

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

@trafico-bot trafico-bot bot added the 🔍 Ready for Review Pull Request is not reviewed yet label Jul 12, 2023
Copy link
Owner

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

Added a few comments, would you mind updating the changelog please?

src/Promitor.Core/Metrics/MeasuredMetric.cs Outdated Show resolved Hide resolved
@hkfgo
Copy link
Contributor Author

hkfgo commented Jul 13, 2023

Added a few comments, would you mind updating the changelog please?

Thanks for the review! Besides everything that's already talked about, I'd also like to know where I should put tests. I couldn't really find unit tests for AzureMonitorScraper/Client

@tomkerkhove
Copy link
Owner

Added a few comments, would you mind updating the changelog please?

Thanks for the review! Besides everything that's already talked about, I'd also like to know where I should put tests. I couldn't really find unit tests for AzureMonitorScraper/Client

Because unfortunately we don't have any given the tight integration and mocking everything is too much work/brittle

changelog/content/experimental/unreleased.md Outdated Show resolved Hide resolved
src/Promitor.Core/Metrics/MeasuredMetric.cs Outdated Show resolved Hide resolved
src/Promitor.Core/Metrics/MeasuredMetric.cs Outdated Show resolved Hide resolved
@tomkerkhove
Copy link
Owner

CLA assistant check Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

xchen seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Please make sure to sign the CLA

@hkfgo
Copy link
Contributor Author

hkfgo commented Jul 14, 2023

CLA assistant check Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
xchen seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Please make sure to sign the CLA

Ah okay, I think I signed it with my personal account, but committed with my company's account.

Copy link
Owner

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

Just two more questions - Sorry :)

src/Promitor.Core/Metrics/MeasuredMetric.cs Outdated Show resolved Hide resolved
src/Promitor.Core/Metrics/MeasuredMetric.cs Outdated Show resolved Hide resolved
Copy link
Owner

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@trafico-bot trafico-bot bot added ✅ Approved Pull Request has been approved and can be merged and removed 🔍 Ready for Review Pull Request is not reviewed yet labels Jul 18, 2023
@tomkerkhove
Copy link
Owner

We're good to go! Would you mind checking the merge conflict + code factor flag please?

@trafico-bot trafico-bot bot added 🔍 Ready for Review Pull Request is not reviewed yet and removed ✅ Approved Pull Request has been approved and can be merged labels Jul 18, 2023
@hkfgo
Copy link
Contributor Author

hkfgo commented Jul 19, 2023

Hey Tom, looks like the code is conflict-free now. Although build is failing on a single failing integration test. The exception looks like something transient unrelated to my changes?

System.Net.Http.HttpRequestException : Error while copying content to a stream.

Would you mind re-running the pipeline again? I don't think I have the privileges

@tomkerkhove
Copy link
Owner

/azp run Promitor CI - Scraper Agent

@azure-pipelines
Copy link

No commit pushedDate could be found for PR 2345 in repo tomkerkhove/promitor

@tomkerkhove
Copy link
Owner

/azp run Promitor CI - Scraper Agent

@azure-pipelines
Copy link

No commit pushedDate could be found for PR 2345 in repo tomkerkhove/promitor

@hkfgo
Copy link
Contributor Author

hkfgo commented Jul 19, 2023

Thanks for responding in your evening Tom :)

My most recent commit should have fixed warnings reported by R#(they were indeed real problems)

@tomkerkhove
Copy link
Owner

Happy to help a contributor out :)

Thanks!

@hkfgo
Copy link
Contributor Author

hkfgo commented Jul 25, 2023

Hey Tom, just letting you know that this has not been forgotten :) I was just a bit swamped these past few days. I'll look into how to run R# locally and also that one failing integration test when I get a chance.

Wrt to that test - seems like it timed out for whatever reason. I have to look at exactly what the code is myself to see if it's related to my changes. You have way more context though, so maybe you could share some potential insights?

@hkfgo
Copy link
Contributor Author

hkfgo commented Jul 25, 2023

Hey Tom, it's ready for another look. I see that there was a recent feature added to master that supported scraping multiple dimensions. You might wanna have a second look at if I merged correctly(I think I did). Thanks!

@tomkerkhove
Copy link
Owner

tomkerkhove commented Jul 29, 2023

/azp run Promitor CI - Scraper Agent

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tomkerkhove
Copy link
Owner

We should be good - Thanks!

@tomkerkhove tomkerkhove merged commit 35ebbe5 into tomkerkhove:master Jul 29, 2023
25 checks passed
@trafico-bot trafico-bot bot added ✨ Merged Pull Request has been merged successfully and removed 🔍 Ready for Review Pull Request is not reviewed yet labels Jul 29, 2023
@tomkerkhove
Copy link
Owner

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Merged Pull Request has been merged successfully
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Intermittent fatal errors: InvalidOperationException: Sequence contains no matching element
3 participants