[Azure] [Billing] Switch to Consumption API 1.1.0 with patches #37158

zmoog · 2023-11-20T15:58:25Z

Proposed commit message

Bump the following package versions to patch the Consumption SDK 1.1.0:

Bump armconsumption package version from 1.0.0+patch to 1.1.0+patch
Bump azcore package version from 1.4.0 to 1.9.0 to use the URL encoder helper used in the Azure SDK

The Consumption SDK does not correctly handle query parameters in the 'next link' when the dataset in Azure requires pagination. See elastic/integrations#7478 (comment) for more details.

We manage the patches to the Azure SDK for Go in the repository fork. To learn more about the patches and the fork, see #37151

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.

How to test this PR locally

Our Azure accounts does not have enough usage details items to trigger pagination; the default page size is 1000 items.

You can trigger the pagination by setting the top parameter to a low value (for example, 10 items) when in the pager at

beats/x-pack/metricbeat/module/azure/billing/service.go

Lines 178 to 184 in a9ac1d1

    
           pager := service.usageDetailsClient.NewListPager(scope, &armconsumption.UsageDetailsClientListOptions{ 
        
           	Expand:    &expand, 
        
           	Filter:    &filter, 
        
           	Metric:    &metrictype, 
        
           	StartDate: &startDate, 
        
           	EndDate:   &endDate, 
        
           })

Related issues

mergify · 2023-11-20T16:00:13Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @zmoog? 🙏.
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-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

elasticmachine · 2023-11-20T16:46:38Z

❕ Build Aborted

Expand to view the summary

Build stats

Duration: 48 min 39 sec

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine · 2023-11-20T17:55:16Z

❕ Build Aborted

Expand to view the summary

Build stats

Duration: 81 min 10 sec

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine · 2023-11-20T20:37:25Z

💚 Build Succeeded

Expand to view the summary

Build stats

Duration: 168 min 59 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

shmsr · 2023-11-21T07:19:53Z

The changes here look good. I'll take a look at the patches.

mergify · 2023-11-21T08:34:07Z

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 zmoog/url-encode-next-link-in-azure-billing upstream/zmoog/url-encode-next-link-in-azure-billing
git merge upstream/main
git push upstream zmoog/url-encode-next-link-in-azure-billing

gpop63 · 2023-11-27T23:13:30Z

I did replicate this by setting Top param in UsageDetailsClientListOptions to 10 or 100 to enable pagination (the way @zmoog told me to). Used modern account which I believe uses MCA agreement.

{"log.level":"error","@timestamp":"2023-11-28T00:32:41.751+0200","log.origin":{"file.name":"module/wrapper.go","file.line":256},"message":"Error fetching data for metricset azure.billing: error retrieving usage information: retrieving usage details failed in client: GET https://management.azure.com/subscriptions/REDACTED/providers/Microsoft.Consumption/usageDetails\n--------------------------------------------------------------------------------\nRESPONSE 400: 400 Bad Request\nERROR CODE: MissingApiVersionParameter\n--------------------------------------------------------------------------------\n{\n  \"error\": {\n    \"code\": \"MissingApiVersionParameter\",\n    \"message\": \"The api-version query parameter (?api-version=) is required for all requests.\"\n  }\n}\n--------------------------------------------------------------------------------\n","service.name":"metricbeat","ecs.version":"1.6.0"}

I tried setting the Skiptoken parameter by parsing nextPage.NextLink to see if this is a viable solution, but this does nothing.

muthu-mps · 2023-11-28T08:56:40Z

I did replicate this by setting Top param in UsageDetailsClientListOptions to 10 or 100 to enable pagination (the way @zmoog told me to). Used modern account which I believe uses MCA agreement.

{"log.level":"error","@timestamp":"2023-11-28T00:32:41.751+0200","log.origin":{"file.name":"module/wrapper.go","file.line":256},"message":"Error fetching data for metricset azure.billing: error retrieving usage information: retrieving usage details failed in client: GET https://management.azure.com/subscriptions/REDACTED/providers/Microsoft.Consumption/usageDetails\n--------------------------------------------------------------------------------\nRESPONSE 400: 400 Bad Request\nERROR CODE: MissingApiVersionParameter\n--------------------------------------------------------------------------------\n{\n  \"error\": {\n    \"code\": \"MissingApiVersionParameter\",\n    \"message\": \"The api-version query parameter (?api-version=) is required for all requests.\"\n  }\n}\n--------------------------------------------------------------------------------\n","service.name":"metricbeat","ecs.version":"1.6.0"}

