fix thread safety in process code

[fearful-symmetry]

What does this PR do?

This fixes elastic/beats#32467

This code originates from metricbeat, which never worried too much about threadsafety, since a single metricset just lives in its own thread. However, the code has carried over to beat's self-monitoring in report/. A previous PR fixed a bug where GetSelf() would not calculate percentages, as it never accessed its own internal map of processes that it uses to calculate percentages. However, setup.go was creating a single instance of the process Stat struct to send to multiple function callbacks in the monitoring, hence a concurrent map access.

Wanted to get this PR up fast, still testing on non-linux systems.

Why is it important?

Potential concurrent hashmap access issue.

Checklist

• My code follows the style guidelines of this project
• I have commented my code, particularly in hard-to-understand areas
• I have added an entry in CHANGELOG.md
• Test on darwin
• Test on Windows

[cmacknz]

To unblock 8.3.3 as safely as possible we are going to downgrade the system metrics package in affected versions: elastic/beats#32467 (comment)

We will merge this to beats main and 8.3 after the 8.3.3 release completes (really after the next build candidate is generated).

[cmacknz]

Is there a test we can write to prove this is thread safe? Maybe the test we need should be added in beats?

[elasticmachine]

💚 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

• Start Time: 2022-07-25T18:09:13.223+0000

• Duration: 9 min 58 sec

Test stats 🧪

Test	Results
Failed	0
Passed	173
Skipped	3
Total	176

🤖 GitHub comments

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

• /test : Re-trigger the build.

[fearful-symmetry]

Is there a test we can write to prove this is thread safe? Maybe the test we need should be added in beats?

@cmacknz Yah, I was wondering about that. If anything, I think the test should go in the report library, since it doesn't have any tests, and that's where the threaded code actually originated.

[cmacknz]

Yah, I was wondering about that. If anything, I think the test should go in the report library, since it doesn't have any tests, and that's where the threaded code actually originated.

Should we just document that this code isn't thread safe and put the locking in beats? I haven't looked at that code yet so I don't know how hard it is, but it seems a bit weird for the fix and the test to live in two different places.

[fearful-symmetry]

Should we just document that this code isn't thread safe and put the locking in beats?

Well, theoretically it should be threadsafe now. The reporting/monitoring code that actually creates and spins up per-thread callbacks are in setup.go and elastic-agent-libs/reporting, so any further thread safety fixes would have to go there.

If we wanted to be extra paranoid, we could have just have each monitoring callback function have its own instance of Stat, but if we're confident this code is thread safe that seems unnecessary.

[fearful-symmetry]

FWIW, a test on the monitoring code would almost be more an integration test, since we're hitting both the monitoring subsystem and any metrics code attached to it.

[fearful-symmetry]

Added a small test to the report library. System metrics like this are always a tad awkward to write tests for, since a lot of metrics will just be context/OS dependent.

[cmacknz]

Needs a CHANGELOG entry :)

[LeeDr]

fearful-symmetry merged commit c8f77ef into elastic:main 17 days ago

This commit may have been made to the 8.3 branch, but it was made well after 8.3.0 was released so I think this should have made it into the 8.3.3 release and should be labeled with that.