Repository navigation
Conversation
Scan-Build Report
Bug Summary
Filter Results by FileReports
|
|||||||||||||||||||||||||||||||||||||||||||||||||
CI Test ResultsRun: #43275183750 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-11-12 18:22:11 UTC |
Reliability & Chaos Results✅ All reliability & chaos checks passed Pipeline: https://gitlab.ddbuild.io/DataDog/java-profiler/-/pipelines/152713125 |
fd5f2e5 to
99fc144
Compare
99fc144 to
24da32f
Compare
PSI cpu/memory, loadavg, cgroup throttling and a co-tenant census are read from procfs/sysfs in the sampler's tick, with NA for unavailable sources; the mem line and cadence stay unchanged.
An optional --allocator argument evaluates load, NA share, the glibc release probe and growth into pass, fail or inconclusive; today's output without the flag is unchanged, and the <5-sample crash no longer passes silently on the verdict path.
gmalloc rows get a jcmd trim_native_heap probe in a 30s tail with RSS recorded around it; the verdict maps to fail, inconclusive or silence on stderr; probe lines never enter the growth series.
A new INCONCLUSIVE_* dotenv key flows through the unchanged REASON_* paths; the scheduled notify job now always runs and is a no-op on a green pipeline.
Distinct warning rendering and headline rules, a non-alert Slack message, and run-JSON partial status with explicit inconclusive counts; every REASON_* rendering stays byte-identical.
The shell-unit-tests job runs the seven new network-free test suites and the validate step lints every changed script; the python-driven fixtures use the image's stability venv.
Describe the load markers, the release probe and its tail, the three verdicts with their provisional bounds, the thread-arena caveat and the rerun-before-regression rule.
The probe jcmd now comes from the JDK that launched the JVM with an executable check; a crashed trend check fails the job loudly instead of degrading to inconclusive; the sampler targets the launched JVM's pid directly; the co-tenant census runs in one awk pass so the markers do not measure their own load.
The census pipes stat files through cat so a vanished process cannot abort the awk pass; an unresolvable java fails the job loudly instead of probing a bogus jcmd path.
An unreadable stat entry is a directory so root permissions cannot mask it; the guard test asserts on maven-get.log instead of a vacuous stdout grep, and covers a non-executable java.
Read smaps_rollup without a useless cat and mark the pre-existing CANCELLED test for the older shellcheck.
A quiet container reads psi memory 0.00 while sustained reclaim distorts RSS; scheduler contention only shapes latency verdicts, so psi cpu is recorded without gating and psi memory takes the provisional 5.0 bound.
24da32f to
4ab89da
Compare
4ab89da to
a9585e8
Compare
|
@DataDog review |
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two distinct script-level issues were found: one where inconclusive run results are lost due to missing artifact paths in the publish step, and one where internal collector subprocesses are miscounted as external runnable co-tenants, corrupting the cotenant diagnostic marker.
🤖 Bits Code Review · Commit a9585e8
| pass | ||
|
|
||
| return failures | ||
| return entries |
There was a problem hiding this comment.
Scheduled publishing inherits INCONCLUSIVE_* dotenv variables, but producer jobs omit build.env from artifacts.paths. This parser only scans downloaded files, so an inconclusive-only run is published as status=passed with inconclusive=0 instead of partial. Consume the inherited keys, deduplicating rows also found in files, to preserve the new verdict in run history.
| return entries | |
| if prefix == 'INCONCLUSIVE_': | |
| for key, value in os.environ.items(): | |
| if not key.startswith(prefix): | |
| continue | |
| match = re.match(pattern, key) | |
| if match: | |
| config, allocator, arch, variant = match.groups() | |
| entry = { | |
| "config": config, | |
| "allocator": allocator, | |
| "arch": arch, | |
| "variant": variant, | |
| "reason": value | |
| } | |
| else: | |
| entry = {"config": "unknown", "reason": f"{key}={value}"} | |
| if entry not in entries: | |
| entries.append(entry) | |
| return entries |
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
There was a problem hiding this comment.
Fixed in e6c5cfb: build.env is now in artifacts:paths of all four reliability template blocks (main/chaos × pipeline/child), so the dotenv report is downloadable. Pinned in generate_run_json_test.sh with a per-pipeline count assertion (2 paths entries) so it cannot drift silently.
| markers_count=$(cat "$markers_root"/[0-9]*/stat 2>/dev/null | awk -v excl="$markers_exclude" ' | ||
| BEGIN { n = split(excl, e, ","); for (i = 1; i <= n; i++) ex[e[i]] = 1 } | ||
| { | ||
| seen = 1 | ||
| pid = $0 | ||
| sub(/ .*/, "", pid) | ||
| # comm may hold spaces and ), so fields restart after the last ) | ||
| sub(/.*\)/, "", $0) | ||
| split($0, f, " ") | ||
| if (f[1] == "R" && !ex[pid] && !ex[f[2]]) count++ | ||
| } | ||
| END { if (seen) print count + 0; else print "NA" } | ||
| ') |
There was a problem hiding this comment.
The normal sampler call nests command substitutions, placing its cat and awk collectors beyond the sampler’s direct children. Checking only PID and immediate parent therefore counts these collectors as runnable co-tenants. A focused reproduction reported cotenant_runnable=1–2 with only collector processes counted, falsely attributing measurement activity to external contention. This corrupts the diagnostic marker, although it does not affect the memory verdict.
| markers_count=$(cat "$markers_root"/[0-9]*/stat 2>/dev/null | awk -v excl="$markers_exclude" ' | |
| BEGIN { n = split(excl, e, ","); for (i = 1; i <= n; i++) ex[e[i]] = 1 } | |
| { | |
| seen = 1 | |
| pid = $0 | |
| sub(/ .*/, "", pid) | |
| # comm may hold spaces and ), so fields restart after the last ) | |
| sub(/.*\)/, "", $0) | |
| split($0, f, " ") | |
| if (f[1] == "R" && !ex[pid] && !ex[f[2]]) count++ | |
| } | |
| END { if (seen) print count + 0; else print "NA" } | |
| ') | |
| markers_count=$(cat "$markers_root"/[0-9]*/stat 2>/dev/null | awk -v excl="$markers_exclude" ' | |
| BEGIN { n = split(excl, e, ","); for (i = 1; i <= n; i++) ex[e[i]] = 1 } | |
| { | |
| seen = 1 | |
| pid = $0 | |
| sub(/ .*/, "", pid) | |
| # comm may hold spaces and ), so fields restart after the last ) | |
| sub(/.*\)/, "", $0) | |
| split($0, f, " ") | |
| parent[pid] = f[2] | |
| if (f[1] == "R") runnable[pid] = 1 | |
| } | |
| END { | |
| if (!seen) { print "NA"; exit } | |
| for (pid in runnable) { | |
| ancestor = pid | |
| while (ancestor && !ex[ancestor] && ancestor != parent[ancestor]) | |
| ancestor = parent[ancestor] | |
| if (!ex[ancestor]) count++ | |
| } | |
| print count + 0 | |
| } | |
| ') |
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
There was a problem hiding this comment.
Fixed in 6b4acfa and fc6183a: the census now walks the ancestor chain instead of checking direct parents only, so its own subshell collectors are no longer counted (mem_watch_markers_test.sh pins grandchildren 907/908 excluded, foreign 906 counted). The walk is bounded at 64 steps with a cycle fixture, so pid reuse cannot hang it; the fixture watchdog uses POSIX integer sleep as you suggested.
A dotenv report reaches later jobs only as variables unless the file is also in artifacts:paths, so the publish parser scanning the artifact download missed INCONCLUSIVE_ rows
The census's own cat|awk collectors are grandchildren of the sampler shell through command substitutions, so the direct-parent check counted them as co-tenants; walk the ancestor chain instead
Pid reuse can pair two stat entries into a ppid cycle; the walk now carries a step limit and counts an unresolvable entry instead of spinning
Summary
Retrofit the reliability
memorygate with the measured estimator discipline: load markers recorded with every RSS sample, a glibc release probe run from the harness (never inside the profiler), and a three-way verdict —pass,failorinconclusive. Tracker: PROF-16568. Stacked on #968.Today the gate reads passive
smaps_rollupRSS every 5 s and fails on a growth trend. On glibc, passive RSS stays flat (<0.2 %) at equal live bytes whether memory is retained or releasable, so the verdict cannot tell a leak from a releasable spike — and with no load record it cannot tell growth from a loaded host. This PR adds the two missing signals and makes the verdict honest about what it knows.What changed
Markers (
mem_watch.sh, newcollect_markers.sh): everymem:sample is followed by a same-tickmarkers:record — PSI cpu/memorysome avg10, 1-min loadavg, cgroupnr_throttled, and a co-tenant runnable census. Unavailable sources are the literalNA, never0. The census runs as onecat | awkpass (a direct awk glob aborts when a/procentry vanishes between glob and open) so the sampler does not measure its own load. Themem:line, CLI, and 5 s cadence are unchanged.Release probe (
memory_trend_check.sh): ongmallocrows the JVM runs 30 s longer and the harness callsjcmd <pid> System.trim_native_heap(the JDK that launched the JVM; executable-checked) after the measurement window closes. RSS is recorded before/after; probe lines never enter the growth series. Other allocators recordnot-applicable. The probe never touches profiler code.Verdict (
memory_trend_check.py, optional--allocator): first-match rules — too few samples, NA share > 0.5, median memory pressurepsi_memory_some_avg10> 5.0, cgroupnr_throttleddelta > 8, or a missing/failed glibc probe ⇒inconclusive; no growth ⇒pass; persistent growth with acceptable markers ⇒fail(on gmalloc, only when the probe recovers < 50 % of the growth). The <5-sample case used to crash withZeroDivisionErrorand pass silently. A crashed trend check fails the job loudly (FAIL:Trend check crashed) instead of degrading to inconclusive rows.Load gating by mechanism, not by host type:
psi_cpu_some_avg10, loadavg and the census are recorded but never gate — scheduler contention distorts latency/stall verdicts, not RSS trends (the firsttest:reliabilityrun confirmed it: shared-CI hosts sit at CPU-pressure medians 11.7–40.3, which made a CPU-pressure bound useless there). The two gating signals are the ones that actually alter the measured series:psi_memory(reclaim directly changes RSS; a quiet container reads 0.00, so 5.0 is provisional) andnr_throttled(cgroup throttling paces the JVM's own allocation/GC). All bounds are named provisional constants; changing one needs engineer sign-off.Reporting:⚠️ PR-comment block, a distinct non-alert Slack message, and run-JSON
inconclusivedoes not fail the job. It flows through a newINCONCLUSIVE_*dotenv key (allREASON_*paths byte-identical) to apartialstatus with explicit counts. The schedulednotify-slackjob now always runs and is a no-op on a green pipeline.Tests: seven network-free suites (~1,500 assertions) wired into
shell-unit-tests, plusbash -n/shellcheckin the validate step. The Python-driven suites use the image's stability venv (/app/bp-stability/venv), which carries numpy/matplotlib; the defaultpython3(3.9) does not.Behavior decisions for reviewer acknowledgment
inconclusiveinstead of failing; (b) on gmalloc, growth the probe releases (≥ 50 % recovery) ispass. Every otherFAIL:path (crash, fetch, lib, non-glibc trend) keeps its exact text.TREND:uprun whose net growth vs the first sample is ≤ 0 still fails on gmalloc (e.g. a series that dips early then rises). Conservative by design: a leak building after an early high-water mark must not pass.MEMWATCH_TARGET_PID) instead of aps|greptext match that could select a sibling process. At most the first tick of RSS is missed; the sampler still runs without the allocator preload.EXPECTED_CONFIGS=36staleness and theREASON_/INCONCLUSIVE_regex mis-split ofprofiler+traceringenerate-run-json.sh.Preserved behavior branches (CI script changes)
Enumerated via
git log -pand preserved in every changed script: the 6-attempt mvn retry andmaven-get.log; fetch/libFAIL:exits;count_cpuscgroup v1/v2/nproc paths; sdkman install;CONFIG/ALLOCATORcases including theLD_PRELOADfind and the unknown-valueexit 1;RCandFAIL:Benchmark crashed;memwatch-trend.png; both YAMLREASONparsers (cut -f2 -d':'scheduled,grep -m1 … cut -f2-PR child);after_script; theCANCELLEDexit innotify_channel.sh.Testing
bash -n+shellcheckclean (at CI's 0.8.0 and current 0.11); both YAMLs parse.test:reliabilityrun on this candidate: all 12 memory rows produced markers and verdicts on real CI hosts through the complete chain (markers → verdict →INCONCLUSIVE_*→ PR comment). The first run's rows all reported inconclusive on CPU pressure under the since-amended rule — that observation is what motivated the mechanism-based gating above; a fresh run on this head will classify rows by memory pressure instead.