Add in-buffer batching to NPM #28040

brycekahle · 2024-07-29T21:46:35Z

What does this PR do?

Reworks perf/ring buffer abstraction and usage to something less fragile.
Adds the option network_config.enable_kernel_batching for NPM. This enables the status quo custom batching, and is true by default. Disabling this option allows the use of in-buffer batching for perf buffers.
Changes to callbacks for the data flow from perf/ring buffers because of channel overhead.

Motivation

Decreased usage of eBPF stack space, allowing the size of the eBPF data structure to increase.
Runtime flexibility on how many events to in-buffer batch for perf buffers. Allowing a tradeoff between buffer size and userspace CPU usage.

Additional Notes

USM batching was not modified because it has a different system. This can be reworked in a future PR.

https://datadoghq.atlassian.net/browse/EBPF-481

Possible Drawbacks / Trade-offs

Ring buffer usage by default is discouraged for NPM/USM. This is because neither product is benefiting from the ordering of ring buffers, nor the reserve helper call to not utilize stack space. Using in-buffer batching with perf buffers results in lower CPU usage.
perf/ring buffer sizes need to be re-evaluated and probably increased. This is due to removing the userspace buffer and in-buffer batching utilizing more of the buffer space before data is read.

Describe how to test/QA your changes

guyarb

Did you run the branch in staging?
did you run load test to measure the impact of the change?
is there any reason to keep a large PR and a large change both to USM and NPM together?

ogaca-dd

LGMT for files owned by ASC

ogaca-dd · 2024-07-30T09:13:24Z

pkg/util/encoding/binary.go

