Support of servicelabel config in GCP Metrics Metricset #41626
Conversation
botelastic
bot
added
the
needs_team
|
This pull request doesn't have a |
|
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
|
|
mergify
bot
added
the
backport-8.x
| // isAnExistingService returns true if the given service is already an existing service supported by default in GCP module | ||
| func isAnExistingService(serviceName string) bool { | ||
| switch serviceName { | ||
| case gcp.ServiceCompute, gcp.ServiceGKE, gcp.ServiceStorage, gcp.ServiceDataproc, | ||
| gcp.ServicePubsub, gcp.ServiceLoadBalancing, gcp.ServiceCloudFunctions, | ||
| gcp.ServiceFirestore, gcp.ServiceCloudSQL, gcp.ServiceRedis: | ||
| return true | ||
| default: | ||
| return false | ||
| } | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
I don't think we need this functionality, we can just overwrite the label if ServiceLabel is set in config and that should be sufficient. If we decide to set the label from the integration side due to some GCP updates or whatever, having this function would actually defeat what we’re trying to achieve in the first place.
There was a problem hiding this comment.
The reason for not allowing override in existing supported services is for the filtering to work fine by using the existing location labels.
Yes, if GCP changes some label for these existing services at their end, we will have to do beats change for those existing services in beats module.
We can think of not having this function, but if existing customer puts in an incorrect label, it can break for him.
There was a problem hiding this comment.
I wouldn't expose this setting to users in normal GCP integrations. I think we should control this config option and update when necessary. I think it makes sense to allow users to set it for the GCP metrics input. What do you think?
| @@ -103,6 +103,7 @@ type config struct { | |||
| Zone string `config:"zone"` | |||
| Region string `config:"region"` | |||
| Regions []string `config:"regions"` | |||
| ServiceLabel string `config:"servicelabel"` | |||
There was a problem hiding this comment.
I wonder if LocationLabel (location_label) would be a better name for this config param. ServiceLabel feels too generic it could refer to any label, but here we’re specifically referring to region/zone/location labels 🤔
| * *servicelabel*: This config option is to add the resource label( eg:"resource.label.location") for a | ||
| GCP service to filter the data of the service. For a new service which is not supported by default | ||
| by Elastic, this label can be set by the user. If not set, tje default servicelabel (resource.label.zone) | ||
| will be chosen |
There was a problem hiding this comment.
| * *servicelabel*: This config option is to add the resource label( eg:"resource.label.location") for a | |
| GCP service to filter the data of the service. For a new service which is not supported by default | |
| by Elastic, this label can be set by the user. If not set, tje default servicelabel (resource.label.zone) | |
| will be chosen | |
| * *location_label*: Use this option to specify the resource label that identifies the location (such as zone or region) for a Google Cloud service when filtering metrics. For example, labels like `resource.labels.zone` or `resource.labels.region` are used by Google Cloud to represent the zone or region of a resource. |
There was a problem hiding this comment.
We can go with location label. Lets finalize on the approach we want to take for existing services. Will update the description then
Proposed commit message
Explain here the changes you made on the PR.
Please explain:
servicelabelconfig option to get the resource label for the new GCP service from the usermetrics metricsetwas used for a new GCP service, the resource label used for data filtering was always the default "resource.label.zone", which may/may not be correct depending on the service. This PR get this label through config from the user.Checklist
CHANGELOG.next.asciidocorCHANGELOG-developer.next.asciidoc.Related issues
Screenshots