Small improvements on the azure module #13859

narph · 2019-10-01T11:04:01Z

Work on validation and adding conditions
Small work on azure docs

jsoriano

Added some suggestions about documentation, and please review the changelog, I think we are missing an entry for the Azure module.

jsoriano · 2019-10-03T09:42:54Z

CHANGELOG.next.asciidoc

@@ -386,6 +386,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d
 - Add support for NATS version 2. {pull}13601[13601]
 - Add `docker.cpu.*.norm.pct` metrics for `cpu` metricset of Docker Metricbeat module. {pull}13695[13695]
 - Add `instance` label by default when using Prometheus collector. {pull}13737[13737]
+- Add small improvements in the azure module (validation, doc update) {pull}13859[13859]


Umm, we should have an entry about the new Azure module, but I think we forgot it in #13196
As the module is not released yet we wouldn't need this new entry.
Could you replace this one with one about adding the module itself?

I have added both entries in the list, is there a reason the second one should be removed?

@narph My understanding is changelog is user facing. Since this is not released yet, they don't need to know how many PRs we spent to add the initial pr, the improvements, the docs and etc. But I agree with Jaime, the new module needs to be an entry by itself 😬

I have merged the 2 PRs in one comment line

metricbeat/docs/modules/azure.asciidoc

jsoriano · 2019-10-03T10:49:37Z

metricbeat/docs/modules/azure.asciidoc

+
+`tenant_id` - The unique identifier of the Azure Active Directory instance
+
+Users can use the azure credentials keys if configured `AZURE_CLIENT_ID`, `AZURE_CLIENT_SECRET`, `AZURE_TENANT_ID`, `AZURE_SUBSCRIPTION_ID`


This part could be moved to some other file so it can be reused in other features that need to authenticate with Azure AD in the future.

I have cleaned up the documentation and touched on your findings as well. At the moment this is the only place where it will be used. Do you have any recommendations where the information regarding credentials should be moved?

We have some shared doc files in libbeat/docs/shared-*. We can wait to move this content to a shared file when we need it in any case.

sounds good

metricbeat/docs/modules/azure.asciidoc

x-pack/metricbeat/module/azure/compute_vm/_meta/docs.asciidoc

x-pack/metricbeat/module/azure/compute_vm_scaleset/_meta/docs.asciidoc

kaiyan-sheng · 2019-10-03T13:17:07Z

x-pack/metricbeat/module/azure/_meta/docs.asciidoc

+
+Costs: Metric queries are charged based on the number of standard API calls. More information on pricing here https://azure.microsoft.com/id-id/pricing/details/monitor/.
+
+Authentication: we are handling authentication on our side (creating/renewing the authentication token), so we advise users to use dedicated credentials for metricbeat only.


Just curious, are there a set of permissions user need to give this account in order for azure module to collect the metrics?

More information on the authentication and the steps to require a service principal are found higher in the docs.asciidoc in the === Module-specific configuration notes.
I have just added the link to the RBAC as well, users will need to assign a role that allows them to read monitoring data, minimum effort would be "Monitoring Reader"

CHANGELOG.next.asciidoc

jsoriano · 2019-10-03T17:02:45Z

x-pack/metricbeat/module/azure/compute_vm/client_helper.go

