feat(output): add basic opentelemetry output#971
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds an optional OpenTelemetry (OTLP) logging output to fact, gated behind an ChangesOpenTelemetry Output Feature
CI/CD and Build Infrastructure
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant EventLoop as output::start
participant OtelClient as otel::Client
participant Exporter as OTLP LogExporter
participant Collector as OTLP/Loki Endpoint
EventLoop->>OtelClient: subscribe via oneshot handshake
OtelClient->>OtelClient: receive Event from broadcast
OtelClient->>OtelClient: convert Event to AnyValue LogRecord
OtelClient->>Exporter: emit log record
Exporter->>Collector: POST /v1/logs (protobuf)
sequenceDiagram
participant CI as ci.yml
participant Build as container-build.yml
participant Registry as Quay Registries
CI->>Build: workflow_call (tag-suffix, make-target)
Build->>Build: vars job computes image tags
Build->>Registry: push stackrox-io image (amd64/arm64)
Build->>Registry: push rhacs-eng retagged image
Build->>Registry: create/push multi-arch manifests
Build-->>CI: output tag
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## mauro/feat/resubscription-change #971 +/- ##
====================================================================
+ Coverage 32.64% 34.13% +1.48%
====================================================================
Files 21 21
Lines 2916 2868 -48
Branches 2916 2868 -48
====================================================================
+ Hits 952 979 +27
+ Misses 1961 1886 -75
Partials 3 3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Molter73
force-pushed
the
mauro/feat/otel-output
branch
5 times, most recently
from
229cd7d to
8bb3067
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/event.py (1)
298-317: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
.match()doesn't anchor at the end of the string.
expected.match(actual)only checks thatactualstarts with a match forexpected; a longeractualvalue with unexpected trailing characters would still be considered a match. Usefullmatch()for exact validation of regex-based expectations.🐛 Proposed fix
- if not isinstance(actual, str) or not expected.match(actual): + if not isinstance(actual, str) or not expected.fullmatch(actual):🤖 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 `@tests/event.py` around lines 298 - 317, The regex check in _diff_path currently uses expected.match(actual), which allows extra trailing characters to slip through. Update the Pattern branch in _diff_path to use fullmatch() instead so regex-based expectations must match the entire actual path, while keeping the existing non-regex equality behavior unchanged.
🧹 Nitpick comments (5)
.github/workflows/container-build.yml (2)
25-28: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider
persist-credentials: falseon checkout steps.zizmor's artipacked finding notes these
actions/checkout@v4steps don't setpersist-credentials: false, leaving the ambientGITHUB_TOKENin.git/configfor the remainder of the job — including duringcargo build/podman build, which execute third-party build scripts.🔒 Suggested fix
- uses: actions/checkout@v4 with: submodules: true fetch-depth: 0 + persist-credentials: falseAlso applies to: 54-56
🤖 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 @.github/workflows/container-build.yml around lines 25 - 28, The checkout steps using actions/checkout@v4 currently leave the ambient GITHUB_TOKEN persisted in .git/config, which can leak into later cargo build and podman build steps. Update each checkout block in the workflow to set persist-credentials to false, including the other checkout occurrence referenced by the review, while keeping the existing submodules and fetch-depth settings in place.Source: Linters/SAST tools
33-35: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winHoist interpolated expressions into
env:to avoid template injection.zizmor flags direct
${{ inputs.tag-suffix }},${{ inputs.make-target }}, and${{ needs.vars.outputs.* }}interpolation insiderun:blocks (lines 33-35, 59, 98-102, 120-124) as template-injection risk. Even though these values currently originate from static strings inci.yml, the canonical mitigation is to pass them through a step-levelenv:block and reference them as shell variables, so the pattern remains safe if these inputs are ever driven by less-trusted values.🔒 Example fix for the `vars` step
- id: vars + env: + TAG_SUFFIX: ${{ inputs.tag-suffix }} run: | cat << EOF >> "$GITHUB_OUTPUT" - tag=$(make tag)${{ inputs.tag-suffix }} - stackrox-io-image=$(make image-name)${{ inputs.tag-suffix }} - rhacs-eng-image=$(FACT_REGISTRY="quay.io/rhacs-eng/fact" make image-name)${{ inputs.tag-suffix }} + tag=$(make tag)${TAG_SUFFIX} + stackrox-io-image=$(make image-name)${TAG_SUFFIX} + rhacs-eng-image=$(FACT_REGISTRY="quay.io/rhacs-eng/fact" make image-name)${TAG_SUFFIX} EOFSimilarly for line 59 (
env: MAKE_TARGET: ${{ inputs.make-target }}thenrun: DOCKER=podman make "${MAKE_TARGET}") and the manifest steps (98-102, 120-124), which interpolateneeds.vars.outputs.*directly.Also applies to: 59-59, 98-102, 120-124
🤖 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 @.github/workflows/container-build.yml around lines 33 - 35, The workflow steps that build image tags and run make targets are interpolating GitHub expressions directly inside run blocks, which is the template-injection risk flagged by zizmor. Move the affected values from inputs.tag-suffix, inputs.make-target, and needs.vars.outputs.* into step-level env entries, then reference those shell variables inside the run commands. Apply this pattern in the tag/image-name step, the MAKE_TARGET step, and the manifest-related steps so the logic stays the same but the expressions are no longer expanded directly in shell scripts.Source: Linters/SAST tools
tests/conftest.py (1)
70-100: 🩺 Stability & Availability | 🔵 TrivialFixed OTLP port may conflict under parallel test execution.
OtlpServer.serve()defaults to a hardcoded port (4318), mirroring the existing fixed-port pattern forGrpcServer(9999). If these tests are ever run withpytest-xdistor otherwise in parallel across workers, both fixed ports could collide across concurrently-running test processes. Not a regression introduced by this diff, but doubling the fixed-port surface (grpc + otlp) is worth tracking if parallelization is a goal.🤖 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 `@tests/conftest.py` around lines 70 - 100, The server fixture currently hardcodes OTLP to a fixed port via OtlpServer.serve(), which can collide when tests run in parallel. Update the server setup in pytest_generate_tests/server so OtlpServer can be started with a worker-unique or free port (similar to how GrpcServer is selected by mode), and make the fixture pass that port through instead of relying on the default 4318. Ensure the change is localized around _get_output_modes, pytest_generate_tests, and server so each test process gets an isolated OTLP endpoint.tests/server.py (1)
41-58: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
ThreadPoolExecutoris never shut down.Neither
GrpcServer.stop()norOtlpServer.stop()callsself.executor.shutdown(). Each test using theserverfixture creates a freshEventServersubclass instance with its own 2-worker pool that's never explicitly released, leaking idle threads across the test session (they'll only terminate at interpreter exit).♻️ Proposed fix
def stop(self): """Stop the gRPC server.""" self.server.stop(1) self.running.clear() + self.executor.shutdown(wait=False)and similarly for
OtlpServer.stop().🤖 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 `@tests/server.py` around lines 41 - 58, `EventServer` creates a `ThreadPoolExecutor` in its constructor but the pool is never released, so update the shutdown path in both `GrpcServer.stop()` and `OtlpServer.stop()` to call `self.executor.shutdown()` after stopping the server work. Make the cleanup part of the existing `stop` methods so each `EventServer` subclass instance properly tears down its worker threads when the test fixture finishes.fact/src/event/process.rs (1)
45-57: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winInconsistent attribute key naming:
exec_pathvsexe_path.
Lineage'sexe_pathfield is emitted under the key"exec_path", while the siblingProcessconversion (line 259-261 in this same file) emits the analogous field under"exe_path". Since these are exported as OTLP log attributes consumed by external tooling (Grafana/Loki dashboards per this PR's docs), inconsistent naming for the same semantic field across nested maps will confuse queries/dashboards built against the OTel schema.🔧 Suggested fix
impl From<Lineage> for opentelemetry::logs::AnyValue { fn from(value: Lineage) -> Self { AnyValue::Map(Box::new(HashMap::from([ ("uid".into(), value.uid.into()), ( - "exec_path".into(), + "exe_path".into(), value.exe_path.to_string_lossy().to_string().into(), ), ]))) } }This isn't exercised by the current Python OTLP test server (which doesn't parse
lineage), so it wouldn't be caught by CI as-is.🤖 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 `@fact/src/event/process.rs` around lines 45 - 57, The `Lineage` OTEL conversion is exporting the `exe_path` field under a different attribute name than the sibling `Process` conversion, causing inconsistent log schema. Update the `From<Lineage> for opentelemetry::logs::AnyValue` implementation in `process.rs` so the map key matches the existing `Process` export naming (`exe_path`) and keep the attribute naming consistent across both conversions for downstream queries and dashboards.
🤖 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 `@fact/src/output/otel.rs`:
- Around line 65-70: The LogExporter setup in otel initialization is panicking
on build failure via expect, which should be converted to normal error handling.
Update the LogExporter::builder() chain in the otel initialization path to
propagate the ExporterBuildError with ? or a context-wrapped error instead of
expect, so failures flow through the existing Result-based path rather than
crashing.
In `@fact/src/output/stdout.rs`:
- Around line 27-29: The subscription handshake in the stdout output path can
panic if the orchestrator is shutting down and the oneshot/channel is already
closed. In the task that uses std::sync::oneshot::channel and
self.subscriber.send(tx), replace both unwrap() calls with graceful error
handling: if sending the subscription request fails or rx.await returns an
error, log the failure and return early instead of panicking. Use the existing
stdout subscription flow around the send/receive handshake to locate the fix.
In `@tests/utils.py`:
- Around line 61-68: The string-quoting logic in the helper that builds
shell-safe literals has an unsafe fallback in the single-quote path for values
containing backslashes or quotes plus backticks or dollar signs. Update the
quoting branch in the relevant utility function to route those cases through the
double-quoted form instead, and make that branch escape backslash, double quote,
backtick, and dollar-sign characters so the value is always treated literally.
Keep the single-quoted fallback only for strings that do not need those extra
escapes.
---
Outside diff comments:
In `@tests/event.py`:
- Around line 298-317: The regex check in _diff_path currently uses
expected.match(actual), which allows extra trailing characters to slip through.
Update the Pattern branch in _diff_path to use fullmatch() instead so
regex-based expectations must match the entire actual path, while keeping the
existing non-regex equality behavior unchanged.
---
Nitpick comments:
In @.github/workflows/container-build.yml:
- Around line 25-28: The checkout steps using actions/checkout@v4 currently
leave the ambient GITHUB_TOKEN persisted in .git/config, which can leak into
later cargo build and podman build steps. Update each checkout block in the
workflow to set persist-credentials to false, including the other checkout
occurrence referenced by the review, while keeping the existing submodules and
fetch-depth settings in place.
- Around line 33-35: The workflow steps that build image tags and run make
targets are interpolating GitHub expressions directly inside run blocks, which
is the template-injection risk flagged by zizmor. Move the affected values from
inputs.tag-suffix, inputs.make-target, and needs.vars.outputs.* into step-level
env entries, then reference those shell variables inside the run commands. Apply
this pattern in the tag/image-name step, the MAKE_TARGET step, and the
manifest-related steps so the logic stays the same but the expressions are no
longer expanded directly in shell scripts.
In `@fact/src/event/process.rs`:
- Around line 45-57: The `Lineage` OTEL conversion is exporting the `exe_path`
field under a different attribute name than the sibling `Process` conversion,
causing inconsistent log schema. Update the `From<Lineage> for
opentelemetry::logs::AnyValue` implementation in `process.rs` so the map key
matches the existing `Process` export naming (`exe_path`) and keep the attribute
naming consistent across both conversions for downstream queries and dashboards.
In `@tests/conftest.py`:
- Around line 70-100: The server fixture currently hardcodes OTLP to a fixed
port via OtlpServer.serve(), which can collide when tests run in parallel.
Update the server setup in pytest_generate_tests/server so OtlpServer can be
started with a worker-unique or free port (similar to how GrpcServer is selected
by mode), and make the fixture pass that port through instead of relying on the
default 4318. Ensure the change is localized around _get_output_modes,
pytest_generate_tests, and server so each test process gets an isolated OTLP
endpoint.
In `@tests/server.py`:
- Around line 41-58: `EventServer` creates a `ThreadPoolExecutor` in its
constructor but the pool is never released, so update the shutdown path in both
`GrpcServer.stop()` and `OtlpServer.stop()` to call `self.executor.shutdown()`
after stopping the server work. Make the cleanup part of the existing `stop`
methods so each `EventServer` subclass instance properly tears down its worker
threads when the test fixture finishes.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 6070caf3-6f12-4fe2-99cd-d0c9f8b532da
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (46)
.github/workflows/ci.yml.github/workflows/container-build.yml.github/workflows/integration-tests.yml.github/workflows/konflux-tests.ymlContainerfileMakefileansible/run-tests.ymldocs/otel/grafana-dashboard-provider.yamldocs/otel/grafana-datasource.yamldocs/otel/grafana-fact-dashboard.jsondocs/otel/loki-config.yamldocs/otel/otel.mdfact/Cargo.tomlfact/src/config/mod.rsfact/src/config/reloader.rsfact/src/config/tests.rsfact/src/event/mod.rsfact/src/event/process.rsfact/src/lib.rsfact/src/metrics/mod.rsfact/src/output/grpc.rsfact/src/output/mod.rsfact/src/output/otel.rsfact/src/output/stdout.rstests/Makefiletests/conftest.pytests/event.pytests/requirements.txttests/server.pytests/test_config_hotreload.pytests/test_editors/test_nvim.pytests/test_editors/test_sed.pytests/test_editors/test_vi.pytests/test_editors/test_vim.pytests/test_file_open.pytests/test_misc.pytests/test_path_chmod.pytests/test_path_chown.pytests/test_path_mkdir.pytests/test_path_rename.pytests/test_path_rmdir.pytests/test_path_unlink.pytests/test_rate_limit.pytests/test_wildcard.pytests/test_xattr.pytests/utils.py
| let exporter_otlp = LogExporter::builder() | ||
| .with_http() | ||
| .with_protocol(opentelemetry_otlp::Protocol::HttpBinary) | ||
| .with_endpoint(endpoint) | ||
| .build() | ||
| .expect("Failed to create log exporter"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
opentelemetry-otlp 0.32 LogExporter builder build() return type error
💡 Result:
In opentelemetry-otlp version 0.32.0 (Rust), the build() method on LogExporterBuilder returns a Result<LogExporter, ExporterBuildError> [1][2]. If you are encountering a type error, it is likely related to the breaking changes introduced in the 0.32.0 release [2][3]. Specifically, the build() method's return type was updated to return Result<LogExporter, ExporterBuildError>, replacing the previous implementation that returned signal-specific errors (such as LogError) from the opentelemetry_sdk [2]. To resolve this: 1. Update any code matching on the error type to use ExporterBuildError instead of previous error enums, which have been removed [2]. 2. If your code uses unwrap() or expect(), it should continue to work without modification, provided your logic is compatible with the new Result signature [2]. 3. Ensure you are aware that build() now auto-selects the transport (gRPC or HTTP) based on the OTEL_EXPORTER_OTLP_PROTOCOL environment variable or enabled feature flags [2][3][4]. If you need specific transport configurations, continue using .with_tonic() or .with_http() before calling build() [1][4].
Citations:
- 1: https://docs.rs/opentelemetry-otlp/latest/opentelemetry_otlp/struct.LogExporterBuilder.html
- 2: https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry-otlp/CHANGELOG.md
- 3: https://github.com/open-telemetry/opentelemetry-rust/blob/main/docs/release_0.32.md
- 4: https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry-otlp/src/logs.rs
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file list ==\n'
git ls-files fact/src/output/otel.rs Cargo.toml Cargo.lock '**/Cargo.toml' | sed 's#^`#-` #'
printf '\n== fact/src/output/otel.rs (relevant region) ==\n'
cat -n fact/src/output/otel.rs | sed -n '1,220p'
printf '\n== dependency versions mentioning opentelemetry-otlp/opentelemetry-sdk ==\n'
rg -n 'opentelemetry-(otlp|sdk)|opentelemetry' Cargo.toml Cargo.lock fact/Cargo.toml -SRepository: stackrox/fact
Length of output: 6548
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
# Inspect Cargo.lock entries for opentelemetry-otlp and related crates.
text = Path('Cargo.lock').read_text() if Path('Cargo.lock').exists() else ''
for crate in ['opentelemetry-otlp', 'opentelemetry-sdk', 'opentelemetry']:
print(f'== {crate} ==')
for m in re.finditer(rf'name = "{re.escape(crate)}"\nversion = "([^"]+)"(?:\n.*?)+?(?=\n\s*\[\[package\]\]|\Z)', text, re.S):
block = m.group(0)
print(block.splitlines()[0])
for line in block.splitlines()[1:8]:
print(line)
print('...')
PYRepository: stackrox/fact
Length of output: 732
🌐 Web query:
opentelemetry-otlp 0.32 ExporterBuildError std::error::Error anyhow context
💡 Result:
In opentelemetry-otlp version 0.32.0, the ExporterBuilder::build() method was updated to return a specific ExporterBuildError enum instead of generic or signal-specific errors [1][2]. This change was introduced to improve error handling clarity and consistency [2][3]. The ExporterBuildError enum is designed to be non_exhaustive and includes the following variants [4][5]: - ThreadSpawnFailed: Occurs when spawning a new thread fails. - NoHttpClient: Occurs when no HTTP client is specified. - UnsupportedCompressionAlgorithm(String): Indicates the use of a compression algorithm that is not supported. - InvalidUri(String, String): Indicates an invalid URI (available when using grpc-tonic, http-proto, or http-json features). - InvalidConfig { name: String, reason: String }: Used for invalid configurations. - InternalFailure(String): Used for implementation-specific internal errors, intended for logging rather than programmatic decision-making [4][5]. Because ExporterBuildError implements the standard std::error::Error trait, it is compatible with error handling crates like anyhow [4][5]. You can typically use the ? operator to propagate these errors in functions that return anyhow::Result<T> [4]. If you were previously calling unwrap() or expect() on the result of a builder, this will still work, but you are encouraged to handle the ExporterBuildError variants specifically to provide better diagnostics [1][2].
Citations:
- 1: https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry-otlp/CHANGELOG.md
- 2: feat: OTLP Exporter builders to return specific Error type open-telemetry/opentelemetry-rust#2790
- 3: OTLP Exporter builder to return more specific Error type open-telemetry/opentelemetry-rust#2654
- 4: https://docs.rs/opentelemetry-otlp/latest/opentelemetry_otlp/enum.ExporterBuildError.html
- 5: https://github.com/open-telemetry/opentelemetry-rust/blob/69ab1c4e/opentelemetry-otlp/src/exporter/mod.rs
Avoid panicking on exporter build failure
LogExporter::builder().build() returns an ExporterBuildError, so this can be propagated with ?/.context()? instead of expect(). That keeps the worker on the normal error path instead of crashing on recoverable init failures.
🤖 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 `@fact/src/output/otel.rs` around lines 65 - 70, The LogExporter setup in otel
initialization is panicking on build failure via expect, which should be
converted to normal error handling. Update the LogExporter::builder() chain in
the otel initialization path to propagate the ExporterBuildError with ? or a
context-wrapped error instead of expect, so failures flow through the existing
Result-based path rather than crashing.
| let (tx, rx) = oneshot::channel(); | ||
| self.subscriber.send(tx).await.unwrap(); | ||
| let mut rx = rx.await.unwrap(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle subscription handshake failure gracefully instead of unwrap().
If the output orchestrator's loop has already exited (e.g. during shutdown), subs_rx/the oneshot sender may be dropped, making these .unwrap()s panic in the spawned task. Prefer logging and returning, matching the graceful termination used elsewhere.
🛡️ Proposed fix
- let (tx, rx) = oneshot::channel();
- self.subscriber.send(tx).await.unwrap();
- let mut rx = rx.await.unwrap();
+ let (tx, rx) = oneshot::channel();
+ if self.subscriber.send(tx).await.is_err() {
+ info!("Output orchestrator gone, stopping stdout output...");
+ return;
+ }
+ let mut rx = match rx.await {
+ Ok(rx) => rx,
+ Err(_) => {
+ info!("Failed to obtain event receiver, stopping stdout output...");
+ return;
+ }
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let (tx, rx) = oneshot::channel(); | |
| self.subscriber.send(tx).await.unwrap(); | |
| let mut rx = rx.await.unwrap(); | |
| let (tx, rx) = oneshot::channel(); | |
| if self.subscriber.send(tx).await.is_err() { | |
| info!("Output orchestrator gone, stopping stdout output..."); | |
| return; | |
| } | |
| let mut rx = match rx.await { | |
| Ok(rx) => rx, | |
| Err(_) => { | |
| info!("Failed to obtain event receiver, stopping stdout output..."); | |
| return; | |
| } | |
| }; |
🤖 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 `@fact/src/output/stdout.rs` around lines 27 - 29, The subscription handshake
in the stdout output path can panic if the orchestrator is shutting down and the
oneshot/channel is already closed. In the task that uses
std::sync::oneshot::channel and self.subscriber.send(tx), replace both unwrap()
calls with graceful error handling: if sending the subscription request fails or
rx.await returns an error, log the failure and return early instead of
panicking. Use the existing stdout subscription flow around the send/receive
handshake to locate the fix.
| if re.search(r'[^a-zA-Z0-9_.:\-/+@\]]', s): | ||
| # Backslash and single-quote cannot appear in single-quoted | ||
| # strings in Rust's shlex, use double-quoting instead. | ||
| if ('\\' in s or "'" in s) and '`' not in s and '$' not in s: | ||
| escaped = s.replace('\\', '\\\\').replace('"', '\\"') | ||
| return f'"{escaped}"' | ||
| escaped = s.replace("'", "\\'") | ||
| return f"'{escaped}'" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Broken single-quote fallback for strings with both a quote and backtick/$.
When a string contains a backslash or ' and also contains ` or $, the code falls through to the single-quote branch (s.replace("'", "\\'")). This contradicts the function's own comment: backslash has no escaping power inside single-quoted shell strings, so \' does not produce a literal quote — it just embeds a literal backslash and then the string is prematurely terminated by the unescaped '. The rest of the string (containing the backtick/$) ends up unquoted, exposing it to command substitution/expansion instead of being treated literally.
The double-quote branch already handles \ and "; extending it to also escape ` and $ removes the need for this unsafe fallback entirely.
🐛 Proposed fix
if re.search(r'[^a-zA-Z0-9_.:\-/+@\]]', s):
- # Backslash and single-quote cannot appear in single-quoted
- # strings in Rust's shlex, use double-quoting instead.
- if ('\\' in s or "'" in s) and '`' not in s and '$' not in s:
- escaped = s.replace('\\', '\\\\').replace('"', '\\"')
+ # Backslash and single-quote cannot appear in single-quoted
+ # strings in Rust's shlex, use double-quoting instead.
+ if '\\' in s or "'" in s:
+ escaped = (
+ s.replace('\\', '\\\\')
+ .replace('"', '\\"')
+ .replace('`', '\\`')
+ .replace('$', '\\$')
+ )
return f'"{escaped}"'
escaped = s.replace("'", "\\'")
return f"'{escaped}'"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if re.search(r'[^a-zA-Z0-9_.:\-/+@\]]', s): | |
| # Backslash and single-quote cannot appear in single-quoted | |
| # strings in Rust's shlex, use double-quoting instead. | |
| if ('\\' in s or "'" in s) and '`' not in s and '$' not in s: | |
| escaped = s.replace('\\', '\\\\').replace('"', '\\"') | |
| return f'"{escaped}"' | |
| escaped = s.replace("'", "\\'") | |
| return f"'{escaped}'" | |
| if re.search(r'[^a-zA-Z0-9_.:\-/+@\]]', s): | |
| # Backslash and single-quote cannot appear in single-quoted | |
| # strings in Rust's shlex, use double-quoting instead. | |
| if '\\' in s or "'" in s: | |
| escaped = ( | |
| s.replace('\\', '\\\\') | |
| .replace('"', '\\"') | |
| .replace('`', '\\`') | |
| .replace('$', '\\$') | |
| ) | |
| return f'"{escaped}"' | |
| escaped = s.replace("'", "\\'") | |
| return f"'{escaped}'" |
🤖 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 `@tests/utils.py` around lines 61 - 68, The string-quoting logic in the helper
that builds shell-safe literals has an unsafe fallback in the single-quote path
for values containing backslashes or quotes plus backticks or dollar signs.
Update the quoting branch in the relevant utility function to route those cases
through the double-quoted form instead, and make that branch escape backslash,
double quote, backtick, and dollar-sign characters so the value is always
treated literally. Keep the single-quoted fallback only for strings that do not
need those extra escapes.
f4dda9f to
a52c44e
Compare
This output allows exporting file activity events as opentelemetry logs via otlp to be collected by compatible systems. The output is gated behind the "otel" feature, which allows regular fact builds to keep using just the gRPC and stodout outputs, no additional dependencies. WIP
Molter73
force-pushed
the
mauro/feat/otel-output
branch
from
a52c44e to
b915ce8
Compare
2 participants
Description
This output allows exporting file activity events as opentelemetry logs via otlp to be collected by compatible systems.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Tested pushing events to Loki and visualizing events in Grafana with the provided documentation.