I tried setting the Skiptoken parameter by parsing nextPage.NextLink to see if this is a viable solution, but this does nothing.

@gpop63 - Thanks for verifying the changes.

I have tried with multiple scenarios to verify the expected behaviour and it works as mentioned above.
I am getting the error you have mentioned only when I run the pagination scenario against the main branch.
Can you please double check the code which you have been using to test is from this PR?

gpop63 · 2023-11-28T09:50:35Z

@muthu-mps this fix is great, I was saying I was able to replicate this issue (using the main branch). So the changes here fix it.

muthu-mps · 2023-11-28T10:18:23Z

Thanks @gpop63

muthu-mps · 2023-11-28T12:11:03Z

Azure Billing Metrics

Verified the following scenarios

Pulled the latest code from this PR.
Configured the EA account in the Azure(billing) metricbeat module.
Verified the metrics collection for billing account with default settings. It works fine.
configured the Top parameter as suggested to test the pagination use-case and verified the metrics collection and it works as expected.
Checked whether the data is pulled for the previous date.
The same has been verified against the MCA account and working as expected.

Issue Replication

The issue is reproducible when doing the same against the main branch.

muthu-mps

LGTM!

zmoog · 2023-11-28T16:42:36Z

Hey friends at @elastic/beats-tech-leads, we need your review on this change because it affects go.mod ^^

zmoog · 2023-11-28T16:44:17Z

We're tageting the upcoming 8.12.0 and 8.11.2.

elasticmachine · 2023-11-28T19:22:45Z

💚 Build Succeeded

Expand to view the summary

Build stats

Start Time: 2023-11-28T16:38:30.153+0000
Duration: 164 min 0 sec

Test stats 🧪

Test	Results
Failed	0
Passed	28694
Skipped	2015
Total	30709

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

mergify · 2023-11-28T21:47:17Z

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 zmoog/url-encode-next-link-in-azure-billing upstream/zmoog/url-encode-next-link-in-azure-billing
git merge upstream/main
git push upstream zmoog/url-encode-next-link-in-azure-billing

elasticmachine · 2023-11-29T10:30:02Z

💚 Build Succeeded

Expand to view the summary

Build stats

Duration: 163 min 21 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

tommyers-elastic · 2023-11-29T17:17:29Z

thanks @zmoog ! i would like to tidy up the tag on the fork so the version is a little better defined in the go.mod here.

zmoog · 2023-11-29T17:35:04Z

On it!

The Consumption SDK does not URL encode the query parameters in the next link when the response requires more than one page to fetch all the usage details items. Our Azure SDK for Go fork contains a small patch to URL encode the next link value and avoid the 400 error. We also upgraded the azcore package to use the standard URL helper for the Azure SDK.

I replaced the original commit with a new one with a better comment.

Tidy up the go.mod file by targeting the tag `v1.1.0-elastic` on the Azure SDK fork instead of the commit.

Yes, again.

elasticmachine · 2023-11-29T20:09:33Z

❕ Build Aborted

Expand to view the summary

Build stats

Duration: 126 min 39 sec

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine · 2023-11-29T20:58:53Z

💚 Build Succeeded

Expand to view the summary

Build stats

Start Time: 2023-11-29T18:15:07.324+0000
Duration: 164 min 31 sec

Test stats 🧪

Test	Results
Failed	0
Passed	28694
Skipped	2015
Total	30709

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

zmoog · 2023-11-29T21:11:10Z

@tommyers-elastic, I made the following changes:

I created a PR to merge the patches into our sdk/.../v1.1.0-elastic branch (I shuffled the branches around but kept the commits with the two patches intact).
Added a v1.1.0-elastic tag on the last commit.
Updated the go.mod file to target the v1.1.0-elastic tag.

You now can:

