feat(docs): improve docs throughout collector#3604
Open
Stringy wants to merge 1 commit into
Open
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3604 +/- ##
=======================================
Coverage 27.34% 27.34%
=======================================
Files 95 95
Lines 5420 5420
Branches 2545 2545
=======================================
Hits 1482 1482
Misses 3211 3211
Partials 727 727
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change expands documentation across build files, collector runtime components, container packaging, and integration tests. It also adds explicit ChangesCollector documentation and build configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (1)
integration-tests/pkg/collector/collector.go (1)
7-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument
Mountssemantics or narrow the claim.The comment says
Mounts,Env, andConfigfollow different code paths, but only explainsEnvandConfig. Add the host/container path semantics forMounts, or remove it from that claim.As per path instructions: focus on major issues impacting performance, readability, maintainability and security; avoid nitpicks and avoid verbosity.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/pkg/collector/collector.go` around lines 7 - 17, Update the StartupOptions documentation to explain that Mounts maps host paths to container paths, or remove Mounts from the statement about settings following different code paths. Keep the existing Env and Config semantics documented.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@collector/CMakeLists.txt`:
- Around line 117-122: Reword the comments around BUILD_SHARED_LIBS and
LIBELF_LIB_SUFFIX to state that project libraries default to static linkage
while required shared runtime dependencies remain supported, avoiding any
implication that the binary is fully static. Update the BUILD_SHARED_LIBS cache
help text to describe static/shared linkage rather than position-independent
code.
In `@collector/container/Dockerfile`:
- Around line 48-50: Update the Dockerfile comment above the COPY command to
describe self-checks only as a separate startup-validation executable, removing
the claim that it is independent from the full collector dependency tree. Do not
change the executable or copy behavior unless the link graph is intentionally
updated elsewhere.
In `@collector/lib/ConfigLoader.h`:
- Around line 180-185: Update the documentation comment above WatchFile to
describe the watchers as up to three categories rather than stating that all
three are always maintained. Clarify that LOADER_CONFIG_FILE is created only
when the file exists and LOADER_CONFIG_REALPATH only applies to symlink targets,
while retaining the existing category descriptions.
In `@collector/lib/HostHeuristics.cpp`:
- Around line 76-77: Update the comment near the Power heuristic condition to
describe the actual predicate: ppc64le systems running kernel 4.18 builds below
477. Do not imply RHEL 8.6-specific validation unless the implementation is also
changed to verify the distribution.
In `@collector/lib/NetworkSignalHandler.cpp`:
- Around line 15-18: Update the comment describing Modifier::ADD near
HandleSignal to state that events add or refresh tracked connections, while the
removal case handles closed sockets. Keep the explanation that this enum drives
HandleSignal’s decision without implying ADD only represents newly established
connections.
In `@collector/lib/NetworkStatusNotifier.h`:
- Around line 26-27: Update the class documentation comment in
NetworkStatusNotifier to state that arena allocation occurs only when
USE_PROTO_ARENAS is enabled, and that HeapProtoAllocator is used otherwise. Keep
the documentation aligned with the existing ProtoAllocator alias behavior.
- Around line 22-24: Update NetworkStatusNotifier::OnRecvControlMessage() so the
branch calling ReceivePublicIPs() checks the public-IP field’s presence, not
has_ip_networks(). Keep the ReceiveIPNetworks() branch guarded by the network
field and preserve the existing handling for messages containing either or both
fields.
In `@collector/lib/RateLimit.h`:
- Around line 9-14: Clarify the `TokenBucket::last_time` member comment to
consistently describe its actual timestamp semantics, matching how refill
calculations use it. Remove the conflicting elapsed-time wording while
preserving the existing `TokenBucket` fields and units.
In `@collector/lib/SelfCheckHandler.h`:
- Around line 19-25: The self-check documentation overstates verification and
startup failure behavior. In collector/lib/SelfCheckHandler.h lines 19-25,
describe the guarantee as matching events reaching the collector rather than
proving the full event pipeline; in collector/self-checks.cpp lines 12-20,
remove or qualify the statement that startup aborts so it reflects the timeout
path logging a warning and returning FINISHED.
In `@collector/self-checks.cpp`:
- Around line 57-60: Update the select() call in the listener self-check to pass
listener + 1 instead of 1, ensuring the listener descriptor is monitored.
Preserve the existing FD_ISSET(listener, &sockets) validation before calling
accept().
In `@integration-tests/pkg/collector/collector.go`:
- Around line 21-23: Update documentation at
integration-tests/pkg/collector/collector.go:21-23 to describe Kubernetes
support as future unless an implementation exists; revise the New documentation
at integration-tests/pkg/collector/collector.go:35-37 to state that it returns
the default manager and delegates execution to the supplied executor; align
integration-tests/suites/base.go:40-48 with current Docker/Podman/CRI support,
explicitly marking Kubernetes as future if mentioned.
In `@integration-tests/pkg/executor/executor.go`:
- Around line 75-77: Update New to explicitly accept only the supported runtime
commands "docker", "podman", and the intended CRI values; return an error for
any unknown or misspelled command instead of routing it to newCriExecutor.
Preserve the existing executor selection for valid runtimes.
In `@integration-tests/pkg/mock_sensor/ring.go`:
- Around line 3-7: The ring buffer implementation around RingChan must make both
oldest-value eviction and new-value enqueue non-blocking, including the gap
between detecting a full output channel and removing an item; update its
send/receive operations accordingly. In
integration-tests/pkg/mock_sensor/server.go lines 62-64, revise the channel
comments to claim blocking prevention only under the ring’s corrected
non-blocking behavior.
In `@integration-tests/pkg/mock_sensor/server.go`:
- Around line 28-31: Update the comment above gDefaultRingSize to describe 32
slots as buffering typical event bursts between scrape intervals, and explicitly
state that older values may be dropped when the ring reaches capacity; do not
claim the buffer prevents drops.
In `@integration-tests/suites/async_connections.go`:
- Around line 74-76: Correct the comment and test setup around the unreachable
address in the async connection test: do not describe 10.255.255.1 as RFC 5737
documentation space. Replace it with a reliably blackholed route/firewall-based
simulation, or explicitly document the environment-specific assumption if
retaining the address.
---
Nitpick comments:
In `@integration-tests/pkg/collector/collector.go`:
- Around line 7-17: Update the StartupOptions documentation to explain that
Mounts maps host paths to container paths, or remove Mounts from the statement
about settings following different code paths. Keep the existing Env and Config
semantics documented.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: dcddd241-e848-4423-a697-74504ba61116
📒 Files selected for processing (48)
CMakeLists.txtMakefilecollector/CMakeLists.txtcollector/collector.cppcollector/container/Dockerfilecollector/container/status-check.shcollector/lib/CMakeLists.txtcollector/lib/CollectorService.cppcollector/lib/CollectorService.hcollector/lib/ConfigLoader.hcollector/lib/ConnTracker.cppcollector/lib/ConnTracker.hcollector/lib/DuplexGRPC.hcollector/lib/ExternalIPsConfig.hcollector/lib/HostHeuristics.cppcollector/lib/HostHeuristics.hcollector/lib/HostInfo.hcollector/lib/Inotify.hcollector/lib/NRadix.hcollector/lib/NetworkConnection.hcollector/lib/NetworkSignalHandler.cppcollector/lib/NetworkSignalHandler.hcollector/lib/NetworkStatusNotifier.cppcollector/lib/NetworkStatusNotifier.hcollector/lib/ProcessSignalHandler.cppcollector/lib/ProcfsScraper.hcollector/lib/ProtoAllocator.hcollector/lib/RateLimit.hcollector/lib/SelfCheckHandler.cppcollector/lib/SelfCheckHandler.hcollector/lib/SignalHandler.hcollector/lib/StoppableThread.hcollector/lib/system-inspector/Service.cppcollector/lib/system-inspector/Service.hcollector/self-checks.cppintegration-tests/pkg/collector/collector.gointegration-tests/pkg/executor/executor.gointegration-tests/pkg/mock_sensor/ring.gointegration-tests/pkg/mock_sensor/server.gointegration-tests/suites/async_connections.gointegration-tests/suites/base.gointegration-tests/suites/connections_and_endpoints.gointegration-tests/suites/listening_ports.gointegration-tests/suites/processes_and_endpoints.gointegration-tests/suites/procfs_scraper.gointegration-tests/suites/repeated_network_flow.gointegration-tests/suites/runtime_config_file.gointegration-tests/suites/threads.go
Comment on lines
117
to
122
| # Collector ships as a statically-linked binary in a minimal UBI-micro | ||
| # container — dynamic linking would require shipping .so files. | ||
| set(BUILD_SHARED_LIBS OFF CACHE BOOL "Build position independent libraries and executables" FORCE) | ||
| # Exception to static linking: libbpf dlopen()s libelf at runtime to parse | ||
| # BPF ELF objects, so libelf must remain a shared library. | ||
| set(LIBELF_LIB_SUFFIX ".so" CACHE STRING "Use libelf.so" FORCE) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
rg -n 'BUILD_SHARED_LIBS|LIBELF_LIB_SUFFIX|target_link_libraries|dnf install|libstdc\+\+|openssl|tbb' \
collector/CMakeLists.txt collector/lib/CMakeLists.txt collector/container/DockerfileRepository: stackrox/collector
Length of output: 1860
🏁 Script executed:
#!/bin/sh
set -eu
sed -n '108,126p' collector/CMakeLists.txt
printf '\n---\n'
sed -n '1,40p' collector/container/DockerfileRepository: stackrox/collector
Length of output: 3098
Reword this to avoid implying a fully static binary. BUILD_SHARED_LIBS only changes the default for project libraries; the image still ships shared runtime deps like libstdc++, openssl, tbb, and libelf remains shared. The cache help text should also describe static/shared linkage, not position-independent code.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@collector/CMakeLists.txt` around lines 117 - 122, Reword the comments around
BUILD_SHARED_LIBS and LIBELF_LIB_SUFFIX to state that project libraries default
to static linkage while required shared runtime dependencies remain supported,
avoiding any implication that the binary is fully static. Update the
BUILD_SHARED_LIBS cache help text to describe static/shared linkage rather than
position-independent code.
Source: Path instructions
Comment on lines
+75
to
+77
| // New creates the appropriate executor based on the configured container | ||
| // runtime. Docker and Podman use the Docker-compatible API client, while | ||
| // CRI (containerd, crio) uses the CRI gRPC API directly. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
rg -n -C 3 'RuntimeInfo\(\)|Command.*(docker|podman|containerd|crio)|newCriExecutor' .Repository: stackrox/collector
Length of output: 3986
🏁 Script executed:
#!/bin/sh
set -e
printf '\n## integration-tests/pkg/config/config.go\n'
sed -n '1,220p' integration-tests/pkg/config/config.go
printf '\n## integration-tests/pkg/executor/executor.go\n'
sed -n '1,140p' integration-tests/pkg/executor/executor.go
printf '\n## all references to runtime command values\n'
rg -n 'runtimeDefaultCommand|envRuntimeCommand|Command:|RuntimeInfo\(\)\.Command|== "docker"|== "podman"|containerd|crio' integration-tests/pkgRepository: stackrox/collector
Length of output: 8346
Reject unsupported runtime commands. New() sends any value other than "docker" or "podman" to newCriExecutor(), so typos or unexpected RUNTIME_COMMAND values silently fall into the CRI path. Return an error for unknown values, or make the fallback explicit in the docs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@integration-tests/pkg/executor/executor.go` around lines 75 - 77, Update New
to explicitly accept only the supported runtime commands "docker", "podman", and
the intended CRI values; return an error for any unknown or misspelled command
instead of routing it to newCriExecutor. Preserve the existing executor
selection for valid runtimes.
Source: Path instructions
Stringy
marked this pull request as ready for review
July 23, 2026 09:26
Stringy
force-pushed
the
giles/docs-for-agents
branch
from
July 27, 2026 11:34
bdea962 to
dda0b08
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@integration-tests/suites/async_connections.go`:
- Around line 74-77: Replace the hard-coded RFC1918 destination used by the
stalled-connect test with a deterministic blackhole setup, such as an isolated
route or firewall rule that guarantees SYN packets remain unanswered. If
retaining the address-based approach, add an explicit precondition check that
confirms the destination is unroutable before running the timeout assertions;
update the affected async connection test setup rather than only its comments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d6444d5e-946b-4063-a6f4-aadc69beef4d
📒 Files selected for processing (48)
CMakeLists.txtMakefilecollector/CMakeLists.txtcollector/collector.cppcollector/container/Dockerfilecollector/container/status-check.shcollector/lib/CMakeLists.txtcollector/lib/CollectorService.cppcollector/lib/CollectorService.hcollector/lib/ConfigLoader.hcollector/lib/ConnTracker.cppcollector/lib/ConnTracker.hcollector/lib/DuplexGRPC.hcollector/lib/ExternalIPsConfig.hcollector/lib/HostHeuristics.cppcollector/lib/HostHeuristics.hcollector/lib/HostInfo.hcollector/lib/Inotify.hcollector/lib/NRadix.hcollector/lib/NetworkConnection.hcollector/lib/NetworkSignalHandler.cppcollector/lib/NetworkSignalHandler.hcollector/lib/NetworkStatusNotifier.cppcollector/lib/NetworkStatusNotifier.hcollector/lib/ProcessSignalHandler.cppcollector/lib/ProcfsScraper.hcollector/lib/ProtoAllocator.hcollector/lib/RateLimit.hcollector/lib/SelfCheckHandler.cppcollector/lib/SelfCheckHandler.hcollector/lib/SignalHandler.hcollector/lib/StoppableThread.hcollector/lib/system-inspector/Service.cppcollector/lib/system-inspector/Service.hcollector/self-checks.cppintegration-tests/pkg/collector/collector.gointegration-tests/pkg/executor/executor.gointegration-tests/pkg/mock_sensor/ring.gointegration-tests/pkg/mock_sensor/server.gointegration-tests/suites/async_connections.gointegration-tests/suites/base.gointegration-tests/suites/connections_and_endpoints.gointegration-tests/suites/listening_ports.gointegration-tests/suites/processes_and_endpoints.gointegration-tests/suites/procfs_scraper.gointegration-tests/suites/repeated_network_flow.gointegration-tests/suites/runtime_config_file.gointegration-tests/suites/threads.go
🚧 Files skipped from review as they are similar to previous changes (45)
- collector/lib/SignalHandler.h
- integration-tests/pkg/mock_sensor/ring.go
- collector/lib/StoppableThread.h
- integration-tests/suites/connections_and_endpoints.go
- collector/container/status-check.sh
- collector/lib/HostHeuristics.h
- CMakeLists.txt
- integration-tests/suites/procfs_scraper.go
- collector/lib/CollectorService.h
- integration-tests/suites/processes_and_endpoints.go
- collector/lib/NetworkStatusNotifier.cpp
- collector/lib/ConfigLoader.h
- collector/lib/DuplexGRPC.h
- collector/lib/Inotify.h
- collector/lib/NetworkSignalHandler.h
- collector/lib/ConnTracker.h
- collector/lib/NetworkConnection.h
- collector/lib/ExternalIPsConfig.h
- collector/lib/SelfCheckHandler.h
- Makefile
- integration-tests/pkg/executor/executor.go
- collector/lib/HostInfo.h
- collector/lib/NetworkStatusNotifier.h
- integration-tests/suites/listening_ports.go
- collector/self-checks.cpp
- collector/lib/NetworkSignalHandler.cpp
- collector/lib/system-inspector/Service.cpp
- collector/lib/NRadix.h
- integration-tests/pkg/mock_sensor/server.go
- collector/lib/CollectorService.cpp
- integration-tests/suites/threads.go
- collector/lib/ProtoAllocator.h
- integration-tests/pkg/collector/collector.go
- collector/lib/CMakeLists.txt
- integration-tests/suites/repeated_network_flow.go
- integration-tests/suites/base.go
- collector/lib/RateLimit.h
- integration-tests/suites/runtime_config_file.go
- collector/lib/ConnTracker.cpp
- collector/lib/ProcfsScraper.h
- collector/CMakeLists.txt
- collector/lib/SelfCheckHandler.cpp
- collector/lib/system-inspector/Service.h
- collector/collector.cpp
- collector/lib/HostHeuristics.cpp
Comment on lines
+74
to
+77
| // RFC 1918 private address with no listener — expected to be | ||
| // non-routable in the test environment, so the kernel's connect() | ||
| // will hang in SYN_SENT until it times out, simulating a | ||
| // firewall-blocked connection. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
Use a deterministic blackhole for the stalled-connect test.
This wording fixes the RFC5737 mistake, but 10.255.255.1 is not guaranteed to be unroutable from every container/CI network. If a route exists, the connection may receive an immediate RST instead of remaining in SYN_SENT, so the test can silently validate the wrong scenario. Use a controlled blackhole route/firewall rule, or verify this network assumption before relying on it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@integration-tests/suites/async_connections.go` around lines 74 - 77, Replace
the hard-coded RFC1918 destination used by the stalled-connect test with a
deterministic blackhole setup, such as an isolated route or firewall rule that
guarantees SYN packets remain unanswered. If retaining the address-based
approach, add an explicit precondition check that confirms the destination is
unroutable before running the timeout assertions; update the affected async
connection test setup rather than only its comments.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
A detailed explanation of the changes in your PR.
Feel free to remove this section if it is overkill for your PR, and the title of your PR is sufficiently descriptive.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
TODO(replace-me)
Use this space to explain how you tested your PR, or, if you didn't test it, why you did not do so. (Valid reasons include "CI is sufficient" or "No testable changes")
In addition to reviewing your code, reviewers must also review your testing instructions, and make sure they are sufficient.
For more details, ref the Confluence page about this section.