Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Close probes concurrently #212

Merged
merged 1 commit into from
Jan 21, 2025

Conversation

pimlu
Copy link
Contributor

@pimlu pimlu commented Jan 16, 2025

What does this PR do?

Modifies the (*Manager) Close() method to close the probes concurrently.

Motivation

The Networks team has dozens of kprobes and each one takes 30-50ms to call the close() syscall on the perf_kprobe PMU, which adds up to an extra second of time on each test. We set up and tear down the tracer entirely for each test, which across all our tests, adds up to minutes of test time.

Additional Notes

Describe how to test your changes

CI passes here showing KMT was all green: DataDog/datadog-agent#33054

@pimlu pimlu marked this pull request as ready for review January 17, 2025 05:22
@pimlu pimlu requested a review from a team as a code owner January 17, 2025 05:22
Copy link
Contributor

@usamasaqib usamasaqib left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Can you share some benchmarks?

@pimlu
Copy link
Contributor Author

pimlu commented Jan 17, 2025

Yes, I needed a separate performance fix in order to properly benchmark this, so both tests are being performed based off that branch.

Methodology: I'm running the CO-RE test suite (we have three suites, so the true number is roughly this times 3)

DD_REMOTE_CONFIGURATION_ENABLED=false sudo -E go test -tags=linux,linux_bpf,npm,process,test ./pkg/network/tracer -v --run TestTracerSuite/CO-RE

Without this PR: Runs CO-RE test suite in 172.042s
With this PR: Runs in 144.262s (19.25% speedup)

@pimlu
Copy link
Contributor Author

pimlu commented Jan 17, 2025

/merge

@dd-devflow
Copy link

dd-devflow bot commented Jan 17, 2025

Devflow running: /merge

View all feedbacks in Devflow UI.


2025-01-17 18:25:00 UTC ℹ️ MergeQueue: waiting for PR to be ready

This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.


2025-01-17 22:25:09 UTC ⚠️ MergeQueue: This merge request was unqueued

devflow unqueued this merge request: It did not become mergeable within the expected time

@pimlu pimlu force-pushed the stuart.geipel/close-probes-concurrently branch from ea577da to 7c888e1 Compare January 21, 2025 22:41
@pimlu
Copy link
Contributor Author

pimlu commented Jan 21, 2025

/merge

@dd-devflow
Copy link

dd-devflow bot commented Jan 21, 2025

Devflow running: /merge

View all feedbacks in Devflow UI.


2025-01-21 22:42:23 UTC ℹ️ MergeQueue: waiting for PR to be ready

This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.


2025-01-21 23:00:35 UTC ⚠️ MergeQueue: This merge request was unqueued

[email protected] unqueued this merge request

@pimlu
Copy link
Contributor Author

pimlu commented Jan 21, 2025

/remove

@dd-devflow
Copy link

dd-devflow bot commented Jan 21, 2025

Devflow running: /remove

View all feedbacks in Devflow UI.


2025-01-21 23:00:33 UTC ℹ️ Devflow: /remove

@pimlu
Copy link
Contributor Author

pimlu commented Jan 21, 2025

/merge

@dd-devflow
Copy link

dd-devflow bot commented Jan 21, 2025

Devflow running: /merge

View all feedbacks in Devflow UI.


2025-01-21 23:00:45 UTC ℹ️ MergeQueue: pull request added to the queue

The median merge time in main is 0s.


2025-01-21 23:05:02 UTC ℹ️ MergeQueue: This merge request was merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants