pkg/dyninst: Fix procscan backoff - #54400
Conversation
grantseltzer
added
changelog/no-changelog
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b05f0e86a5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Files inventory check summaryFile checks results against ancestor 8db82fe9: Results for datadog-agent_7.84.0~devel.git.57.c64775e.pipeline.130016266-1_amd64.deb:No change detected Results for datadog-iot-agent_7.84.0~devel.git.57.c64775e.pipeline.130016266-1_amd64.deb:No change detected |
Static quality checks✅ Please find below the results from static quality gates Successful checksInfo
19 successful checks with minimal change (< 2 KiB)
|
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: 78d82eb Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_logs | % cpu utilization | +1.66 | [+0.79, +2.53] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle_all_features | memory utilization | +0.22 | [+0.18, +0.26] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | +0.21 | [+0.17, +0.26] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_no_fs_load | memory utilization | -0.05 | [-0.15, +0.04] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_metrics_logs | memory utilization | -0.06 | [-0.30, +0.19] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_mean_fs_load | memory utilization | -0.11 | [-0.15, -0.08] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_private_action_runner | memory utilization | -0.59 | [-0.71, -0.47] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_idle | memory utilization | -0.70 | [-0.76, -0.64] | 1 | Logs bounds checks dashboard |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | quality_gate_idle | intake_connections | 10/10 | 3 ≤ 4 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | 147.51MiB ≤ 154MiB | bounds checks dashboard |
| ✅ | quality_gate_idle | total_bytes_received | 10/10 | 730.74KiB ≤ 819.20KiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | 3 ≤ 4 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | 491.79MiB ≤ 512MiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | total_bytes_received | 10/10 | 1.13MiB ≤ 1.25MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 17 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 185.15MiB ≤ 195MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_logs | total_bytes_received | 10/10 | 264.17MiB ≤ 292MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 359.98 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 19 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 412.64MiB ≤ 430MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | total_bytes_received | 10/10 | 0.94GiB ≤ 1.04GiB | bounds checks dashboard |
| ✅ | quality_gate_private_action_runner | memory_usage | 10/10 | 71.46MiB ≤ 75MiB | bounds checks dashboard |
| ✅ | quality_gate_security_idle | cpu_usage | 10/10 | 29.43 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_idle | memory_usage | 10/10 | 300.90MiB ≤ 330MiB | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | cpu_usage | 10/10 | 60.40 ≤ 200 | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | memory_usage | 10/10 | 275.05MiB ≤ 310MiB | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | cpu_usage | 10/10 | 20.82 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | memory_usage | 10/10 | 285.79MiB ≤ 320MiB | bounds checks dashboard |
Explanation
Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%
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
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".
Replicate Execution Details
We run multiple replicates for each experiment/variant. However, we allow replicates to be automatically retried if there are any failures, up to 8 times, at which point the replicate is marked dead and we are unable to run analysis for the entire experiment. We call each of these attempts at running replicates a replicate execution. This section lists all replicate executions that failed due to the target crashing or being oom killed.
Note: In the below tables we bucket failures by experiment, variant, and failure type. For each of these buckets we list out the replicate indexes that failed with an annotation signifying how many times said replicate failed with the given failure mode. In the below example the baseline variant of the experiment named experiment_with_failures had two replicates that failed by oom kills. Replicate 0, which failed 8 executions, and replicate 1 which failed 6 executions, all with the same failure mode.
| Experiment | Variant | Replicates | Failure | Logs | Debug Dashboard |
|---|---|---|---|---|---|
| experiment_with_failures | baseline | 0 (x8) 1 (x6) | Oom killed | Debug Dashboard |
The debug dashboard links will take you to a debugging dashboard specifically designed to investigate replicate execution failures.
❌ Retried Profiling Replicate Execution Failures (ddprof)
Note: Profiling replicas may still be executing. See the debug dashboard for up to date status.
| Experiment | Variant | Replicates | Failure | Debug Dashboard |
|---|---|---|---|---|
| quality_gate_idle_all_features | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_idle_all_features | comparison | 10 | Oom killed | Debug Dashboard |
| quality_gate_logs | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_logs | comparison | 10 | Oom killed | Debug Dashboard |
| quality_gate_metrics_logs | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_metrics_logs | comparison | 10 | Oom killed | Debug Dashboard |
| quality_gate_security_idle | baseline | 10 | Crashed (exit code: 134) | Debug Dashboard |
| quality_gate_security_idle | comparison | 10 | Crashed (exit code: 134) | Debug Dashboard |
| quality_gate_security_mean_fs_load | comparison | 10 | Crashed (exit code: 134) | Debug Dashboard |
| quality_gate_security_no_fs_load | baseline | 10 | Crashed (exit code: 134) | Debug Dashboard |
CI Pass/Fail Decision
✅ Passed. All Quality Gates passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_private_action_runner, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
5f13038 to
d18f2c0
Compare
|
Bits Code status: ✅ Done Comment @DataDog to request changes |
|
Now that i've refined this cleanly, here's a good simple explanation that i've verified: Every five seconds the agent walks the list of running processes. For each one it asks a short series of questions, cheapest first: have I already instrumented this, is it even due for another look yet, what program is it running, was that program built with Go, and has its tracer published the details we need. The first question that comes back "no" or "not yet" ends the examination of that process for this round. Most processes stop at the second question for the cost of a single lookup, which is what keeps looking at everything every five seconds affordable. The important part is what happens after a "no". Nothing is ever a final answer. The process goes back in the queue and gets looked at again later, with the wait doubling each time. The first look lands within five seconds of the process starting, then roughly five seconds later, then fifteen, thirty five, seventy five, and so on up to a ceiling of five minutes, repeating at that ceiling for as long as the process lives. So a service whose tracer takes ten seconds to announce itself is picked up on the second or third look instead of being missed forever, and a startup script that spends a minute preparing before launching the real service is picked up shortly after that launch. The only thing that takes a process out of consideration is the process exiting. Two supporting details make it hold together. A process is identified by its id together with the moment it started, not by its id alone, so when the system recycles an id we never mistake the new process for the dead one. And any process a scan does not see is forgotten on the spot, so the bookkeeping can never grow larger than the number of things actually running. |
| // candidate is the retry state for a process that has not been instrumented | ||
| // yet, whatever the reason. | ||
| type candidate struct { |
There was a problem hiding this comment.
Are we going to have 1 of these structs in memory, potentially forever? candidate naming convention seems off compared to the description
There was a problem hiding this comment.
Nope, the first scan that doesn't see the process deletes it, so nothing outlives the process it describes.
One per running process that isn't instrumented yet, so it's bounded by live processes, which the kernel bounds by pid_max. Each entry is about 32 bytes and the first scan that doesn't see the process deletes it, so nothing outlives the process it describes.
|
|
||
| // WithRetryBackoff sets the delay before the first retry of a tracer metadata | ||
| // read and the cap that the doubling delay grows to. | ||
| func WithRetryBackoff(base, maxDelay time.Duration) Option { |
There was a problem hiding this comment.
There aren't pre-existing utilities for backoff?
There was a problem hiding this comment.
Claude originally pulled in a util but we needed to change it and it became more trouble than it was worth. It was pkg/util/backoff. It randomizes the delay inside a range as opposed to a simple delay, Also it works in float64 seconds while everything here is based on ticks.
It was implementing all of that and I was like for fucks sake it's 5 lines of code.
|
|
||
| // mustNewLRU panics if the cache creation fails, which only happens if the size | ||
| // is non-positive. | ||
| func mustNewLRU[K comparable, V any](size int) *lru.Cache[K, V] { |
There was a problem hiding this comment.
I wish more library developers would ship with a Must
| // The start time is read on every scan rather than cached by pid: it is | ||
| // the only thing that tells us whether this is still the same process, | ||
| // so a cached copy would be exactly as stale as the answer we need. | ||
| startTime, err := p.readStartTime(int32(pid)) |
There was a problem hiding this comment.
Any performance concerns doing this every time?
There was a problem hiding this comment.
It is the main cost of a scan but it's deliberate. The start time is the only thing that tells us a pid still refers to the same proc so we cannot cache it.
That said it's a small impact. It's a open+read+close syscall for each process every 5s. The read isn't hitting disk, procfs is completely in memory. I ran the benchmarks on my machine and it was 21ms of reads per scan. I expect there's a small fixed cost and it scales relatively linearly with number of procs from there.
| p.scheduleRetry(key, c, now) | ||
| continue | ||
| } | ||
| if tracerMetadata.TracerLanguage != "go" { |
There was a problem hiding this comment.
Could TracerLanguage ever be an empty string?
There was a problem hiding this comment.
Yea I don't think it's validated anywhere. But even if the payload is missing the language completely it'll fall under != go and have the behavior that we want. It'll get retried (as opposed to written off forever how it used to).
12bee1c to
21a7a58
Compare
Removes the per verdict counters the scanner kept and reported through the system-probe stats endpoint. Nothing consumed them beyond that endpoint, no telemetry metric or monitor was built on them, and ten counters is more than a debug only surface earns. The distinction between a tracer that has not published its metadata yet and one whose metadata cannot be read now survives only as a log level, since the scanner retries both on the same schedule, and the snapshot timelines assert the retry schedule directly through the candidate state instead. The executable cache dedup, which was the one thing the counters were carrying that nothing else checked, moves to TestSyntheticProcfsScanWork, which now pins the number of binaries a first scan parses against real files rather than merely asserting it is non zero; that assertion is stronger than the counter's, and was verified to fail when the cache is disabled. The scan cadence stats are kept, since they report the interval this change caps.
Replaces the adaptive scan cadence with a plain five second tick. The interval used to be the scan interval plus a hundred times however long the last scan took, capped at thirty seconds, which held scanning to one percent of a core but meant the interval was never actually the configured value: on a host with two thousand processes it settled around five seconds, and the first scan after a restart pushed the next one out past thirteen. That is a lot of machinery and a lot of explaining for an optimization nothing has asked for yet, so it goes; if scan cost turns out to matter it can come back with evidence behind it. The max scan interval goes with it, since that ceiling existed only to bound the penalty term, as does the reported scan interval, since a constant is not worth reporting. The jitter goes too: its factor was never set outside tests, so it had been multiplying every interval by exactly one, and the scans are local work that nothing downstream can be stampeded by. The interval moves from three seconds to five because every process on the host costs a stat read on every scan, which the calibration benchmark puts at twelve microseconds against real procfs, or about one percent of a core at two thousand processes every three seconds; five buys most of that back and still leaves three retries inside a process' first minute. The retry backoff base moves with it, since it exists to put the first retry on the next scan and a shorter value would buy nothing. The test that pinned the ceiling now pins the interval instead, and was verified to fail if the penalty is reintroduced.
Swaps the candidate set from an LRU cache to a map. The set needs no bound of its own: an entry appears when a process is evaluated and is dropped by the first scan that does not see the process, so it cannot outgrow the number of live processes, which the kernel already bounds by pid_max. The cap was that same bound written down a second time, and the eviction path it guarded is unreachable in practice, so it took a constant, an option, a config field and a snapshot timeline with it. Sweeping exited candidates now deletes while ranging rather than allocating a slice of every key on every scan, worth about sixteen kilobytes a scan on a host with two thousand candidates. This also removes an empty if statement left behind when the scanner counters were deleted, which compiled and passed vet because the eviction result was still read in the condition.
A process found not to be running a Go binary is now never looked at again, rather than re-examined on a one minute ceiling. Its candidate entry records that verdict and every later scan skips it for the cost of the stat read that enumerating pids costs anyway; the entry goes away when the process exits, as any other does. This reverses the exec accommodation: a shell entrypoint that later execs the real service keeps its pid and start time, so that service is now missed for as long as it runs. That trade is deliberate, and it takes with it the second backoff ceiling, its option, its config field, the executable key each candidate carried, the exec detection that reset a schedule, and the timeline covering it. The saving is simplicity rather than cost: the one minute ceiling already recovered nearly all of what re-examining these processes was spending, so steady state at two thousand processes is unmoved at around 0.44% of a core.
Deletes the scanner's own copy of the tracer memfd search and calls tracermetadata.GetTracerMetadata again, as the code did before this branch. The copy existed to tell a process that has published no metadata apart from one whose descriptors we are not allowed to read, because the shared helper reports both as not found, and that distinction fed two counters that have since been removed. All it bought after that was a warning rather than a trace line for the permission case, which the same scan already declines to warn about when resolving an executable fails the same way. The metadata read now goes through the same rate limited logging as every other step, with a not found result added to the errors that step considers uninteresting, since most processes on a host have published nothing. Both paths were the same search already, fd three first and then the rest of the directory, and both cap the read at the same sixty four kilobytes.
Trims the comments that were restating what the code says, renames the ELF section set to goELFSections, and shortens the note on the candidate map. A Go binary whose tracer reports some other language is now written off rather than retried forever: nothing about that answer is going to change, so it belongs with the binaries that are not Go at all. The write-off itself moves into a small helper now that two branches reach it, which is also where the note about exec keeping a pid and start time now lives.
Removes the one second floor a process had to clear before the scanner would look at it. It was load bearing when a process got a single look at a fixed age and a wrong guess meant a permanent miss; with the loop retrying, looking early and being wrong costs nothing. What it was costing is discovery latency: with a five second tick, only processes born in the last second before a scan were affected, and those had their first look pushed out a whole interval, so roughly a fifth of all processes waited an extra five seconds to be found. Nothing downstream needs the floor. Discovery on its own does no actuator work, since a process with no probes is ignored by the state machine, and probes only arrive after Remote Config answers for that runtime id, which is a network round trip that no short lived process survives. If a process does vanish mid attach, the attach error is caught, the program is unloaded and the entry is reaped when the removal arrives. The floor did also cover the window between fork and exec, where a child still shows its parent's executable and would now be written off permanently, but that window is microseconds and the process would have to be a traced Go service to matter.
A process found not to be running a Go binary, or whose tracer reports a language other than Go, was recorded as never instrumentable and skipped by every later scan. exec keeps a process' pid and start time, so a wrapper script that was still running its entrypoint the first time we looked at it took the service it later became down with it, for as long as that service ran. That is the same permanent miss this branch set out to remove, and the old windowed scanner did not have it, since it looked again at 100s and 1000s and never inspected the executable at all. Both verdicts now go on the same backoff as every other one, so exiting is once again the only terminal state, and the write-off, its never sentinel and the snapshot field that reported it are gone. The cap is now read from the scanner rather than passed down, since there is one of them again. Measured on the same host as the other numbers on this branch, resolving an executable against real procfs costs 14.8us, so re-resolving every one of two thousand processes once per five minute cap is 0.01% of a core amortized, against the 0.44% a scan already costs to read their start times; the steady state benchmark is unmoved at 0.45%. Validated with a new timeline that walks a wrapper through three looks, execs it into a Go binary at the same pid and start time, and shows the service discovered on the next look due.
Takes the reviewer's wording for the executable filter's doc comment and drops the doc comments from the retry constants. Rewords the scan interval constant, which called itself the delay between two scans when it is the delay between the end of one and the start of the next, so a slow scan pushes the following one out by its own duration. Also cuts a sentence from the seenAt field that restated what the sweep reading it already documents.
…uration The scan loop rested a fixed five seconds after every scan, so the fraction of a core it spent was the scan's own duration over five seconds and nothing bounded it. A scan reads the start time of every process on the host and searches the open descriptors of those due for a retry, and neither the number of processes nor the number of descriptors any one of them holds is under the scanner's control, so that fraction is not something the code can predict: a half second scan is nine percent of a core, and a five second one is half of it. The rest is now the larger of the interval and twenty times how long the scan took, which holds the loop under about one twentieth of a core however slow a scan becomes. This is a safety valve rather than a return of the adaptive cadence that was removed earlier on this branch: that one added a penalty on top of the base so the interval was never the configured value, and capped the penalty so the bound broke down once scans got slow enough, whereas this leaves the interval at exactly five seconds on every host we have measured, a scan at two thousand processes taking tens of milliseconds against the two hundred and fifty the floor would need to engage. Validated with a new test that drives a scan slow enough to trip the floor and asserts the longer rest, verified to fail without the change, and by reworking the test that pins the fixed interval to use a scan short enough to stay inside the budget while still being long enough that a penalty proportional to it would be obvious.
2cb3ed4 to
be6ee6c
Compare
… for a memfd Searching a process' open descriptors for a tracer memfd read the whole /proc/<pid>/fd directory into memory before examining any of it, which allocates a string for every descriptor the process holds. That was tolerable when the search ran three times in a process' lifetime, but the reconcile loop now runs it on a backoff for the life of every Go process that has not published metadata, and a service holding two hundred thousand descriptors turns each of those into a ten megabyte allocation that is discarded immediately. The directory is now read five hundred and twelve names at a time and each name is checked as it arrives, so the search holds a fixed amount of memory and stops at the first match instead of always paying for the whole table. procfs generates the directory by walking the descriptor table in order, so the chunks arrive lowest descriptor first, which is where a memfd published early in a process' startup sits; the quick path that checks descriptor three still runs first and is unchanged. Nothing about which processes are found changes, only what it costs to find them. Validated with a new test, the first this function has had, that pushes a memfd past the first chunk and asserts the search still finds it at its exact path, along with a negative case, and verified to fail when the search reads only one chunk.
### What does this PR do? Bump `agent_rpm_arm64`/`agent_suse_arm64`'s `max_on_disk_size` using `quality-gates.exception-threshold-bump`'s fix from #54615: the only two gates PR #54400 is actually short on headroom for. Also align each RPM/SUSE twin gate pair's `max_on_wire_size` up to the higher of the two (`agent_rpm_amd64_fips`, `agent_rpm_arm64_fips`, `agent_suse_arm64`, `iot_agent_suse_amd64`): RPM and SUSE packages are the same format, so a twin pair drifting apart is noise, not signal. ### Motivation PR #54400 (an incident fix) is blocked by these two gates, both at zero margin on `main` after #54204's Rust data-security check. A rebase alone left it short by 34 KiB. ### Describe how you validated your changes Confirmed against PR #54400's live metrics: it currently fails only `agent_rpm_arm64`/`agent_suse_arm64` on-disk, by 34.26 KiB; against these new thresholds, none of its 33 gates fail. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
be6ee6c to
cbff2c2
Compare
### What does this PR do? Bump `agent_rpm_arm64`/`agent_suse_arm64`'s `max_on_disk_size` using `quality-gates.exception-threshold-bump`'s fix from #54615: the only two gates PR #54400 is actually short on headroom for. Also align each RPM/SUSE twin gate pair's `max_on_wire_size` up to the higher of the two (`agent_rpm_amd64_fips`, `agent_rpm_arm64_fips`, `agent_suse_arm64`, `iot_agent_suse_amd64`): RPM and SUSE packages are the same format, so a twin pair drifting apart is noise, not signal. ### Motivation #54400 (an incident fix) is blocked by these two gates, both at zero margin on `main` after #54204's Rust data-security check. A rebase alone left it short by 34 KiB. ### Describe how you validated your changes Confirmed against #54400's live metrics: it currently fails only `agent_rpm_arm64`/`agent_suse_arm64` on-disk, by 34.26 KiB. Against these new thresholds, none of its 33 gates fail. Co-authored-by: regis.desgroppes <regis.desgroppes@datadoghq.com>
| // Go binary. | ||
| checkGoExecutable func(path string) (bool, error) | ||
|
|
||
| goExecutables *lru.Cache[process.FileKey, bool] |
There was a problem hiding this comment.
can you please add a comment on the relationship between goExecutables and candidates?
Is candidates only for Go processes? If so, spell that out in a comment.
| ), | ||
| }) | ||
| } | ||
| func NewScanner(procfsRoot string, opts ...Option) *Scanner { |
There was a problem hiding this comment.
nit: I see a single caller; consider hard-coding the options inside the constructor. Perhaps using options is not necessary.
| // yet, whatever the reason. | ||
| type candidate struct { | ||
| // startTime distinguishes this process from a later one that reuses its pid. | ||
| startTime ticks |
There was a problem hiding this comment.
Is dealing with ticks worth it? Particularly now that the Scanner also deals with durations expressed in normal units, could we use time.Time here for nextAttempt and seenAt? I can understand that ticks is useful for startTime maybe in order to deal with the process key and make it cheap to see if the process is newly discovered or not, but I'm guessing ticks are not useful beyond that.
Anyway, take it or leave it.
There was a problem hiding this comment.
That start time is purely to distinguish processes from one another, ticks make sense because that's what we get from procfs. No need to convert.
There was a problem hiding this comment.
Ok you were right, I misunderstood your comment. Implemented.
…in maps The scanner kept its live process set in a btree and its Go-binary verdicts in an LRU, both instantiated over types that appear nowhere else in the binary. Go stencils generics per GC shape, so a btree keyed on a named 16-byte struct emits a fresh copy of every method and embeds the fully qualified shape name once per symbol, which then lands in the pclntab, rodata and DWARF; that metadata rather than the code itself accounted for most of a 63 KB growth in system-probe, which pushed the arm64 agent package past a static quality gate that had almost no headroom left. Neither structure needed what it was chosen for: both consumers of the live set treat it as a set keyed by pid and never observe its order, and the executable cache only needs a bound, which two generations of plain map give for a kilobyte where the LRU cost thirty one. Dropping both leaves system-probe 30 KB smaller than main, since the btree instantiation the scanner used to pay for is gone too. Validated by building system-probe before and after and diffing per-symbol and per-section sizes, and by running the procscan and procsubscribe tests under the race detector, whose snapshots now sort explicitly where they previously leaned on btree ordering.
cbff2c2 to
c64775e
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
It will be processed automatically as soon as GitHub reports it as mergeable. View in MergeQueue UI.
The expected merge time in
DDCI pipeline didn't start (sourcing_failed)... Please retry. DDCI Change Request: |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
grantseltzer
added
the
backport/7.83.x
|
Backport evaluation: View Slack thread |
Backport 623b04c from #54400. ___ ### What does this PR do? Changes how the live debugger finds processes to instrument. It used to look at each process exactly three times, when the process turned 3, 100 and 1000 seconds old. If the tracer inside that process had not announced itself by the third look, that process was never instrumented again for as long as it ran. Now every process is checked repeatedly, with a growing gap between checks, until it is either instrumented or gone. A slow start is a delay of seconds instead of a permanent miss. Two supporting changes come with it. Processes that are not Go programs are identified with a cheap check and set aside, so that looking at everything repeatedly stays affordable. And scans now run on a plain five second timer, where the old one stretched itself in proportion to how long the previous scan took, with no upper limit. Includes benchmarks for the cost of a scan. ### Motivation This came out of an investigation into a service that took roughly half an hour to upload its debugging symbols. Symbols are only uploaded once a service is being instrumented, so a discovery failure surfaces as an unexplained delay somewhere else entirely, which is what made it hard to track down. The tracer announces itself at the very end of its own startup, behind a network call that can hang for ten seconds. The first look happened a few seconds after the process started, so the tracer could not win that race, and the design gave it only two more chances ever. The same investigation turned up a second problem: because the timer grew with scan duration and had no ceiling, a single slow scan could push the next one out far enough to swallow two of the three chances a process ever got. ### Describe how you validated your changes Extended the existing table-driven scanner tests to cover the retry schedule, a tracer that only announces itself after many attempts, agent restart, process ID reuse, permission failures, processes that are not Go programs, and the fixed timer. Added benchmarks, since the new loop looks at every process every time. On a host with two thousand processes, an ordinary scan costs about 0.44% of one core, and the first scan after a restart costs about 2% of a core for that one scan. ### Additional Notes A process that starts as a shell script and later replaces itself with the real service keeps the same process ID, so if we happen to look at it while it is still the script, we write it off along with the service it becomes if it's not Go. Container enrypoints happen on proc start so this is not an issue except the case of a proc running as a script that does set up and does an exec. I don't think that really matters for support. Co-authored-by: sabrina.lu <sabrina.lu@datadoghq.com>
5 participants
What does this PR do?
Changes how the live debugger finds processes to instrument. It used to look at each process exactly three times, when the process turned 3, 100 and 1000 seconds old. If the tracer inside that process had not announced itself by the third look, that process was never instrumented again for as long as it ran. Now every process is checked repeatedly, with a growing gap between checks, until it is either instrumented or gone. A slow start is a delay of seconds instead of a permanent miss.
Two supporting changes come with it. Processes that are not Go programs are identified with a cheap check and set aside, so that looking at everything repeatedly stays affordable. And scans now run on a plain five second timer, where the old one stretched itself in proportion to how long the previous scan took, with no upper limit.
Includes benchmarks for the cost of a scan.
Motivation
This came out of an investigation into a service that took roughly half an hour to upload its debugging symbols. Symbols are only uploaded once a service is being instrumented, so a discovery failure surfaces as an unexplained delay somewhere else entirely, which is what made it hard to track down.
The tracer announces itself at the very end of its own startup, behind a network call that can hang for ten seconds. The first look happened a few seconds after the process started, so the tracer could not win that race, and the design gave it only two more chances ever. The same investigation turned up a second problem: because the timer grew with scan duration and had no ceiling, a single slow scan could push the next one out far enough to swallow two of the three chances a process ever got.
Describe how you validated your changes
Extended the existing table-driven scanner tests to cover the retry schedule, a tracer that only announces itself after many attempts, agent restart, process ID reuse, permission failures, processes that are not Go programs, and the fixed timer.
Added benchmarks, since the new loop looks at every process every time. On a host with two thousand processes, an ordinary scan costs about 0.44% of one core, and the first scan after a restart costs about 2% of a core for that one scan.
Additional Notes
A process that starts as a shell script and later replaces itself with the real service keeps the same process ID, so if we happen to look at it while it is still the script, we write it off along with the service it becomes if it's not Go. Container enrypoints happen on proc start so this is not an issue except the case of a proc running as a script that does set up and does an exec. I don't think that really matters for support.