Skip to content

Commit

Permalink
[Metricbeat] gcp: fix instance machineType reporting (#27363)
Browse files Browse the repository at this point in the history
Refresh instance metadata for each incoming response by updating the
`s.computeMetadata` field in the `metadataCollector` struct.

The code was applying a sort of caching to the computeMetadata, but in
the wrong place. Most probably it was a leftover, as a proper caching
for instance metadata is implemented in the `instance` function on line
146.
Due to this check `computeMetadata` were not refreshed for each
response.
`metadataCollector` is instanciated only once for the entire module.
Thus by not overriding `computeMetadata` the metadata of the first
response handled was used for all subsequent events.

Removing the check make the code update the `s.computeMetadata` property
for each new response. With this update the correct metadata based on
the instance ID reported in the response are used.

Fixes a bug were all metrics were being reported with the same
`machineType`.

NOTE: the code update the same struct field at each iteration; this is
most probably not safe in a parallel execution context; this code is not
running multiple times in different threads so for the moment this
change should not have negative side effects.
  • Loading branch information
endorama committed Aug 17, 2021
1 parent 928f9c5 commit 5339327
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 5 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.next.asciidoc
Expand Up @@ -407,6 +407,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d
- Fix cloudwatch metricset collecting duplicate data points. {pull}27248[27248]
- Fix flaky test TestAddCounterInvalidArgWhenQueryClosed. {issue}27312[27312] {pull}27313[27313]
- Add percent formatters to system/process {pull}27374[27374]
- Fix instance machineType reporting in compute metricset of GCP module {pull}27363[27363]

*Packetbeat*

Expand Down
11 changes: 6 additions & 5 deletions x-pack/metricbeat/module/gcp/metrics/compute/metadata.go
Expand Up @@ -61,11 +61,11 @@ type metadataCollector struct {

// Metadata implements googlecloud.MetadataCollector to the known set of labels from a Compute TimeSeries single point of data.
func (s *metadataCollector) Metadata(ctx context.Context, resp *monitoringpb.TimeSeries) (gcp.MetadataCollectorData, error) {
if s.computeMetadata == nil {
_, err := s.instanceMetadata(ctx, s.instanceID(resp), s.instanceZone(resp))
if err != nil {
return gcp.MetadataCollectorData{}, err
}
// NOTE: ignoring the return value because instanceMetadata changes s.computeMetadata in place.
// This is probably not thread safe.
_, err := s.instanceMetadata(ctx, s.instanceID(resp), s.instanceZone(resp))
if err != nil {
return gcp.MetadataCollectorData{}, err
}

stackdriverLabels := gcp.NewStackdriverMetadataServiceForTimeSeries(resp)
Expand Down Expand Up @@ -107,6 +107,7 @@ func (s *metadataCollector) Metadata(ctx context.Context, resp *monitoringpb.Tim

// instanceMetadata returns the labels of an instance
func (s *metadataCollector) instanceMetadata(ctx context.Context, instanceID, zone string) (*computeMetadata, error) {
// FIXME: remove side effect on metadataCollector instance and use return value instead
i, err := s.instance(ctx, instanceID, zone)
if err != nil {
return nil, errors.Wrapf(err, "error trying to get data from instance '%s' in zone '%s'", instanceID, zone)
Expand Down

0 comments on commit 5339327

Please sign in to comment.