Review the PR Port Azure SDK patches to Consumption API v1.1.0 azure-sdk-for-go#1 on the Azure SDK fork.
Review this PR

NOTICE.txt

tommyers-elastic

LGTM

* Switch to consumption 1.1.0 with patches The Consumption SDK does not URL encode the query parameters in the next link when the response requires more than one page to fetch all the usage details items. Our Azure SDK for Go fork contains a small patch to URL encode the next link value and avoid the 400 error. We also upgraded the azcore package to use the standard URL helper for the Azure SDK. * Update NOTICE.txt * Target tag v1.1.0-elastic instead of the commit Tidy up the go.mod file by targeting the tag `v1.1.0-elastic` on the Azure SDK fork instead of the commit. (cherry picked from commit 9de3307)

* Switch to consumption 1.1.0 with patches The Consumption SDK does not URL encode the query parameters in the next link when the response requires more than one page to fetch all the usage details items. Our Azure SDK for Go fork contains a small patch to URL encode the next link value and avoid the 400 error. We also upgraded the azcore package to use the standard URL helper for the Azure SDK. * Update NOTICE.txt * Target tag v1.1.0-elastic instead of the commit Tidy up the go.mod file by targeting the tag `v1.1.0-elastic` on the Azure SDK fork instead of the commit. (cherry picked from commit 9de3307) # Conflicts: # NOTICE.txt # go.mod # go.sum