@@ -32,8 +35,14 @@ func mapMetric(client *azure.Client, metric azure.MetricConfig, resource resourc
 		if len(*metricDefinitions.Value) == 0 {
 			return nil, errors.Errorf("no metric definitions were found for resource %s and namespace %s.", *resource.ID, *namespace.Properties.MetricNamespaceName)
 		}
+		var filteredMetricDefinitions []insights.MetricDefinition
+		for _, metricDefinition := range *metricDefinitions.Value {
+			if (metricDefinition.Name.LocalizedValue != nil && !strings.Contains(*metricDefinition.Name.LocalizedValue, "(Deprecated)")) || metricDefinition.Name.LocalizedValue == nil {


I think this would be simpler and equivalent

Suggested change

if (metricDefinition.Name.LocalizedValue != nil && !strings.Contains(*metricDefinition.Name.LocalizedValue, "(Deprecated)")) || metricDefinition.Name.LocalizedValue == nil {

if metricDefinition.Name.LocalizedValue == nil || !strings.Contains(*metricDefinition.Name.LocalizedValue, "(Deprecated)")) {

Btw, why are we filtering out deprecated values?

Can the use of "localized" values be a problem if languages different to English are used?

It seems that the API will not return the "deprecated" message although it will do so for the "preview". Non the less, testing different languages for the metric localized name, indeed should translate the "deprecated" word as well (https://docs.microsoft.com/nl-NL/azure/azure-monitor/platform/metrics-supported#microsoftcomputevirtualmachines , for nl version "afgeschaft"). I have removed the entire condition in both places.

x-pack/metricbeat/module/azure/compute_vm_scaleset/compute_vm_scaleset.go

x-pack/metricbeat/module/azure/azure.go

jsoriano

LGTM, just a couple of extra small suggestions

jsoriano · 2019-10-08T16:08:27Z

x-pack/metricbeat/module/azure/azure.go

+	}
+
+	//validate config based on metricset
+	switch base.Name() {


Could this be metricsetName?

Suggested change

switch base.Name() {

switch metricsetName {

jsoriano · 2019-10-08T16:10:15Z

x-pack/metricbeat/module/azure/azure.go

+
+// FetchValues methods implements the data gathering and data conversion to the right metricset
+func (m *MetricSet) FetchValues(fn mapMetric, report mb.ReporterV2) error {
+	err := m.Client.InitResources(fn, report)


If fn would be an attribute of the metricset I think that this FetchValues could be the Fetch method of all derived metricsets.

Suggested change

err := m.Client.InitResources(fn, report)

err := m.Client.InitResources(m.mapMetricFn, report)

I like the suggestion, I have made the changes

narph added 2 commits October 1, 2019 09:18

Refactoring and improvements

4697da6

Work on documentation

b6dae7f

narph requested a review from a team as a code owner October 1, 2019 11:04

narph self-assigned this Oct 1, 2019

narph added Team:Integrations Label for the Integrations team [zube]: In Progress Metricbeat Metricbeat labels Oct 1, 2019

zube bot added [zube]: Inbox and removed [zube]: In Progress labels Oct 1, 2019

andresrc added [zube]: In Progress and removed [zube]: Inbox labels Oct 1, 2019

narph added 4 commits October 1, 2019 14:52

Add validation on config

192ee23

Update changelog file

fde4955

fix double records

8eb6829

Merge branch 'master' into metricbeat-azure

58b7142

narph assigned jsoriano and unassigned jsoriano Oct 3, 2019

narph requested a review from jsoriano October 3, 2019 07:59

jsoriano reviewed Oct 3, 2019

View reviewed changes

Work on documentation

55a206f

narph requested a review from jsoriano October 3, 2019 13:04

kaiyan-sheng reviewed Oct 3, 2019

View reviewed changes

jsoriano reviewed Oct 3, 2019

View reviewed changes

Work on review feedback

c9dc70b

houndci-bot reviewed Oct 7, 2019

View reviewed changes

x-pack/metricbeat/module/azure/azure.go Show resolved Hide resolved

houndci-bot reviewed Oct 7, 2019

View reviewed changes

x-pack/metricbeat/module/azure/azure.go Show resolved Hide resolved

houndci-bot reviewed Oct 7, 2019

View reviewed changes

x-pack/metricbeat/module/azure/azure.go Show resolved Hide resolved

narph added 2 commits October 7, 2019 14:27

fix comment

8ce8d66

remove "deprecated" condition

2f11e9e

narph requested a review from jsoriano October 7, 2019 12:53

narph added [zube]: In Review and removed [zube]: In Progress labels Oct 8, 2019

jsoriano approved these changes Oct 8, 2019

View reviewed changes

narph added 3 commits October 8, 2019 20:53

Small changes

6b2dac4

Removing Fetch

cb4b698

Fix dimension name

bbf8b5e

jsoriano approved these changes Oct 9, 2019

View reviewed changes

narph merged commit 1fb41ea into elastic:master Oct 9, 2019

narph deleted the metricbeat-azure branch October 9, 2019 10:05

zube bot added [zube]: Done and removed [zube]: In Review labels Oct 9, 2019

andresrc added v7.5.0 and removed [zube]: Done labels Oct 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Small improvements on the azure module #13859

Small improvements on the azure module #13859

narph commented Oct 1, 2019

jsoriano left a comment

jsoriano Oct 3, 2019

narph Oct 3, 2019

kaiyan-sheng Oct 3, 2019

narph Oct 7, 2019

jsoriano Oct 3, 2019

narph Oct 3, 2019

jsoriano Oct 3, 2019

narph Oct 7, 2019

kaiyan-sheng Oct 3, 2019

narph Oct 7, 2019

jsoriano Oct 3, 2019

narph Oct 7, 2019 •

edited

Loading

jsoriano left a comment

jsoriano Oct 8, 2019

jsoriano Oct 8, 2019

narph Oct 9, 2019

jsoriano Oct 9, 2019


		`tenant_id` - The unique identifier of the Azure Active Directory instance

		Users can use the azure credentials keys if configured `AZURE_CLIENT_ID`, `AZURE_CLIENT_SECRET`, `AZURE_TENANT_ID`, `AZURE_SUBSCRIPTION_ID`


		Costs: Metric queries are charged based on the number of standard API calls. More information on pricing here https://azure.microsoft.com/id-id/pricing/details/monitor/.

		Authentication: we are handling authentication on our side (creating/renewing the authentication token), so we advise users to use dedicated credentials for metricbeat only.

	if (metricDefinition.Name.LocalizedValue != nil && !strings.Contains(*metricDefinition.Name.LocalizedValue, "(Deprecated)")) \|\| metricDefinition.Name.LocalizedValue == nil {
	if metricDefinition.Name.LocalizedValue == nil \|\| !strings.Contains(*metricDefinition.Name.LocalizedValue, "(Deprecated)")) {

	err := m.Client.InitResources(fn, report)
	err := m.Client.InitResources(m.mapMetricFn, report)

Small improvements on the azure module #13859

Small improvements on the azure module #13859

Conversation

narph commented Oct 1, 2019

jsoriano 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

Choose a reason for hiding this comment

narph Oct 7, 2019 • edited Loading

Choose a reason for hiding this comment

jsoriano 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

narph Oct 7, 2019 •

edited

Loading