+	}
+
+	return func(buf []byte) {
+		if len(buf) == 0 {


Is there a reason to call callback with nil as the first argument when len(buf) == 0 and not always create the object with newFn()?
This is the behaviour I would expect without reading the comment of this function.

Note: I would also allow you to use type constraint for T because you would need to use *T.

Yes, to differentiate between an empty buffer and a buffer with zeroes.

brycekahle · 2024-07-30T18:30:29Z

did you run load test to measure the impact of the change?

Yes, which is how I'm able to give guidance about the CPU usage of ring buffers. I did a bunch of load tests comparing different setups (perf buffers, ring buffers, channels, callbacks).

brycekahle · 2024-07-30T18:31:47Z

is there any reason to keep a large PR and a large change both to USM and NPM together?

I did not change USM batching, just to how it consumes events from a perf/ring buffer.

cit-pr-commenter · 2024-07-30T19:58:57Z

Go Package Import Differences

Baseline: fc52f43
Comparison: d66a9b0

binary

os

arch

change

system-probe

linux

amd64

+3, -0

+github.com/DataDog/datadog-agent/pkg/ebpf/perf
+github.com/DataDog/datadog-agent/pkg/util/encoding
+github.com/DataDog/datadog-agent/pkg/util/slices

system-probe

linux

arm64

+3, -0

+github.com/DataDog/datadog-agent/pkg/ebpf/perf
+github.com/DataDog/datadog-agent/pkg/util/encoding
+github.com/DataDog/datadog-agent/pkg/util/slices

pr-commenter · 2024-07-30T20:34:24Z

Test changes on VM

Use this command from test-infra-definitions to manually test this PR changes on a VM:

inv create-vm --pipeline-id=40520591 --os-family=ubuntu

Note: This applies to commit d66a9b0

pr-commenter · 2024-07-30T20:58:47Z

Regression Detector

Regression Detector Results

Run ID: 0361c001-397d-4921-8985-c0e05a36e375 Metrics dashboard Target profiles

Baseline: fc52f43
Comparison: d66a9b0

Performance changes are noted in the perf column of each table:

✅ = significantly better comparison variant performance
❌ = significantly worse comparison variant performance
➖ = no significant change in performance

No significant changes in experiment optimization goals

Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%

There were no significant changes in experiment optimization goals at this confidence level and effect size tolerance.

Fine details of change detection per experiment

perf	experiment	goal	Δ mean %	Δ mean % CI	links
➖	tcp_syslog_to_blackhole	ingress throughput	+3.74	[-9.15, +16.63]	Logs
➖	pycheck_1000_100byte_tags	% cpu utilization	+2.55	[-2.28, +7.38]	Logs
➖	otel_to_otel_logs	ingress throughput	+0.58	[-0.23, +1.39]	Logs
➖	basic_py_check	% cpu utilization	+0.03	[-2.58, +2.64]	Logs
➖	uds_dogstatsd_to_api	ingress throughput	-0.00	[-0.00, +0.00]	Logs
➖	tcp_dd_logs_filter_exclude	ingress throughput	-0.00	[-0.01, +0.01]	Logs
➖	idle	memory utilization	-0.11	[-0.15, -0.08]	Logs
➖	uds_dogstatsd_to_api_cpu	% cpu utilization	-0.85	[-1.74, +0.03]	Logs
➖	file_tree	memory utilization	-3.69	[-3.77, -3.60]	Logs

Explanation

A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".

For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:

Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
Its configuration does not mark it "erratic".

brycekahle · 2024-07-30T21:12:55Z

Some data from load testing comparing main to branch, perf buffers and using in-buffer batching on the branch:

prevent unnecessary import in other binaries

guyarb · 2024-08-01T03:00:01Z

And what about dogfooding?

guyarb

I'm not comfortable with USM changes here, the PR changes too many things all at once.
I'd like to exclude USM from the changes in this PR

leeavital · 2024-08-05T12:26:13Z

Agreed with Guy here, this PR too big. Individual bits can't reverted independently if we find problems, and there are too many parts changing and not enough detail in the commit message to allow for a good review.

Is there a reason it can't be broken up?

akarpz · 2024-08-05T19:01:48Z

@leeavital some pieces have been broken out already (see the linked PRs above), not sure if Bryce has plans to do more, or if it's reasonable

brycekahle · 2024-11-23T00:12:50Z

Superseded by #31402

brycekahle added changelog/no-changelog team/ebpf-platform qa/done QA done before merge and regressions are covered by tests labels Jul 29, 2024

brycekahle added this to the 7.57.0 milestone Jul 29, 2024

brycekahle requested review from a team as code owners July 29, 2024 21:46

github-actions bot added the component/system-probe label Jul 29, 2024

guyarb reviewed Jul 30, 2024

View reviewed changes

ogaca-dd approved these changes Jul 30, 2024

View reviewed changes

brycekahle added 8 commits July 30, 2024 12:47

in buffer batching with perf buffers

626a612

do not copy ConnectionStats until we have to

d7182d8

no npm channels

f4c7cb3

callback for sharedlibraries

3ccf353

usm callbacks

be50564

remove CallbackChannel

cb7122d

fix object leak on binary unmarshal error

ea83484

minor style fixes

fa14516

brycekahle force-pushed the bryce.kahle/no-perf-channel branch from 107c3f7 to fa14516 Compare July 30, 2024 19:47

use linux_bpf build tag for perf package

d1baaaf

brycekahle added 2 commits July 30, 2024 14:38

slightly stricter build tag

770b913

prevent unnecessary import in other binaries

fix build tag on TestStatsOverflow

d66a9b0

richl-dd approved these changes Jul 31, 2024

View reviewed changes

guyarb requested changes Aug 1, 2024

View reviewed changes

brycekahle marked this pull request as draft August 2, 2024 18:37

This was referenced Aug 2, 2024

do not copy ConnectionStats until we have to #28173

Merged

add NPM connection batch extractor #28174

Merged

brycekahle modified the milestones: 7.57.0, Triage Aug 8, 2024

brycekahle closed this Nov 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add in-buffer batching to NPM #28040

Add in-buffer batching to NPM #28040

brycekahle commented Jul 29, 2024 •

edited

Loading

guyarb left a comment

ogaca-dd left a comment

ogaca-dd Jul 30, 2024

brycekahle Jul 30, 2024

brycekahle commented Jul 30, 2024

brycekahle commented Jul 30, 2024

cit-pr-commenter bot commented Jul 30, 2024 •

edited

Loading

pr-commenter bot commented Jul 30, 2024 •

edited

Loading

pr-commenter bot commented Jul 30, 2024 •

edited

Loading

Fine details of change detection per experiment

Explanation

brycekahle commented Jul 30, 2024

guyarb commented Aug 1, 2024

guyarb left a comment

leeavital commented Aug 5, 2024

akarpz commented Aug 5, 2024

brycekahle commented Nov 23, 2024

Add in-buffer batching to NPM #28040

Add in-buffer batching to NPM #28040

Conversation

brycekahle commented Jul 29, 2024 • edited Loading

What does this PR do?

Motivation

Additional Notes

Possible Drawbacks / Trade-offs

Describe how to test/QA your changes

guyarb left a comment

Choose a reason for hiding this comment

ogaca-dd left a comment

Choose a reason for hiding this comment

ogaca-dd Jul 30, 2024

Choose a reason for hiding this comment

brycekahle Jul 30, 2024

Choose a reason for hiding this comment

brycekahle commented Jul 30, 2024

brycekahle commented Jul 30, 2024

cit-pr-commenter bot commented Jul 30, 2024 • edited Loading

Go Package Import Differences

pr-commenter bot commented Jul 30, 2024 • edited Loading

Test changes on VM

pr-commenter bot commented Jul 30, 2024 • edited Loading

Regression Detector

Regression Detector Results

No significant changes in experiment optimization goals

Fine details of change detection per experiment

Explanation

brycekahle commented Jul 30, 2024

guyarb commented Aug 1, 2024

guyarb left a comment

Choose a reason for hiding this comment

leeavital commented Aug 5, 2024

akarpz commented Aug 5, 2024

brycekahle commented Nov 23, 2024

brycekahle commented Jul 29, 2024 •

edited

Loading

cit-pr-commenter bot commented Jul 30, 2024 •

edited

Loading

pr-commenter bot commented Jul 30, 2024 •

edited

Loading

pr-commenter bot commented Jul 30, 2024 •

edited

Loading