Fix bug in filtering, cleanup #31595

fearful-symmetry · 2022-05-11T18:05:13Z

What does this PR do?

This PR fixes a bug results in the top_n filtering options not being applied in system/process, as well as refactors a bunch of components to make the new linter happy.

Why is it important?

This is an actual bug.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
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.

How to test this PR locally

Pull down and build, make sure the include_top_n flags work in metricbeat.

elasticmachine · 2022-05-11T18:05:16Z

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

cmacknz · 2022-05-11T18:10:42Z

libbeat/metric/system/process/helpers.go

 	totalCPUDeltaMillis := int64(s1.CPU.Total.Ticks.ValueOr(0) - s0.CPU.Total.Ticks.ValueOr(0))

-	pct := float64(totalCPUDeltaMillis) / float64(timeDeltaMillis)
+	// In theory this can only happen if the time delta is 0, which is unlikely but possible.
+	if timeDeltaMillis == 0 {


You are doing this comparison against a float now, which leaves it open to floating point precisions issues comparing against an exact value.

Ah, good point.

cmacknz · 2022-05-11T18:11:09Z

libbeat/metric/system/process/process.go

 	}
 	// We use this to track processes over time.
 	procStats.ProcsMap = pidMap

 	// filter the process list that will be passed down to users
-	procStats.includeTopProcesses(plist)
+	plist = procStats.includeTopProcesses(plist)


Is this the primary bug?

cmacknz · 2022-05-11T18:12:37Z

libbeat/metric/system/process/process_test.go

+	t.Logf("Proc: %s", procData[0].StringToPrint())
+}
+
+func TestFilter(t *testing.T) {


This was the missing test from the looks of things?

Yah, added that.

elasticmachine · 2022-05-11T18:27:16Z

❕ Build Aborted

There is a new build on-going so the previous on-going builds have been aborted.

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-05-11T18:55:19.757+0000
Duration: 362 min 29 sec

Test stats 🧪

Test	Results
Failed	0
Passed	17084
Skipped	1282
Total	18366

Steps errors

Expand to view the steps failures

`Checks if running on a Unix-like node`

Took 0 min 0 sec . View more details here

`Error signal`

Took 0 min 0 sec . View more details here
Description: untar: step failed with error null

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

fearful-symmetry · 2022-05-11T19:43:45Z

@kvch have we solved the "typecheck failing on stuff that has cgo" issue yet?

fearful-symmetry · 2022-05-12T16:56:57Z

After talking with @kvch, I'm going to merge this without the linters passing, as we don't really care about them here, and the rest of the tests are passing. Tested this myself with a full build of metricbeat.

* fix bug in filtering, cleanup * continuing to make linter happy * deal with platform-specific code issues (cherry picked from commit 2a6624d) # Conflicts: # libbeat/metric/system/process/process.go # libbeat/metric/system/process/process_darwin.go # libbeat/metric/system/process/process_linux_common.go

* Fix bug in filtering, cleanup (#31595) * fix bug in filtering, cleanup * continuing to make linter happy * deal with platform-specific code issues (cherry picked from commit 2a6624d) # Conflicts: # libbeat/metric/system/process/process.go # libbeat/metric/system/process/process_darwin.go # libbeat/metric/system/process/process_linux_common.go * fix cherry-pick * format again Co-authored-by: Alex K <8418476+fearful-symmetry@users.noreply.github.com> Co-authored-by: Alex Kristiansen <alex.kristiansen@elastic.co>

* fix bug in filtering, cleanup * continuing to make linter happy * deal with platform-specific code issues

fix bug in filtering, cleanup

5819ea9

fearful-symmetry added bug Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team backport-v8.2.0 Automated backport with mergify labels May 11, 2022

fearful-symmetry requested a review from a team as a code owner May 11, 2022 18:05

fearful-symmetry self-assigned this May 11, 2022

fearful-symmetry requested review from cmacknz and faec and removed request for a team May 11, 2022 18:05

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 May 11, 2022

cmacknz reviewed May 11, 2022

View reviewed changes

fearful-symmetry added 2 commits May 11, 2022 11:54

continuing to make linter happy

9f0cb5b

deal with platform-specific code issues

ba779ba

faec approved these changes May 12, 2022

View reviewed changes

cmacknz approved these changes May 12, 2022

View reviewed changes

fearful-symmetry merged commit 2a6624d into elastic:main May 12, 2022

mergify bot mentioned this pull request May 12, 2022

[8.2](backport #31595) Fix bug in filtering, cleanup #31617

Merged

kvch mentioned this pull request May 16, 2022

Fix build tags for darwin after refactoring of metrics handles elastic/elastic-agent-system-metrics#18

Merged

kvch added a commit to kvch/elastic-agent-system-metrics that referenced this pull request May 16, 2022

Pick process list filtering fix from elastic/beats#31595

29a47cb

kvch added a commit to elastic/elastic-agent-system-metrics that referenced this pull request May 17, 2022

Pick process list filtering fix from elastic/beats#31595 (#19)

02d84e6

fearful-symmetry mentioned this pull request May 17, 2022

Add missing changelog entry for system/process fixes #31645

Merged

chrisberkhout pushed a commit that referenced this pull request Jun 1, 2023

Fix bug in filtering, cleanup (#31595)

c4b9b83

* fix bug in filtering, cleanup * continuing to make linter happy * deal with platform-specific code issues

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug in filtering, cleanup #31595

Fix bug in filtering, cleanup #31595

fearful-symmetry commented May 11, 2022

elasticmachine commented May 11, 2022

cmacknz May 11, 2022

fearful-symmetry May 11, 2022

cmacknz May 11, 2022

fearful-symmetry May 11, 2022

cmacknz May 11, 2022

fearful-symmetry May 11, 2022

elasticmachine commented May 11, 2022 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

`Checks if running on a Unix-like node`

`Error signal`

fearful-symmetry commented May 11, 2022

fearful-symmetry commented May 12, 2022

Fix bug in filtering, cleanup #31595

Fix bug in filtering, cleanup #31595

Conversation

fearful-symmetry commented May 11, 2022

What does this PR do?

Why is it important?

Checklist

How to test this PR locally

elasticmachine commented May 11, 2022

cmacknz May 11, 2022

Choose a reason for hiding this comment

fearful-symmetry May 11, 2022

Choose a reason for hiding this comment

cmacknz May 11, 2022

Choose a reason for hiding this comment

fearful-symmetry May 11, 2022

Choose a reason for hiding this comment

cmacknz May 11, 2022

Choose a reason for hiding this comment

fearful-symmetry May 11, 2022

Choose a reason for hiding this comment

elasticmachine commented May 11, 2022 • edited by jenkins-beats-ci bot Loading

❕ Build Aborted

Build stats

Test stats 🧪

Steps errors

Checks if running on a Unix-like node

Error signal

🤖 GitHub comments

fearful-symmetry commented May 11, 2022

fearful-symmetry commented May 12, 2022

elasticmachine commented May 11, 2022 •

edited by jenkins-beats-ci bot

Loading

`Checks if running on a Unix-like node`

`Error signal`