[Metricbeat] [Azure] Fix monitor namespace empty string

[gpop63]

Proposed commit message

Metricnamespace was always being sent to the Azure Monitor API in the following functions:

• GetMetricDefinitions
• GetMetricValues

The problem is that the Monitor API does not expect empty strings, if there is no namespace it should be sent as nil. This can cause unexpected behaviors. Conditionally sending it fixes this problem.

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.

Author's Checklist

• [ ]

How to test this PR locally

Related issues

• Relates [Azure] Metricbeat fails with error 400 Bad Request when collecting logs using the azure.monitor metricset #36181

Use cases

Screenshots

Logs

[mergify]

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @gpop63? 🙏.
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]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2023-08-30T16:51:34.373+0000

• Duration: 56 min 44 sec

Test stats 🧪

Test	Results
Failed	0
Passed	1478
Skipped	96
Total	1574

💚 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]

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 fix_azure_monitor_namespace upstream/fix_azure_monitor_namespace
git merge upstream/main
git push upstream fix_azure_monitor_namespace

[zmoog]

Relates to #36181

[zmoog]

I understand all integrations have always set a namespace; @gpop63, are you aware of any not using it?

In what use cases do we not set the namespace?

[gpop63]

From what I saw, all data streams that use the monitor metricset have always set a namespace, only the generic monitor data stream allows users to setup their own configs - which is where things can go wrong.

Without this fix it is indeed not working properly since a client could decide to not use any namespace but we still send "" and Azure Monitor API will give 400 Bad Request.

[zmoog]

Without this fix it is indeed not working properly since a client could decide to not use any namespace but we still send "" and Azure Monitor API will give 400 Bad Request.

Yeah, I can see your point about extending the Azure Monitor metricset; this would allow the metricset to support new use cases where users do not want to set a namespace.

However, this change only partially resolves the root problem creating #36181.

The issue #36181 happens because a user made a mistake in writing the configuration, and a blank namespace value slipped into the request causing the status 400. Before this change, there was an implicit assumption the namespace was required. But this was not reflected in the config validator.

If we change the metricset requirements and handle nil namespace values, we avoid the status 400 from happening. However, AFAIK, we have no guarantee the result will always be the same if users set the namespace value to nil by mistake again.

[zmoog]

So I think this is more of a "Add support for the blank namespace in the Azure Monitor metricset" than a "Fix X".

Fixing the error 400 is a (welcome) side-effect of this change. But we're not sure about what can happen when users unintentionally set namespace to blank.

[zmoog]

The change seems sound, but I want to address potential problems.

@gpop63, are there risks in allowing empty namespaces, like an increase in API costs or pulling too many records?

[gpop63]

These errors/behaviors would only happen with the generic monitor data stream. The namespace basically helps you narrow the specific resources you want to retrieve metrics from.

There might be cost differences when not using a namespace since the request will be broader and not targeting specific resources. The primary cost is related to the volume of data collected, rather than the use of a namespace.

Are you suggesting enforcing a rule so the namespace is always required? In the Azure Monitor API the namespace is an optional parameter, users might have different use cases - wouldn't we limit them? @zmoog

[gpop63]

@zmoog From my point of view these are 2 different issues, this one is simply fixing the behavior of the metricset so it works just like before the SDK upgrade (1). Then, we can add additional checks/validation for the config to see if namespace is sent and formatted properly. I might be wrong or missing something.

(1) Before the SDK upgrade no namespaces were allowed, right?

[zmoog]

(1) Before the SDK upgrade no namespaces were allowed, right?

From my testing, the older SDK version targeted an older API version that was tolerant of empty namespaces. It accepted empty values instead of sending "400 Bad Request" as a response.

[zmoog]

this one is simply fixing the behavior of the metricset so it works just like before the SDK upgrade

Namespace	Old SDK	New SDK
With value	Ok	Ok
Empty	Ok	Error

What's the behaviour of the Old SDK version with an empty namespace? If it returns the same values as the New SDK with a nil value, I think we can merge this PR.

@gpop63, do you have time to give these scenarios a try?

[gpop63]

@zmoog just tested both SDKs

• With value they both work as expected
• Old SDK does not care if namespace is empty, no errors thrown and it fetched some metrics
• Empty namespace with new SDK will throw this Received invalid query parameter: [api-version, 2018-01-01],[metricnamespace, ]
• With newer SDK if we send nil, it's basically the same thing as empty value with the older one

[zmoog]

@zmoog just tested both SDKs

• With value they both work as expected
• Old SDK does not care if namespace is empty, no errors thrown and it fetched some metrics
• Empty namespace with new SDK will throw this Received invalid query parameter: [api-version, 2018-01-01],[metricnamespace, ]
• With newer SDK if we send nil, it's basically the same thing as empty value with the older one

Okay.

I am primarily interested in the Old SDK vs. (New SDK + fix) behavior with an empty namespace; the fix allows the New SDK to run without errors.

Will these two return the same data?

[zmoog]

We need to check the Old SDK vs. (New SDK + fix) behaviour with an empty namespace; the fix allows the New SDK to run without errors.

Will these two return the same data?

[gpop63]

@zmoog Yes, from my tests Old SDK vs New SDK (+fix) behave the exact same way with an empty namespace.