[Metricbeat] Add Load Balancing metricset to Google Cloud Platform module

[sayden]

Adds Load Balancing metricset to Google Cloud Platform module.

We must take a look at the format of the distribution data that is returned by GCP in case we want to omit them as the "nature" of the lightweight module doesn't allow special modifications in the incoming data from the "engine"

c.f. #15811

[mtojek]

Nice! Only a couple of nitpicks.

[mtojek]

nit: you can sort all fields for searching purposes

[sayden]

Good idea, indeed. Sorted.

[mtojek]

nit:s/Metricset/metricset/g

[sayden]

Done

[mtojek]

nit: please sort

[sayden]

Aaand sorted

[mtojek]

nit: case googlecloud.ServicePubsub, googlecloud.ServiceLoadBalancing:

[sayden]

done

[mtojek]

nit: case googlecloud.ServicePubsub, googlecloud.ServiceLoadBalancing:

[sayden]

done

[exekias]

what do you think about returning nil, nil by default when the service is not supported? This way you don't need to keep adding services when adding a new metricset

[mtojek]

... or the other way round - nil, nil for supported services except customized ones. The code will need to be adjusted if only you need a custom call to NewMetadataService. Unsupported service will still report an error.

[exekias]

problem with that is that the list of services is forever growing (see they are just constants we defined in our side). Every time a new service is added to stackdriver we would need to add it here, with little pay off, as this is a function to optionally enrich events when we know how to retrieve their metadata. It should do nothing for the rest.

[mtojek]

What do you think about such construct? The order is important, that's the only requirement, but we can cover it with unit tests:

func NewMetadataServiceForConfig(c config) (googlecloud.MetadataService, error) {
	switch c.ServiceName {
	case googlecloud.ServiceCompute:
		return compute.NewMetadataService(c.ProjectID, c.Zone, c.opt...)
	case supportedServicesAsEnum():
		return nil, nil
	default:
		return nil, errors.Errorf("service '%s' not supported", c.ServiceName)
	}
}

supportedServicesAsEnum() would read all defined constant values.

[exekias]

I think I understand what you propose. My concern comes from this scenario:

I'm happily using the stackdriver metricset with Metricbeat 7.6, I use it to monitor all my GCP services. After some time GCP adds a new service to their offering, I try to monitor it with stackdriver metricset and get this error: service 'foo' not supported. Why is it not supported if the way to collect metrics is always the same?

Then I will need to wait for the Beats team to include the service name in this enum and release a new version, then upgrade to it. All this because of how we handle metadata, which is, at the end, optional

[mtojek]

Agree here, but this implicates getting rid of constants at all.

EDIT:

To be clear - I'm not blocking this PR. Feel free to go!

[sayden]

I see your points. The thing was to support 3 cases here:

• A correct / known GCP Service which is supported, having or not a specific implementation for labels (potentially we should aim to reach the highest quality and to have implementations for each metricset).
• A correct GCP Service which is not supported: Service recognized but labelling is not supported yet. Action point -> Open enhancement request.
• An incorrect / unknown GCP service: Service not recognized -> Action point: check service name.

The problem is that in the first PR we removed all "future metricsets" as a request and this function stay, looking naive.

One thing that is worth mention: When adding a new metricset, you don't "just" add the metrics you want to fetch. You must add the fields.yml and the docs.asciidoc and, potentially, you might add an implementation to fetch labels if the current metrics looks incomplete. I like this explicit approach where my future self will face a "metricset not recognized" while developing a new metricset if I don't pass by here to see that a labelling implementation might be needed.

I don't want to discourage comments here but this was discussed in the main module PR 😅 We can change it here but I think it must go in a different PR (because we have 3 live PR's which are using this code right now).