feat(docs): improve docs throughout collector#3604
Draft
Stringy wants to merge 2 commits into
Draft
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (10)
📝 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 🚥 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. 🔧 Checkov (3.3.8)collector/container/DockerfileTraceback (most recent call last): 🔧 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
+21
to
+23
| // Manager abstracts the collector container lifecycle so that the same | ||
| // test suites can run against different execution backends (e.g. Docker, | ||
| // Kubernetes) without changing test logic. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align runtime-abstraction documentation across layers.
The changed comments mix current Docker/Podman/CRI support with future Kubernetes support and describe the current collector factory as Docker-based despite accepting an abstract executor.
integration-tests/pkg/collector/collector.go#L21-L23: describe Kubernetes as future support unless a backend exists.integration-tests/pkg/collector/collector.go#L35-L37: describeNewas returning the default manager and delegating execution to the supplied executor.integration-tests/suites/base.go#L40-L48: use Docker/Podman/CRI, or explicitly label Kubernetes as future support.
📍 Affects 2 files
integration-tests/pkg/collector/collector.go#L21-L23(this comment)integration-tests/pkg/collector/collector.go#L35-L37integration-tests/suites/base.go#L40-L48
🤖 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 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.
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
Comment on lines
+3
to
+7
| // RingChan is a non-blocking ring buffer channel. When the output channel | ||
| // is full, the oldest unread value is dropped to make room for the new one. | ||
| // This prevents gRPC event handlers from blocking when tests consume events | ||
| // slower than they arrive — critical because blocking the gRPC handler would | ||
| // stall the entire collector→sensor stream. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not promise a non-blocking stream until eviction is non-blocking.
The ring implementation can block between checking a full output channel and removing its oldest value, so both comments currently overstate the availability guarantee.
integration-tests/pkg/mock_sensor/ring.go#L3-L7: make eviction and enqueue use non-blocking operations.integration-tests/pkg/mock_sensor/server.go#L62-L64: describe the channels as preventing blocking only after the ring implementation is fixed.
📍 Affects 2 files
integration-tests/pkg/mock_sensor/ring.go#L3-L7(this comment)integration-tests/pkg/mock_sensor/server.go#L62-L64
🤖 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/mock_sensor/ring.go` around lines 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.
Source: Path instructions
Comment on lines
+28
to
31
| // Ring channel size for live event streaming to test assertions. Small | ||
| // enough to avoid excessive memory use but large enough to buffer events | ||
| // between scrape intervals without dropping. | ||
| gDefaultRingSize = 32 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Do not document the 32-slot ring as lossless.
RingChan drops the oldest value when full, so “large enough ... without dropping” is not guaranteed. Describe 32 as a typical-burst buffer and explicitly note that values may be dropped when capacity is exceeded.
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/mock_sensor/server.go` around lines 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.
Source: Path instructions
Comment on lines
+74
to
+76
| // RFC 5737 documentation range — guaranteed non-routable, 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 | ⚡ Quick win
10.255.255.1 is not RFC 5737 documentation space. It's RFC 1918 private space, so connect() can fail immediately depending on the container network. Use a blackholed route/firewall rule, or document the environment-specific assumption.
🤖 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 - 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.
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.