Avoid sending non-numeric floats in cloud foundry integrations #22634

jsoriano · 2020-11-17T17:59:55Z

What does this PR do?

Cloud Foundry integrations are sending some values as they are received
from the Firehose, some of these values can be floats with non-numeric
values (NaN/Inf), that are not supported by JSON and Elasticsearch.

I haven't been able to reproduce this, but there are only two places
where Metricbeat can receive non-numeric floats.

Add defensive code to avoid sending these values to the outputs:

In the container metricset, remove cpu percentages with non-numeric values.
In the value metricset, drop whole events with non-numeric values.

Add some helpers and mocks to be able to test the module with specific envelopes.

Why is it important?

Sending non-numeric float values to some outputs is problematic.

There were only integration tests for this module, these tests are not executed in CI yet.
With the added helpers we can test the module without needing a CF environment.

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.

Related issues

Fixes Handle NaNs in Cloud Foundry metrics #22599
Related to [Meta] Cloud Foundry Support #16520

elasticmachine · 2020-11-17T18:48:14Z

💚 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

Build Cause: [Pull request #22634 updated]
Start Time: 2020-11-24T16:18:06.047+0000
Duration: 47 min 19 sec

Test stats 🧪

Test	Results
Failed	0
Passed	4959
Skipped	354
Total	5313

Steps errors

Expand to view the steps failures

`Terraform Apply on x-pack/metricbeat/module/aws`

Took 0 min 15 sec . View more details on here

`Terraform Apply on x-pack/metricbeat/module/aws`

Took 0 min 16 sec . View more details on here

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	4959
Skipped	354
Total	5313

Cloud Foundry integrations are sending some values as they are received from the Firehose, some of these values can be floats with non-numeric values (NaN/Inf), that are not supported by JSON and Elasticsearch. Add defensive code to avoid sending these values to the outputs.

elasticmachine · 2020-11-20T18:04:42Z

Pinging @elastic/integrations-platforms (Team:Platforms)

x-pack/metricbeat/module/cloudfoundry/value/value.go

x-pack/metricbeat/module/cloudfoundry/container/container.go

ycombinator · 2020-11-23T08:28:50Z

x-pack/metricbeat/module/cloudfoundry/value/value.go

+	nameKey := "cloudfoundry.value.name"
+	if v, err := event.RootFields.GetValue(valueKey); err == nil {
+		if v, ok := v.(float64); ok && (math.IsNaN(v) || math.IsInf(v, 0)) {
+			name, _ := event.RootFields.GetValue(nameKey)


Should we do any error checking here?

Ok, I will produce a different message if the name is not found.

ycombinator

The interfaces to enable mocking for unit testing are 👍👍.

Just left a couple of questions around error checking.

… code to check floats

ycombinator

LGTM

jsoriano · 2020-11-24T16:20:03Z

@ycombinator leveraging that I moved the common code to a helper function, I have added a unit test for it, please take a quick look after this change. Thanks!

ycombinator

Still LGTM.

…ic#22634) Cloud Foundry integrations are sending some values as they are received from the Firehose, some of these values can be floats with non-numeric values (NaN/Inf), that are not supported by JSON and Elasticsearch. Add defensive code to avoid sending these values to the outputs. Also, add unit tests using mocked cloud foundry hubs. (cherry picked from commit 0619788)

… (#22750) Cloud Foundry integrations are sending some values as they are received from the Firehose, some of these values can be floats with non-numeric values (NaN/Inf), that are not supported by JSON and Elasticsearch. Add defensive code to avoid sending these values to the outputs. Also, add unit tests using mocked cloud foundry hubs. (cherry picked from commit 0619788)

… (#22751) Cloud Foundry integrations are sending some values as they are received from the Firehose, some of these values can be floats with non-numeric values (NaN/Inf), that are not supported by JSON and Elasticsearch. Add defensive code to avoid sending these values to the outputs. Also, add unit tests using mocked cloud foundry hubs. (cherry picked from commit 0619788)

…-issues * upstream/master: (41 commits) Fix version parser regex for packaging (elastic#22581) Fix local_dynamic documentation and add providers inline doc. (elastic#22657) fix: use proper param name for e2e tests (elastic#22836) [Heartbeat] Fix exit on disabled monitor (elastic#22829) Update Golang to 1.14.12 (elastic#22790) docs: fix setup.template.overwrite typos (elastic#22804) Add docs section for ECS EC2 monitoring (elastic#22784) Fixing logic to keep list of unique cluster UUIDs (elastic#22808) Skip somewhat flaky UDP system test on Windows (elastic#22810) Fix polling node when it is not ready and monitor by hostname (elastic#22666) Skip Filebeat test_shutdown on windows 7 (elastic#22797) Make monitoring Namespace thread-safe (elastic#22640) Drop pkt_dstaddr and pkt_srcaddr when equals to "-" (elastic#22721) Add support for reading from UNIX datagram sockets (elastic#22699) Fix export dashboard command from Elastic Cloud (elastic#22746) Skip flaky winlogbeat test on Windows-7 (elastic#22754) Missing `>` (elastic#22763) (elastic#22766) Fix k8s watcher issue when node access to list nodes and ns (elastic#22714) [Metricbeat/Kibana/stats] Enforce `exclude_usage=true` (elastic#22732) Avoid sending non-numeric floats in cloud foundry integrations (elastic#22634) ...

…dows-7 * upstream/master: (41 commits) Fix version parser regex for packaging (elastic#22581) Fix local_dynamic documentation and add providers inline doc. (elastic#22657) fix: use proper param name for e2e tests (elastic#22836) [Heartbeat] Fix exit on disabled monitor (elastic#22829) Update Golang to 1.14.12 (elastic#22790) docs: fix setup.template.overwrite typos (elastic#22804) Add docs section for ECS EC2 monitoring (elastic#22784) Fixing logic to keep list of unique cluster UUIDs (elastic#22808) Skip somewhat flaky UDP system test on Windows (elastic#22810) Fix polling node when it is not ready and monitor by hostname (elastic#22666) Skip Filebeat test_shutdown on windows 7 (elastic#22797) Make monitoring Namespace thread-safe (elastic#22640) Drop pkt_dstaddr and pkt_srcaddr when equals to "-" (elastic#22721) Add support for reading from UNIX datagram sockets (elastic#22699) Fix export dashboard command from Elastic Cloud (elastic#22746) Skip flaky winlogbeat test on Windows-7 (elastic#22754) Missing `>` (elastic#22763) (elastic#22766) Fix k8s watcher issue when node access to list nodes and ns (elastic#22714) [Metricbeat/Kibana/stats] Enforce `exclude_usage=true` (elastic#22732) Avoid sending non-numeric floats in cloud foundry integrations (elastic#22634) ...

jsoriano added the Team:Platforms Label for the Integrations - Platforms team label Nov 17, 2020

jsoriano self-assigned this Nov 17, 2020

botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Nov 17, 2020

jsoriano force-pushed the cf-not-numeric-floats branch 2 times, most recently from cb9e045 to cce865e Compare November 20, 2020 17:44

Add unit tests using mocked cloud foundry hubs

52f72fc

jsoriano force-pushed the cf-not-numeric-floats branch from cce865e to 52f72fc Compare November 20, 2020 17:49

jsoriano added the needs_backport PR is waiting to be backported to other branches. label Nov 20, 2020

Add changelog

f27c19e

jsoriano added v7.10.1 v7.11.0 bug labels Nov 20, 2020

jsoriano marked this pull request as ready for review November 20, 2020 18:04

jsoriano requested a review from ycombinator November 20, 2020 18:04

ycombinator reviewed Nov 23, 2020

View reviewed changes

x-pack/metricbeat/module/cloudfoundry/value/value.go Show resolved Hide resolved

ycombinator reviewed Nov 23, 2020

View reviewed changes

x-pack/metricbeat/module/cloudfoundry/container/container.go Show resolved Hide resolved

ycombinator reviewed Nov 23, 2020

View reviewed changes

jsoriano added 3 commits November 23, 2020 13:04

Log value name only if available when ignoring value event

ca3f516

Small refactor to log unexpected errors at the debug level, and share…

1460069

… code to check floats

Merge remote-tracking branch 'origin/master' into cf-not-numeric-floats

b2c06b6

ycombinator approved these changes Nov 24, 2020

View reviewed changes

Add test for helper function

0376252

ycombinator approved these changes Nov 24, 2020

View reviewed changes

jsoriano merged commit 0619788 into elastic:master Nov 25, 2020

jsoriano deleted the cf-not-numeric-floats branch November 25, 2020 10:23

jsoriano mentioned this pull request Nov 25, 2020

Cherry-pick #22634 to 7.x: Avoid sending non-numeric floats in cloud foundry integrations #22750

Merged

6 tasks

jsoriano removed the needs_backport PR is waiting to be backported to other branches. label Nov 25, 2020

jsoriano mentioned this pull request Nov 25, 2020

Cherry-pick #22634 to 7.10: Avoid sending non-numeric floats in cloud foundry integrations #22751

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid sending non-numeric floats in cloud foundry integrations #22634

Avoid sending non-numeric floats in cloud foundry integrations #22634

jsoriano commented Nov 17, 2020 •

edited

Loading

elasticmachine commented Nov 17, 2020 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

`Terraform Apply on x-pack/metricbeat/module/aws`

`Terraform Apply on x-pack/metricbeat/module/aws`

Test stats 🧪

elasticmachine commented Nov 20, 2020

ycombinator Nov 23, 2020

jsoriano Nov 23, 2020

ycombinator left a comment

ycombinator left a comment

jsoriano commented Nov 24, 2020

ycombinator left a comment

Avoid sending non-numeric floats in cloud foundry integrations #22634

Avoid sending non-numeric floats in cloud foundry integrations #22634

Conversation

jsoriano commented Nov 17, 2020 • edited Loading

What does this PR do?

Why is it important?

Checklist

Related issues

elasticmachine commented Nov 17, 2020 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

Steps errors

Terraform Apply on x-pack/metricbeat/module/aws

Terraform Apply on x-pack/metricbeat/module/aws

💚 Flaky test report

Test stats 🧪

elasticmachine commented Nov 20, 2020

ycombinator Nov 23, 2020

Choose a reason for hiding this comment

jsoriano Nov 23, 2020

Choose a reason for hiding this comment

ycombinator left a comment

Choose a reason for hiding this comment

ycombinator left a comment

Choose a reason for hiding this comment

jsoriano commented Nov 24, 2020

ycombinator left a comment

Choose a reason for hiding this comment

jsoriano commented Nov 17, 2020 •

edited

Loading

elasticmachine commented Nov 17, 2020 •

edited by jenkins-beats-ci bot

Loading

`Terraform Apply on x-pack/metricbeat/module/aws`

`Terraform Apply on x-pack/metricbeat/module/aws`