… (#37242) * Switch to consumption 1.1.0 with patches The Consumption SDK does not URL encode the query parameters in the next link when the response requires more than one page to fetch all the usage details items. Our Azure SDK for Go fork contains a small patch to URL encode the next link value and avoid the 400 error. We also upgraded the azcore package to use the standard URL helper for the Azure SDK. * Update NOTICE.txt * Target tag v1.1.0-elastic instead of the commit Tidy up the go.mod file by targeting the tag `v1.1.0-elastic` on the Azure SDK fork instead of the commit. (cherry picked from commit 9de3307) Co-authored-by: Maurizio Branca <maurizio.branca@elastic.co>

…ic#37158) * Switch to consumption 1.1.0 with patches The Consumption SDK does not URL encode the query parameters in the next link when the response requires more than one page to fetch all the usage details items. Our Azure SDK for Go fork contains a small patch to URL encode the next link value and avoid the 400 error. We also upgraded the azcore package to use the standard URL helper for the Azure SDK. * Update NOTICE.txt * Target tag v1.1.0-elastic instead of the commit Tidy up the go.mod file by targeting the tag `v1.1.0-elastic` on the Azure SDK fork instead of the commit.

JuddDeaver · 2024-03-26T17:50:54Z

@zmoog I don't see any actual version tags applied to this PR. I have support case 01523048 where they are hitting this known issue. Has his fix made it into the product, and what version?

zmoog · 2024-03-26T18:23:52Z

The fix has been released in Beats and Elastic Agent version 8.11.2.

zmoog · 2024-03-26T18:26:12Z

@JuddDeaver, which tags should I apply to the PR to properly communicate this info?

JuddDeaver · 2024-03-26T18:45:27Z

In that case I think adding the v8.11.2 label would typically be used to indicate the version it was merged. I thought this was usually added by automation, but that might not be an accurate assumption for this repo.

zmoog · 2024-03-26T18:53:44Z

Yeah, it's not a common practice for this repo, but conveying this information with tags/labels or a last comment with a recap makes sense.

I'll do one or the other in current and future PRs.

Thanks for the heads up.

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Nov 20, 2023

zmoog changed the title ~~Switch to Consumption API 1.1.0 with patches~~ [Azure] [Billing] Switch to Consumption API 1.1.0 with patches Nov 20, 2023

zmoog self-assigned this Nov 20, 2023

zmoog added the Team:Cloud-Monitoring Label for the Cloud Monitoring team label Nov 20, 2023

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Nov 20, 2023

zmoog added the bug label Nov 20, 2023

zmoog marked this pull request as ready for review November 20, 2023 17:44

zmoog requested a review from a team as a code owner November 20, 2023 17:44

zmoog requested review from gpop63 and muthu-mps November 20, 2023 17:45

zmoog added the backport-v8.11.0 Automated backport with mergify label Nov 20, 2023

muthu-mps approved these changes Nov 28, 2023

View reviewed changes

zmoog added 5 commits November 29, 2023 19:01

Target a new commit

5dc4f33

I replaced the original commit with a new one with a better comment.

Update NOTICE.txt

33b95f4

Update changelog

5f4b3c7

Target tag v1.1.0-elastic instead of the commit

cfa82f2

Tidy up the go.mod file by targeting the tag `v1.1.0-elastic` on the Azure SDK fork instead of the commit.

zmoog force-pushed the zmoog/url-encode-next-link-in-azure-billing branch from 8dbccb7 to cfa82f2 Compare November 29, 2023 18:03

Update NOTICE.txt

e24a557

Yes, again.

tommyers-elastic reviewed Nov 30, 2023

View reviewed changes

NOTICE.txt Show resolved Hide resolved

tommyers-elastic approved these changes Nov 30, 2023

View reviewed changes

zmoog merged commit 9de3307 into elastic:main Nov 30, 2023
113 checks passed

zmoog deleted the zmoog/url-encode-next-link-in-azure-billing branch November 30, 2023 10:06

mergify bot mentioned this pull request Nov 30, 2023

[8.11](backport #37158) [Azure] [Billing] Switch to Consumption API 1.1.0 with patches #37242

Merged

zmoog added the backport-v7.17.0 Automated backport with mergify label Nov 30, 2023

mergify bot mentioned this pull request Nov 30, 2023

[7.17](backport #37158) [Azure] [Billing] Switch to Consumption API 1.1.0 with patches #37244

Open

zmoog added the v8.11.2 label Mar 26, 2024

	pager := service.usageDetailsClient.NewListPager(scope, &armconsumption.UsageDetailsClientListOptions{
	Expand: &expand,
	Filter: &filter,
	Metric: &metrictype,
	StartDate: &startDate,
	EndDate: &endDate,
	})

[Azure] [Billing] Switch to Consumption API 1.1.0 with patches #37158

[Azure] [Billing] Switch to Consumption API 1.1.0 with patches #37158

Conversation

zmoog commented Nov 20, 2023 • edited

Proposed commit message

Checklist

How to test this PR locally

Related issues

mergify bot commented Nov 20, 2023

elasticmachine commented Nov 20, 2023

❕ Build Aborted

Build stats

🤖 GitHub comments

elasticmachine commented Nov 20, 2023

❕ Build Aborted

Build stats

🤖 GitHub comments

elasticmachine commented Nov 20, 2023

💚 Build Succeeded

Build stats

❕ Flaky test report

🤖 GitHub comments

shmsr commented Nov 21, 2023 • edited

mergify bot commented Nov 21, 2023

gpop63 commented Nov 27, 2023

muthu-mps commented Nov 28, 2023

gpop63 commented Nov 28, 2023

muthu-mps commented Nov 28, 2023

muthu-mps commented Nov 28, 2023

Azure Billing Metrics

Verified the following scenarios

Issue Replication

muthu-mps left a comment

Choose a reason for hiding this comment

zmoog commented Nov 28, 2023

zmoog commented Nov 28, 2023

elasticmachine commented Nov 28, 2023

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

mergify bot commented Nov 28, 2023

elasticmachine commented Nov 29, 2023

💚 Build Succeeded

Build stats

❕ Flaky test report

🤖 GitHub comments

tommyers-elastic commented Nov 29, 2023

zmoog commented Nov 29, 2023

elasticmachine commented Nov 29, 2023

❕ Build Aborted

Build stats

🤖 GitHub comments

elasticmachine commented Nov 29, 2023

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

zmoog commented Nov 29, 2023 • edited

tommyers-elastic left a comment

Choose a reason for hiding this comment

JuddDeaver commented Mar 26, 2024

zmoog commented Mar 26, 2024

zmoog commented Mar 26, 2024

JuddDeaver commented Mar 26, 2024

zmoog commented Mar 26, 2024

zmoog commented Nov 20, 2023 •

edited

shmsr commented Nov 21, 2023 •

edited

zmoog commented Nov 29, 2023 •

edited