OTEL logs #21356
Open
OTEL logs #21356
Conversation
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
github-actions
bot
added
area/docs
area/collectors
Contributor
Author
|
@cubic-dev-ai review this PR |
Contributor
@vkalintiris I've started the AI code review. It'll take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
40 issues found across 203 files
Note: This PR contains a large number of files. cubic only reviews up to 150 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all 40 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/crates/netdata-log-viewer/log-viewer-plugin/src/lib.rs">
<violation number="1" location="src/crates/netdata-log-viewer/log-viewer-plugin/src/lib.rs:83">
Hard-coding a developer-specific path prevents the plugin from watching the system journal. Point it back to the real journald directory (e.g., /var/log/journal) or make it configurable.</violation>
</file>
<file name="src/crates/netdata-plugin/schema/src/lib.rs">
<violation number="1" location="src/crates/netdata-plugin/schema/src/lib.rs:185">
`netdata_schema()` now panics for types without `x-config-*` metadata because `ConfigDeclarationBuilder::build` unwraps every field even when none were provided. Ensure config declarations are only built when all required fields exist (or provide sane defaults) so deriving `NetdataSchema` on ordinary structs doesn’t crash.</violation>
</file>
<file name="src/crates/journal-core/src/file/offset_array.rs">
<violation number="1" location="src/crates/journal-core/src/file/offset_array.rs:312">
When copying subsequent arrays, use `next_node.remaining_items` instead of the previous node’s value; otherwise the last array is read past its valid entries.</violation>
</file>
<file name="src/crates/journal-core/src/file/index_filter.rs">
<violation number="1" location="src/crates/journal-core/src/file/index_filter.rs:53">
`has_matches` returns true for conjunctions even when their intersection is empty, so AND filters can yield false positives.</violation>
<violation number="2" location="src/crates/journal-core/src/file/index_filter.rs:65">
`end + 1` overflows when `end == u32::MAX`, so filtering the final bucket panics.</violation>
</file>
<file name="src/crates/journal-engine/src/logs/query.rs">
<violation number="1" location="src/crates/journal-engine/src/logs/query.rs:202">
`Vec::with_capacity(limit)` tries to reserve `usize::MAX` elements whenever the caller leaves the limit unset, so the default path panics or OOMs before processing any logs. Use a bounded capacity (or start with an empty Vec) instead of allocating `usize::MAX`.</violation>
</file>
<file name="src/crates/netdata-log-viewer/log-viewer-plugin/src/catalog.rs">
<violation number="1" location="src/crates/netdata-log-viewer/log-viewer-plugin/src/catalog.rs:234">
Backward pagination subtracts 1 from a `u64` anchor without checking for zero, causing underflow when the anchor is at the beginning of the log.</violation>
<violation number="2" location="src/crates/netdata-log-viewer/log-viewer-plugin/src/catalog.rs:520">
Log entries are fetched without applying the user’s selection filter, so the returned table ignores the requested filters.</violation>
</file>
<file name="src/crates/journal-core/src/file/guarded_cell.rs">
<violation number="1" location="src/crates/journal-core/src/file/guarded_cell.rs:225">
`with_guarded` keeps the guard mutably borrowed while invoking the user closure, so any re-entrant guard check panics via `RefCell` instead of returning `ValueGuardInUse`. Release the borrow before calling `f` and reacquire it only to mark the guard as held after `f` succeeds.</violation>
</file>
<file name="src/crates/journal-core/src/file/file.rs">
<violation number="1" location="src/crates/journal-core/src/file/file.rs:670">
`JournalFile::sync()` flushes only the header map and window manager but ignores the writable data/field hash table mmaps, so hash table updates are not persisted despite calling `sync()`.</violation>
<violation number="2" location="src/crates/journal-core/src/file/file.rs:839">
`object_header_mut` also skips locking the GuardedCell guard, allowing concurrent remapping while a mutable header reference is live and causing UB.</violation>
</file>
<file name="src/crates/journal-engine/src/histogram.rs">
<violation number="1" location="src/crates/journal-engine/src/histogram.rs:694">
Existing partial bucket responses are never resumed because an empty file list is returned when no new buckets are created, so histograms can get stuck partially indexed after any interrupted indexing run.</violation>
</file>
<file name="src/crates/journal-registry/src/registry/mod.rs">
<violation number="1" location="src/crates/journal-registry/src/registry/mod.rs:184">
Starting the watcher after scanning the directory introduces a race window where files created between those two operations are permanently missed; initialize the watcher before (or re-scan after) the directory walk to avoid dropping journal files.</violation>
</file>
<file name="src/crates/netdata-plugin/rt/src/lib.rs">
<violation number="1" location="src/crates/netdata-plugin/rt/src/lib.rs:334">
The 400 response emitted for an empty payload is JSON but `format` is set to `"text/plain"`, so the content type is wrong. Use `application/json` to match the actual payload.</violation>
<violation number="2" location="src/crates/netdata-plugin/rt/src/lib.rs:866">
When a function name is not registered the runtime just logs an error and returns, leaving the transaction without any `FunctionResult` so Netdata never receives completion and the request hangs. Send an explicit error result (e.g., 404) before returning.</violation>
</file>
<file name="src/crates/netdata-plugin/protocol/src/tokio_codec.rs">
<violation number="1" location="src/crates/netdata-plugin/protocol/src/tokio_codec.rs:203">
Encoding `Message::FunctionProgress` panics because the encoder uses `unimplemented!()`, so sending a progress update will crash the codec instead of emitting a protocol frame.</violation>
</file>
<file name="src/crates/journal-common/src/time.rs">
<violation number="1" location="src/crates/journal-common/src/time.rs:162">
Unchecked Seconds subtraction wraps instead of panicking on underflow, contradicting the API/tests and producing bogus timestamps in release builds.</violation>
<violation number="2" location="src/crates/journal-common/src/time.rs:187">
Unchecked Microseconds subtraction wraps on underflow in release builds, so the operator violates its own panic contract and can emit wildly incorrect timestamps.</violation>
</file>
<file name="src/crates/journal-engine/src/logs/table.rs">
<violation number="1" location="src/crates/journal-engine/src/logs/table.rs:144">
Truncating cell text with `&display[..*width]` can cut through a UTF-8 code point, panicking whenever a non-ASCII value needs truncation. Ensure truncation only happens on valid character boundaries.</violation>
</file>
<file name="src/crates/netdata-plugin/types/src/http_access.rs">
<violation number="1" location="src/crates/netdata-plugin/types/src/http_access.rs:47">
`from_hex` only recognizes a lowercase `0x` prefix, so uppercase hex values (`0X…`) that were valid before now parse as zero permissions.</violation>
</file>
<file name="src/crates/journal-engine/src/indexing.rs">
<violation number="1" location="src/crates/journal-engine/src/indexing.rs:512">
Propagate iterator errors from `collect_indexes` instead of logging and returning `Ok`, otherwise callers are told indexing succeeded even when it aborted early.</violation>
</file>
<file name="src/crates/journal-core/src/file/reader.rs">
<violation number="1" location="src/crates/journal-core/src/file/reader.rs:71">
`field_data_restart` never resets `self.field_data_iterator`, so calling it does not actually restart field data enumeration and later calls keep skipping previously returned objects.</violation>
<violation number="2" location="src/crates/journal-core/src/file/reader.rs:332">
`load_remappings` ignores all errors from `field_data_objects`, so real I/O or corruption failures while loading remapping data are silently turned into success and remappings never load.</violation>
</file>
<file name="src/crates/netdata-plugin/rt/src/charts/writer.rs">
<violation number="1" location="src/crates/netdata-plugin/rt/src/charts/writer.rs:35">
Escape apostrophes (and other special characters) before wrapping chart and dimension metadata in single quotes so user-provided titles/names cannot break the generated Netdata commands.</violation>
</file>
<file name="src/crates/netdata-log-viewer/journal-function/src/netdata/transformations.rs">
<violation number="1" location="src/crates/netdata-log-viewer/journal-function/src/netdata/transformations.rs:335">
Hex `_CAP_EFFECTIVE` values without a `0x` prefix are parsed as decimal, so no capability names are ever shown.</violation>
<violation number="2" location="src/crates/netdata-log-viewer/journal-function/src/netdata/transformations.rs:580">
`MessageIdTransformation` drops the actual MESSAGE_ID and returns only the description, contradicting the documented `"UUID (description)"` output.</violation>
</file>
<file name="src/crates/netdata-plugin/docs/README.md">
<violation number="1" location="src/crates/netdata-plugin/docs/README.md:494">
The example for payload handling ends with `FUNCTION PAYLOAD END`, but the Netdata protocol actually sends `FUNCTION_PAYLOAD_END`; using the wrong command name will prevent plugins from detecting the end of payload data.</violation>
</file>
<file name="src/crates/netdata-plugin/docs/FUNCTION_UI_DEVELOPER_GUIDE.md">
<violation number="1" location="src/crates/netdata-plugin/docs/FUNCTION_UI_DEVELOPER_GUIDE.md:371">
Sample log explorer row inserts `rowOptions` in column index 1, which misaligns the `priority` column and contradicts the documented data layout. Move `rowOptions` to the end of the row.</violation>
</file>
<file name="src/crates/netdata-plugin/docs/DYNCFG.md">
<violation number="1" location="src/crates/netdata-plugin/docs/DYNCFG.md:53">
DynCfg documentation should state that the `cmds` field must be space-separated (and examples should use spaces), because the parser ignores `|` characters and otherwise registers zero supported commands.</violation>
</file>
<file name="src/crates/netdata-log-viewer/journal-function/src/netdata/types.rs">
<violation number="1" location="src/crates/netdata-log-viewer/journal-function/src/netdata/types.rs:13">
`info` defaults to `true` in `JournalRequest::default()`, but serde deserialization uses `false` because `#[serde(default)]` falls back to `bool::default()`. Requests that omit this flag will incorrectly suppress metadata. Provide an explicit serde default that returns `true`.</violation>
</file>
<file name="src/crates/netdata-plugin/schema/examples/simple_usage.rs">
<violation number="1" location="src/crates/netdata-plugin/schema/examples/simple_usage.rs:14">
The Server Settings tab lists a `workers` field, but `WebServerConfig` only defines `host` and `port`, so the generated schema advertises a nonexistent control. Add the missing field or update the metadata to match the struct.</violation>
<violation number="2" location="src/crates/netdata-plugin/schema/examples/simple_usage.rs:18">
The Security tab lists fields (`enable_tls`, `tls_cert_path`, `api_key`) that are not defined on `WebServerConfig`, so the schema describes controls that do not exist. Define these fields or remove them from the metadata.</violation>
</file>
<file name="src/crates/journal-registry/src/registry/monitor.rs">
<violation number="1" location="src/crates/journal-registry/src/registry/monitor.rs:21">
The notify watcher callback drops `Err` results silently, so filesystem monitoring failures are never reported; log or propagate the error instead of ignoring it.</violation>
</file>
<file name="src/crates/journal-registry/README.md">
<violation number="1" location="src/crates/journal-registry/README.md:29">
`main` returns immediately after spawning the background event processor, so the Tokio runtime shuts down and cancels the loop before it can handle any events. Keep the runtime alive (e.g., await the join handle or block on a signal) instead of returning immediately.</violation>
</file>
<file name="src/crates/journal-common/src/compat.rs">
<violation number="1" location="src/crates/journal-common/src/compat.rs:38">
The compatibility helper panics on a zero divisor because it performs `value % divisor` without guarding against zero, unlike the standard `is_multiple_of` which returns `false` when the divisor is zero. Add a zero check before performing the modulo to preserve compatibility.</violation>
</file>
<file name="src/crates/journal-engine/src/logs/transformations.rs">
<violation number="1" location="src/crates/journal-engine/src/logs/transformations.rs:395">
Negative realtime timestamps are split incorrectly because `/` and `%` yield mismatched seconds and a negative remainder, producing invalid nanoseconds for chrono. Use `div_euclid` and `rem_euclid` so seconds and sub-second parts stay consistent even when the timestamp is before the Unix epoch.</violation>
</file>
<file name="src/crates/netdata-plugin/rt/Cargo.toml">
<violation number="1" location="src/crates/netdata-plugin/rt/Cargo.toml:2">
Crate name `rt` does not match the workspace dependency alias `netdata-plugin-rt`, so any dependency on `netdata-plugin-rt` will fail to resolve.</violation>
</file>
<file name="src/crates/rdp/src/main.rs">
<violation number="1" location="src/crates/rdp/src/main.rs:307">
Unchecked exponentiation of the test count overflows for fuzz lengths ≥11, causing the CLI to panic or wrap before fuzzing. Guard the input or use checked/saturating arithmetic when summing `total_tests`.</violation>
</file>
<file name="src/crates/netdata-otel/otel-plugin/src/lib.rs">
<violation number="1" location="src/crates/netdata-otel/otel-plugin/src/lib.rs:79">
`run(args)` accepts CLI arguments but `async_run` ignores them, so callers cannot configure the plugin via the provided argument vector despite the documented contract.</violation>
</file>
<file name="src/crates/netdata-log-viewer/README.md">
<violation number="1" location="src/crates/netdata-log-viewer/README.md:242">
The Jaeger connectivity check probes port 4317 even though the Quick Start container mapping exposes Jaeger on host port 4318, so the documented troubleshooting command will always fail.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
src/crates/netdata-log-viewer/journal-function/src/netdata/transformations.rs
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
40 issues found across 203 files
Note: This PR contains a large number of files. cubic only reviews up to 150 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all 40 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/crates/netdata-log-viewer/log-viewer-plugin/src/lib.rs">
<violation number="1" location="src/crates/netdata-log-viewer/log-viewer-plugin/src/lib.rs:83">
Hard-coding a developer-specific path prevents the plugin from watching the system journal. Point it back to the real journald directory (e.g., /var/log/journal) or make it configurable.</violation>
</file>
<file name="src/crates/netdata-plugin/schema/src/lib.rs">
<violation number="1" location="src/crates/netdata-plugin/schema/src/lib.rs:185">
`netdata_schema()` now panics for types without `x-config-*` metadata because `ConfigDeclarationBuilder::build` unwraps every field even when none were provided. Ensure config declarations are only built when all required fields exist (or provide sane defaults) so deriving `NetdataSchema` on ordinary structs doesn’t crash.</violation>
</file>
<file name="src/crates/journal-core/src/file/offset_array.rs">
<violation number="1" location="src/crates/journal-core/src/file/offset_array.rs:312">
When copying subsequent arrays, use `next_node.remaining_items` instead of the previous node’s value; otherwise the last array is read past its valid entries.</violation>
</file>
<file name="src/crates/journal-core/src/file/index_filter.rs">
<violation number="1" location="src/crates/journal-core/src/file/index_filter.rs:53">
`has_matches` returns true for conjunctions even when their intersection is empty, so AND filters can yield false positives.</violation>
<violation number="2" location="src/crates/journal-core/src/file/index_filter.rs:65">
`end + 1` overflows when `end == u32::MAX`, so filtering the final bucket panics.</violation>
</file>
<file name="src/crates/journal-engine/src/logs/query.rs">
<violation number="1" location="src/crates/journal-engine/src/logs/query.rs:202">
`Vec::with_capacity(limit)` tries to reserve `usize::MAX` elements whenever the caller leaves the limit unset, so the default path panics or OOMs before processing any logs. Use a bounded capacity (or start with an empty Vec) instead of allocating `usize::MAX`.</violation>
</file>
<file name="src/crates/netdata-log-viewer/log-viewer-plugin/src/catalog.rs">
<violation number="1" location="src/crates/netdata-log-viewer/log-viewer-plugin/src/catalog.rs:234">
Backward pagination subtracts 1 from a `u64` anchor without checking for zero, causing underflow when the anchor is at the beginning of the log.</violation>
<violation number="2" location="src/crates/netdata-log-viewer/log-viewer-plugin/src/catalog.rs:520">
Log entries are fetched without applying the user’s selection filter, so the returned table ignores the requested filters.</violation>
</file>
<file name="src/crates/journal-core/src/file/guarded_cell.rs">
<violation number="1" location="src/crates/journal-core/src/file/guarded_cell.rs:225">
`with_guarded` keeps the guard mutably borrowed while invoking the user closure, so any re-entrant guard check panics via `RefCell` instead of returning `ValueGuardInUse`. Release the borrow before calling `f` and reacquire it only to mark the guard as held after `f` succeeds.</violation>
</file>
<file name="src/crates/journal-core/src/file/file.rs">
<violation number="1" location="src/crates/journal-core/src/file/file.rs:670">
`JournalFile::sync()` flushes only the header map and window manager but ignores the writable data/field hash table mmaps, so hash table updates are not persisted despite calling `sync()`.</violation>
<violation number="2" location="src/crates/journal-core/src/file/file.rs:839">
`object_header_mut` also skips locking the GuardedCell guard, allowing concurrent remapping while a mutable header reference is live and causing UB.</violation>
</file>
<file name="src/crates/journal-engine/src/histogram.rs">
<violation number="1" location="src/crates/journal-engine/src/histogram.rs:694">
Existing partial bucket responses are never resumed because an empty file list is returned when no new buckets are created, so histograms can get stuck partially indexed after any interrupted indexing run.</violation>
</file>
<file name="src/crates/journal-registry/src/registry/mod.rs">
<violation number="1" location="src/crates/journal-registry/src/registry/mod.rs:184">
Starting the watcher after scanning the directory introduces a race window where files created between those two operations are permanently missed; initialize the watcher before (or re-scan after) the directory walk to avoid dropping journal files.</violation>
</file>
<file name="src/crates/netdata-plugin/rt/src/lib.rs">
<violation number="1" location="src/crates/netdata-plugin/rt/src/lib.rs:334">
The 400 response emitted for an empty payload is JSON but `format` is set to `"text/plain"`, so the content type is wrong. Use `application/json` to match the actual payload.</violation>
<violation number="2" location="src/crates/netdata-plugin/rt/src/lib.rs:866">
When a function name is not registered the runtime just logs an error and returns, leaving the transaction without any `FunctionResult` so Netdata never receives completion and the request hangs. Send an explicit error result (e.g., 404) before returning.</violation>
</file>
<file name="src/crates/netdata-plugin/protocol/src/tokio_codec.rs">
<violation number="1" location="src/crates/netdata-plugin/protocol/src/tokio_codec.rs:203">
Encoding `Message::FunctionProgress` panics because the encoder uses `unimplemented!()`, so sending a progress update will crash the codec instead of emitting a protocol frame.</violation>
</file>
<file name="src/crates/journal-common/src/time.rs">
<violation number="1" location="src/crates/journal-common/src/time.rs:162">
Unchecked Seconds subtraction wraps instead of panicking on underflow, contradicting the API/tests and producing bogus timestamps in release builds.</violation>
<violation number="2" location="src/crates/journal-common/src/time.rs:187">
Unchecked Microseconds subtraction wraps on underflow in release builds, so the operator violates its own panic contract and can emit wildly incorrect timestamps.</violation>
</file>
<file name="src/crates/journal-engine/src/logs/table.rs">
<violation number="1" location="src/crates/journal-engine/src/logs/table.rs:144">
Truncating cell text with `&display[..*width]` can cut through a UTF-8 code point, panicking whenever a non-ASCII value needs truncation. Ensure truncation only happens on valid character boundaries.</violation>
</file>
<file name="src/crates/netdata-plugin/types/src/http_access.rs">
<violation number="1" location="src/crates/netdata-plugin/types/src/http_access.rs:47">
`from_hex` only recognizes a lowercase `0x` prefix, so uppercase hex values (`0X…`) that were valid before now parse as zero permissions.</violation>
</file>
<file name="src/crates/journal-engine/src/indexing.rs">
<violation number="1" location="src/crates/journal-engine/src/indexing.rs:512">
Propagate iterator errors from `collect_indexes` instead of logging and returning `Ok`, otherwise callers are told indexing succeeded even when it aborted early.</violation>
</file>
<file name="src/crates/journal-core/src/file/reader.rs">
<violation number="1" location="src/crates/journal-core/src/file/reader.rs:71">
`field_data_restart` never resets `self.field_data_iterator`, so calling it does not actually restart field data enumeration and later calls keep skipping previously returned objects.</violation>
<violation number="2" location="src/crates/journal-core/src/file/reader.rs:332">
`load_remappings` ignores all errors from `field_data_objects`, so real I/O or corruption failures while loading remapping data are silently turned into success and remappings never load.</violation>
</file>
<file name="src/crates/netdata-plugin/rt/src/charts/writer.rs">
<violation number="1" location="src/crates/netdata-plugin/rt/src/charts/writer.rs:35">
Escape apostrophes (and other special characters) before wrapping chart and dimension metadata in single quotes so user-provided titles/names cannot break the generated Netdata commands.</violation>
</file>
<file name="src/crates/netdata-log-viewer/journal-function/src/netdata/transformations.rs">
<violation number="1" location="src/crates/netdata-log-viewer/journal-function/src/netdata/transformations.rs:335">
Hex `_CAP_EFFECTIVE` values without a `0x` prefix are parsed as decimal, so no capability names are ever shown.</violation>
<violation number="2" location="src/crates/netdata-log-viewer/journal-function/src/netdata/transformations.rs:580">
`MessageIdTransformation` drops the actual MESSAGE_ID and returns only the description, contradicting the documented `"UUID (description)"` output.</violation>
</file>
<file name="src/crates/netdata-plugin/docs/README.md">
<violation number="1" location="src/crates/netdata-plugin/docs/README.md:494">
The example for payload handling ends with `FUNCTION PAYLOAD END`, but the Netdata protocol actually sends `FUNCTION_PAYLOAD_END`; using the wrong command name will prevent plugins from detecting the end of payload data.</violation>
</file>
<file name="src/crates/netdata-plugin/docs/FUNCTION_UI_DEVELOPER_GUIDE.md">
<violation number="1" location="src/crates/netdata-plugin/docs/FUNCTION_UI_DEVELOPER_GUIDE.md:371">
Sample log explorer row inserts `rowOptions` in column index 1, which misaligns the `priority` column and contradicts the documented data layout. Move `rowOptions` to the end of the row.</violation>
</file>
<file name="src/crates/netdata-plugin/docs/DYNCFG.md">
<violation number="1" location="src/crates/netdata-plugin/docs/DYNCFG.md:53">
DynCfg documentation should state that the `cmds` field must be space-separated (and examples should use spaces), because the parser ignores `|` characters and otherwise registers zero supported commands.</violation>
</file>
<file name="src/crates/netdata-log-viewer/journal-function/src/netdata/types.rs">
<violation number="1" location="src/crates/netdata-log-viewer/journal-function/src/netdata/types.rs:13">
`info` defaults to `true` in `JournalRequest::default()`, but serde deserialization uses `false` because `#[serde(default)]` falls back to `bool::default()`. Requests that omit this flag will incorrectly suppress metadata. Provide an explicit serde default that returns `true`.</violation>
</file>
<file name="src/crates/netdata-plugin/schema/examples/simple_usage.rs">
<violation number="1" location="src/crates/netdata-plugin/schema/examples/simple_usage.rs:14">
The Server Settings tab lists a `workers` field, but `WebServerConfig` only defines `host` and `port`, so the generated schema advertises a nonexistent control. Add the missing field or update the metadata to match the struct.</violation>
<violation number="2" location="src/crates/netdata-plugin/schema/examples/simple_usage.rs:18">
The Security tab lists fields (`enable_tls`, `tls_cert_path`, `api_key`) that are not defined on `WebServerConfig`, so the schema describes controls that do not exist. Define these fields or remove them from the metadata.</violation>
</file>
<file name="src/crates/journal-registry/src/registry/monitor.rs">
<violation number="1" location="src/crates/journal-registry/src/registry/monitor.rs:21">
The notify watcher callback drops `Err` results silently, so filesystem monitoring failures are never reported; log or propagate the error instead of ignoring it.</violation>
</file>
<file name="src/crates/journal-registry/README.md">
<violation number="1" location="src/crates/journal-registry/README.md:29">
`main` returns immediately after spawning the background event processor, so the Tokio runtime shuts down and cancels the loop before it can handle any events. Keep the runtime alive (e.g., await the join handle or block on a signal) instead of returning immediately.</violation>
</file>
<file name="src/crates/journal-common/src/compat.rs">
<violation number="1" location="src/crates/journal-common/src/compat.rs:38">
The compatibility helper panics on a zero divisor because it performs `value % divisor` without guarding against zero, unlike the standard `is_multiple_of` which returns `false` when the divisor is zero. Add a zero check before performing the modulo to preserve compatibility.</violation>
</file>
<file name="src/crates/journal-engine/src/logs/transformations.rs">
<violation number="1" location="src/crates/journal-engine/src/logs/transformations.rs:395">
Negative realtime timestamps are split incorrectly because `/` and `%` yield mismatched seconds and a negative remainder, producing invalid nanoseconds for chrono. Use `div_euclid` and `rem_euclid` so seconds and sub-second parts stay consistent even when the timestamp is before the Unix epoch.</violation>
</file>
<file name="src/crates/netdata-plugin/rt/Cargo.toml">
<violation number="1" location="src/crates/netdata-plugin/rt/Cargo.toml:2">
Crate name `rt` does not match the workspace dependency alias `netdata-plugin-rt`, so any dependency on `netdata-plugin-rt` will fail to resolve.</violation>
</file>
<file name="src/crates/rdp/src/main.rs">
<violation number="1" location="src/crates/rdp/src/main.rs:307">
Unchecked exponentiation of the test count overflows for fuzz lengths ≥11, causing the CLI to panic or wrap before fuzzing. Guard the input or use checked/saturating arithmetic when summing `total_tests`.</violation>
</file>
<file name="src/crates/netdata-otel/otel-plugin/src/lib.rs">
<violation number="1" location="src/crates/netdata-otel/otel-plugin/src/lib.rs:79">
`run(args)` accepts CLI arguments but `async_run` ignores them, so callers cannot configure the plugin via the provided argument vector despite the documented contract.</violation>
</file>
<file name="src/crates/netdata-log-viewer/README.md">
<violation number="1" location="src/crates/netdata-log-viewer/README.md:242">
The Jaeger connectivity check probes port 4317 even though the Quick Start container mapping exposes Jaeger on host port 4318, so the documented troubleshooting command will always fail.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| } | ||
|
|
||
| // Initialize result vector with capacity for efficiency | ||
| let mut collected_entries: Vec<LogEntryId> = Vec::with_capacity(limit); |
Contributor
There was a problem hiding this comment.
Vec::with_capacity(limit) tries to reserve usize::MAX elements whenever the caller leaves the limit unset, so the default path panics or OOMs before processing any logs. Use a bounded capacity (or start with an empty Vec) instead of allocating usize::MAX.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/crates/journal-engine/src/logs/query.rs, line 202:
<comment>`Vec::with_capacity(limit)` tries to reserve `usize::MAX` elements whenever the caller leaves the limit unset, so the default path panics or OOMs before processing any logs. Use a bounded capacity (or start with an empty Vec) instead of allocating `usize::MAX`.</comment>
<file context>
@@ -0,0 +1,414 @@
+ }
+
+ // Initialize result vector with capacity for efficiency
+ let mut collected_entries: Vec<LogEntryId> = Vec::with_capacity(limit);
+
+ for file_index in relevant_indexes {
</file context>
Suggested change
| let mut collected_entries: Vec<LogEntryId> = Vec::with_capacity(limit); | |
| let mut collected_entries: Vec<LogEntryId> = Vec::new(); |
src/crates/netdata-log-viewer/journal-function/src/netdata/transformations.rs
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
40 issues found across 203 files
Note: This PR contains a large number of files. cubic only reviews up to 150 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all 40 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/crates/netdata-log-viewer/log-viewer-plugin/src/lib.rs">
<violation number="1" location="src/crates/netdata-log-viewer/log-viewer-plugin/src/lib.rs:83">
Hard-coding a developer-specific path prevents the plugin from watching the system journal. Point it back to the real journald directory (e.g., /var/log/journal) or make it configurable.</violation>
</file>
<file name="src/crates/netdata-plugin/schema/src/lib.rs">
<violation number="1" location="src/crates/netdata-plugin/schema/src/lib.rs:185">
`netdata_schema()` now panics for types without `x-config-*` metadata because `ConfigDeclarationBuilder::build` unwraps every field even when none were provided. Ensure config declarations are only built when all required fields exist (or provide sane defaults) so deriving `NetdataSchema` on ordinary structs doesn’t crash.</violation>
</file>
<file name="src/crates/journal-core/src/file/offset_array.rs">
<violation number="1" location="src/crates/journal-core/src/file/offset_array.rs:312">
When copying subsequent arrays, use `next_node.remaining_items` instead of the previous node’s value; otherwise the last array is read past its valid entries.</violation>
</file>
<file name="src/crates/journal-core/src/file/index_filter.rs">
<violation number="1" location="src/crates/journal-core/src/file/index_filter.rs:53">
`has_matches` returns true for conjunctions even when their intersection is empty, so AND filters can yield false positives.</violation>
<violation number="2" location="src/crates/journal-core/src/file/index_filter.rs:65">
`end + 1` overflows when `end == u32::MAX`, so filtering the final bucket panics.</violation>
</file>
<file name="src/crates/journal-engine/src/logs/query.rs">
<violation number="1" location="src/crates/journal-engine/src/logs/query.rs:202">
`Vec::with_capacity(limit)` tries to reserve `usize::MAX` elements whenever the caller leaves the limit unset, so the default path panics or OOMs before processing any logs. Use a bounded capacity (or start with an empty Vec) instead of allocating `usize::MAX`.</violation>
</file>
<file name="src/crates/netdata-log-viewer/log-viewer-plugin/src/catalog.rs">
<violation number="1" location="src/crates/netdata-log-viewer/log-viewer-plugin/src/catalog.rs:234">
Backward pagination subtracts 1 from a `u64` anchor without checking for zero, causing underflow when the anchor is at the beginning of the log.</violation>
<violation number="2" location="src/crates/netdata-log-viewer/log-viewer-plugin/src/catalog.rs:520">
Log entries are fetched without applying the user’s selection filter, so the returned table ignores the requested filters.</violation>
</file>
<file name="src/crates/journal-core/src/file/guarded_cell.rs">
<violation number="1" location="src/crates/journal-core/src/file/guarded_cell.rs:225">
`with_guarded` keeps the guard mutably borrowed while invoking the user closure, so any re-entrant guard check panics via `RefCell` instead of returning `ValueGuardInUse`. Release the borrow before calling `f` and reacquire it only to mark the guard as held after `f` succeeds.</violation>
</file>
<file name="src/crates/journal-core/src/file/file.rs">
<violation number="1" location="src/crates/journal-core/src/file/file.rs:670">
`JournalFile::sync()` flushes only the header map and window manager but ignores the writable data/field hash table mmaps, so hash table updates are not persisted despite calling `sync()`.</violation>
<violation number="2" location="src/crates/journal-core/src/file/file.rs:839">
`object_header_mut` also skips locking the GuardedCell guard, allowing concurrent remapping while a mutable header reference is live and causing UB.</violation>
</file>
<file name="src/crates/journal-engine/src/histogram.rs">
<violation number="1" location="src/crates/journal-engine/src/histogram.rs:694">
Existing partial bucket responses are never resumed because an empty file list is returned when no new buckets are created, so histograms can get stuck partially indexed after any interrupted indexing run.</violation>
</file>
<file name="src/crates/journal-registry/src/registry/mod.rs">
<violation number="1" location="src/crates/journal-registry/src/registry/mod.rs:184">
Starting the watcher after scanning the directory introduces a race window where files created between those two operations are permanently missed; initialize the watcher before (or re-scan after) the directory walk to avoid dropping journal files.</violation>
</file>
<file name="src/crates/netdata-plugin/rt/src/lib.rs">
<violation number="1" location="src/crates/netdata-plugin/rt/src/lib.rs:334">
The 400 response emitted for an empty payload is JSON but `format` is set to `"text/plain"`, so the content type is wrong. Use `application/json` to match the actual payload.</violation>
<violation number="2" location="src/crates/netdata-plugin/rt/src/lib.rs:866">
When a function name is not registered the runtime just logs an error and returns, leaving the transaction without any `FunctionResult` so Netdata never receives completion and the request hangs. Send an explicit error result (e.g., 404) before returning.</violation>
</file>
<file name="src/crates/netdata-plugin/protocol/src/tokio_codec.rs">
<violation number="1" location="src/crates/netdata-plugin/protocol/src/tokio_codec.rs:203">
Encoding `Message::FunctionProgress` panics because the encoder uses `unimplemented!()`, so sending a progress update will crash the codec instead of emitting a protocol frame.</violation>
</file>
<file name="src/crates/journal-common/src/time.rs">
<violation number="1" location="src/crates/journal-common/src/time.rs:162">
Unchecked Seconds subtraction wraps instead of panicking on underflow, contradicting the API/tests and producing bogus timestamps in release builds.</violation>
<violation number="2" location="src/crates/journal-common/src/time.rs:187">
Unchecked Microseconds subtraction wraps on underflow in release builds, so the operator violates its own panic contract and can emit wildly incorrect timestamps.</violation>
</file>
<file name="src/crates/journal-engine/src/logs/table.rs">
<violation number="1" location="src/crates/journal-engine/src/logs/table.rs:144">
Truncating cell text with `&display[..*width]` can cut through a UTF-8 code point, panicking whenever a non-ASCII value needs truncation. Ensure truncation only happens on valid character boundaries.</violation>
</file>
<file name="src/crates/netdata-plugin/types/src/http_access.rs">
<violation number="1" location="src/crates/netdata-plugin/types/src/http_access.rs:47">
`from_hex` only recognizes a lowercase `0x` prefix, so uppercase hex values (`0X…`) that were valid before now parse as zero permissions.</violation>
</file>
<file name="src/crates/journal-engine/src/indexing.rs">
<violation number="1" location="src/crates/journal-engine/src/indexing.rs:512">
Propagate iterator errors from `collect_indexes` instead of logging and returning `Ok`, otherwise callers are told indexing succeeded even when it aborted early.</violation>
</file>
<file name="src/crates/journal-core/src/file/reader.rs">
<violation number="1" location="src/crates/journal-core/src/file/reader.rs:71">
`field_data_restart` never resets `self.field_data_iterator`, so calling it does not actually restart field data enumeration and later calls keep skipping previously returned objects.</violation>
<violation number="2" location="src/crates/journal-core/src/file/reader.rs:332">
`load_remappings` ignores all errors from `field_data_objects`, so real I/O or corruption failures while loading remapping data are silently turned into success and remappings never load.</violation>
</file>
<file name="src/crates/netdata-plugin/rt/src/charts/writer.rs">
<violation number="1" location="src/crates/netdata-plugin/rt/src/charts/writer.rs:35">
Escape apostrophes (and other special characters) before wrapping chart and dimension metadata in single quotes so user-provided titles/names cannot break the generated Netdata commands.</violation>
</file>
<file name="src/crates/netdata-log-viewer/journal-function/src/netdata/transformations.rs">
<violation number="1" location="src/crates/netdata-log-viewer/journal-function/src/netdata/transformations.rs:335">
Hex `_CAP_EFFECTIVE` values without a `0x` prefix are parsed as decimal, so no capability names are ever shown.</violation>
<violation number="2" location="src/crates/netdata-log-viewer/journal-function/src/netdata/transformations.rs:580">
`MessageIdTransformation` drops the actual MESSAGE_ID and returns only the description, contradicting the documented `"UUID (description)"` output.</violation>
</file>
<file name="src/crates/netdata-plugin/docs/README.md">
<violation number="1" location="src/crates/netdata-plugin/docs/README.md:494">
The example for payload handling ends with `FUNCTION PAYLOAD END`, but the Netdata protocol actually sends `FUNCTION_PAYLOAD_END`; using the wrong command name will prevent plugins from detecting the end of payload data.</violation>
</file>
<file name="src/crates/netdata-plugin/docs/FUNCTION_UI_DEVELOPER_GUIDE.md">
<violation number="1" location="src/crates/netdata-plugin/docs/FUNCTION_UI_DEVELOPER_GUIDE.md:371">
Sample log explorer row inserts `rowOptions` in column index 1, which misaligns the `priority` column and contradicts the documented data layout. Move `rowOptions` to the end of the row.</violation>
</file>
<file name="src/crates/netdata-plugin/docs/DYNCFG.md">
<violation number="1" location="src/crates/netdata-plugin/docs/DYNCFG.md:53">
DynCfg documentation should state that the `cmds` field must be space-separated (and examples should use spaces), because the parser ignores `|` characters and otherwise registers zero supported commands.</violation>
</file>
<file name="src/crates/netdata-log-viewer/journal-function/src/netdata/types.rs">
<violation number="1" location="src/crates/netdata-log-viewer/journal-function/src/netdata/types.rs:13">
`info` defaults to `true` in `JournalRequest::default()`, but serde deserialization uses `false` because `#[serde(default)]` falls back to `bool::default()`. Requests that omit this flag will incorrectly suppress metadata. Provide an explicit serde default that returns `true`.</violation>
</file>
<file name="src/crates/netdata-plugin/schema/examples/simple_usage.rs">
<violation number="1" location="src/crates/netdata-plugin/schema/examples/simple_usage.rs:14">
The Server Settings tab lists a `workers` field, but `WebServerConfig` only defines `host` and `port`, so the generated schema advertises a nonexistent control. Add the missing field or update the metadata to match the struct.</violation>
<violation number="2" location="src/crates/netdata-plugin/schema/examples/simple_usage.rs:18">
The Security tab lists fields (`enable_tls`, `tls_cert_path`, `api_key`) that are not defined on `WebServerConfig`, so the schema describes controls that do not exist. Define these fields or remove them from the metadata.</violation>
</file>
<file name="src/crates/journal-registry/src/registry/monitor.rs">
<violation number="1" location="src/crates/journal-registry/src/registry/monitor.rs:21">
The notify watcher callback drops `Err` results silently, so filesystem monitoring failures are never reported; log or propagate the error instead of ignoring it.</violation>
</file>
<file name="src/crates/journal-registry/README.md">
<violation number="1" location="src/crates/journal-registry/README.md:29">
`main` returns immediately after spawning the background event processor, so the Tokio runtime shuts down and cancels the loop before it can handle any events. Keep the runtime alive (e.g., await the join handle or block on a signal) instead of returning immediately.</violation>
</file>
<file name="src/crates/journal-common/src/compat.rs">
<violation number="1" location="src/crates/journal-common/src/compat.rs:38">
The compatibility helper panics on a zero divisor because it performs `value % divisor` without guarding against zero, unlike the standard `is_multiple_of` which returns `false` when the divisor is zero. Add a zero check before performing the modulo to preserve compatibility.</violation>
</file>
<file name="src/crates/journal-engine/src/logs/transformations.rs">
<violation number="1" location="src/crates/journal-engine/src/logs/transformations.rs:395">
Negative realtime timestamps are split incorrectly because `/` and `%` yield mismatched seconds and a negative remainder, producing invalid nanoseconds for chrono. Use `div_euclid` and `rem_euclid` so seconds and sub-second parts stay consistent even when the timestamp is before the Unix epoch.</violation>
</file>
<file name="src/crates/netdata-plugin/rt/Cargo.toml">
<violation number="1" location="src/crates/netdata-plugin/rt/Cargo.toml:2">
Crate name `rt` does not match the workspace dependency alias `netdata-plugin-rt`, so any dependency on `netdata-plugin-rt` will fail to resolve.</violation>
</file>
<file name="src/crates/rdp/src/main.rs">
<violation number="1" location="src/crates/rdp/src/main.rs:307">
Unchecked exponentiation of the test count overflows for fuzz lengths ≥11, causing the CLI to panic or wrap before fuzzing. Guard the input or use checked/saturating arithmetic when summing `total_tests`.</violation>
</file>
<file name="src/crates/netdata-otel/otel-plugin/src/lib.rs">
<violation number="1" location="src/crates/netdata-otel/otel-plugin/src/lib.rs:79">
`run(args)` accepts CLI arguments but `async_run` ignores them, so callers cannot configure the plugin via the provided argument vector despite the documented contract.</violation>
</file>
<file name="src/crates/netdata-log-viewer/README.md">
<violation number="1" location="src/crates/netdata-log-viewer/README.md:242">
The Jaeger connectivity check probes port 4317 even though the Quick Start container mapping exposes Jaeger on host port 4318, so the documented troubleshooting command will always fail.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| info!("Catalog function initialized"); | ||
|
|
||
| // let path = "/var/log/journal"; | ||
| let path = "/home/vk/repos/tmp/aws-4320"; |
Contributor
There was a problem hiding this comment.
Hard-coding a developer-specific path prevents the plugin from watching the system journal. Point it back to the real journald directory (e.g., /var/log/journal) or make it configurable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/crates/netdata-log-viewer/log-viewer-plugin/src/lib.rs, line 83:
<comment>Hard-coding a developer-specific path prevents the plugin from watching the system journal. Point it back to the real journald directory (e.g., /var/log/journal) or make it configurable.</comment>
<file context>
@@ -0,0 +1,108 @@
+ info!("Catalog function initialized");
+
+ // let path = "/var/log/journal";
+ let path = "/home/vk/repos/tmp/aws-4320";
+ match catalog_function.watch_directory(path) {
+ Ok(()) => {}
</file context>
Suggested change
| let path = "/home/vk/repos/tmp/aws-4320"; | |
| let path = "/var/log/journal"; |
| } | ||
|
|
||
| // Build config declaration | ||
| self.config_declaration = Some(config_props.build()); |
Contributor
There was a problem hiding this comment.
netdata_schema() now panics for types without x-config-* metadata because ConfigDeclarationBuilder::build unwraps every field even when none were provided. Ensure config declarations are only built when all required fields exist (or provide sane defaults) so deriving NetdataSchema on ordinary structs doesn’t crash.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/crates/netdata-plugin/schema/src/lib.rs, line 185:
<comment>`netdata_schema()` now panics for types without `x-config-*` metadata because `ConfigDeclarationBuilder::build` unwraps every field even when none were provided. Ensure config declarations are only built when all required fields exist (or provide sane defaults) so deriving `NetdataSchema` on ordinary structs doesn’t crash.</comment>
<file context>
@@ -0,0 +1,300 @@
+ }
+
+ // Build config declaration
+ self.config_declaration = Some(config_props.build());
+ }
+}
</file context>
| loop { | ||
| { | ||
| let array = journal_file.offset_array_ref(node.offset())?; | ||
| let remaining_items = node.remaining_items.get(); |
Contributor
There was a problem hiding this comment.
When copying subsequent arrays, use next_node.remaining_items instead of the previous node’s value; otherwise the last array is read past its valid entries.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/crates/journal-core/src/file/offset_array.rs, line 312:
<comment>When copying subsequent arrays, use `next_node.remaining_items` instead of the previous node’s value; otherwise the last array is read past its valid entries.</comment>
<file context>
@@ -294,10 +295,37 @@ impl List {
+ loop {
+ {
+ let array = journal_file.offset_array_ref(node.offset())?;
+ let remaining_items = node.remaining_items.get();
+ array.collect_offsets(0, remaining_items, offsets)?;
+ }
</file context>
Suggested change
| let remaining_items = node.remaining_items.get(); | |
| let remaining_items = next_node.remaining_items.get(); |
| IndexFilterExpr::None => false, | ||
| IndexFilterExpr::Match(bitmap) => !bitmap.is_empty(), | ||
| IndexFilterExpr::Conjunction(filter_exprs) => { | ||
| filter_exprs.iter().all(|expr| expr.has_matches()) |
Contributor
There was a problem hiding this comment.
has_matches returns true for conjunctions even when their intersection is empty, so AND filters can yield false positives.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/crates/journal-core/src/file/index_filter.rs, line 53:
<comment>`has_matches` returns true for conjunctions even when their intersection is empty, so AND filters can yield false positives.</comment>
<file context>
@@ -0,0 +1,369 @@
+ IndexFilterExpr::None => false,
+ IndexFilterExpr::Match(bitmap) => !bitmap.is_empty(),
+ IndexFilterExpr::Conjunction(filter_exprs) => {
+ filter_exprs.iter().all(|expr| expr.has_matches())
+ }
+ IndexFilterExpr::Disjunction(filter_exprs) => {
</file context>
| } | ||
|
|
||
| // Initialize result vector with capacity for efficiency | ||
| let mut collected_entries: Vec<LogEntryId> = Vec::with_capacity(limit); |
Contributor
There was a problem hiding this comment.
Vec::with_capacity(limit) tries to reserve usize::MAX elements whenever the caller leaves the limit unset, so the default path panics or OOMs before processing any logs. Use a bounded capacity (or start with an empty Vec) instead of allocating usize::MAX.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/crates/journal-engine/src/logs/query.rs, line 202:
<comment>`Vec::with_capacity(limit)` tries to reserve `usize::MAX` elements whenever the caller leaves the limit unset, so the default path panics or OOMs before processing any logs. Use a bounded capacity (or start with an empty Vec) instead of allocating `usize::MAX`.</comment>
<file context>
@@ -0,0 +1,414 @@
+ }
+
+ // Initialize result vector with capacity for efficiency
+ let mut collected_entries: Vec<LogEntryId> = Vec::with_capacity(limit);
+
+ for file_index in relevant_indexes {
</file context>
Suggested change
| let mut collected_entries: Vec<LogEntryId> = Vec::with_capacity(limit); | |
| let mut collected_entries: Vec<LogEntryId> = Vec::new(); |
| runtime.block_on(async_run(args)) | ||
| } | ||
|
|
||
| async fn async_run(_args: Vec<String>) -> i32 { |
Contributor
There was a problem hiding this comment.
run(args) accepts CLI arguments but async_run ignores them, so callers cannot configure the plugin via the provided argument vector despite the documented contract.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/crates/netdata-otel/otel-plugin/src/lib.rs, line 79:
<comment>`run(args)` accepts CLI arguments but `async_run` ignores them, so callers cannot configure the plugin via the provided argument vector despite the documented contract.</comment>
<file context>
@@ -0,0 +1,192 @@
+ runtime.block_on(async_run(args))
+}
+
+async fn async_run(_args: Vec<String>) -> i32 {
+ initialize_tracing();
+
</file context>
| } | ||
|
|
||
| pub fn step(&mut self, journal_file: &'a JournalFile<M>, direction: Direction) -> Result<bool> { | ||
| self.drop_guards(); |
Contributor
There was a problem hiding this comment.
field_data_restart never resets self.field_data_iterator, so calling it does not actually restart field data enumeration and later calls keep skipping previously returned objects.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/crates/journal-core/src/file/reader.rs, line 71:
<comment>`field_data_restart` never resets `self.field_data_iterator`, so calling it does not actually restart field data enumeration and later calls keep skipping previously returned objects.</comment>
<file context>
@@ -0,0 +1,466 @@
+ }
+
+ pub fn step(&mut self, journal_file: &'a JournalFile<M>, direction: Direction) -> Result<bool> {
+ self.drop_guards();
+
+ if let Some(filter) = self.filter.as_mut() {
</file context>
| docker ps | grep jaeger | ||
|
|
||
| # Check connectivity | ||
| nc -zv localhost 4317 |
Contributor
There was a problem hiding this comment.
The Jaeger connectivity check probes port 4317 even though the Quick Start container mapping exposes Jaeger on host port 4318, so the documented troubleshooting command will always fail.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/crates/netdata-log-viewer/README.md, line 242:
<comment>The Jaeger connectivity check probes port 4317 even though the Quick Start container mapping exposes Jaeger on host port 4318, so the documented troubleshooting command will always fail.</comment>
<file context>
@@ -0,0 +1,258 @@
+docker ps | grep jaeger
+
+# Check connectivity
+nc -zv localhost 4317
+```
+
</file context>
Suggested change
| nc -zv localhost 4317 | |
| +nc -zv localhost 4318 |
| transaction, | ||
| status: 400, | ||
| expires: 0, | ||
| format: "text/plain".to_string(), |
Contributor
There was a problem hiding this comment.
The 400 response emitted for an empty payload is JSON but format is set to "text/plain", so the content type is wrong. Use application/json to match the actual payload.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/crates/netdata-plugin/rt/src/lib.rs, line 334:
<comment>The 400 response emitted for an empty payload is JSON but `format` is set to `"text/plain"`, so the content type is wrong. Use `application/json` to match the actual payload.</comment>
<file context>
@@ -0,0 +1,1015 @@
+ transaction,
+ status: 400,
+ expires: 0,
+ format: "text/plain".to_string(),
+ payload: format!("Invalid request: {}", e).as_bytes().to_vec(),
+ };
</file context>
Suggested change
| format: "text/plain".to_string(), | |
| format: "application/json".to_string(), |
| _ => return raw_value.to_string(), | ||
| }; | ||
|
|
||
| description.to_string() |
Contributor
There was a problem hiding this comment.
MessageIdTransformation drops the actual MESSAGE_ID and returns only the description, contradicting the documented "UUID (description)" output.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/crates/netdata-log-viewer/journal-function/src/netdata/transformations.rs, line 580:
<comment>`MessageIdTransformation` drops the actual MESSAGE_ID and returns only the description, contradicting the documented `"UUID (description)"` output.</comment>
<file context>
@@ -0,0 +1,613 @@
+ _ => return raw_value.to_string(),
+ };
+
+ description.to_string()
+ }
+}
</file context>
Suggested change
| description.to_string() | |
| format!("{} ({})", raw_value, description) |
Contributor
There was a problem hiding this comment.
40 issues found across 203 files
Note: This PR contains a large number of files. cubic only reviews up to 150 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all 40 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/crates/netdata-log-viewer/log-viewer-plugin/src/lib.rs">
<violation number="1" location="src/crates/netdata-log-viewer/log-viewer-plugin/src/lib.rs:83">
Hard-coding a developer-specific path prevents the plugin from watching the system journal. Point it back to the real journald directory (e.g., /var/log/journal) or make it configurable.</violation>
</file>
<file name="src/crates/netdata-plugin/schema/src/lib.rs">
<violation number="1" location="src/crates/netdata-plugin/schema/src/lib.rs:185">
`netdata_schema()` now panics for types without `x-config-*` metadata because `ConfigDeclarationBuilder::build` unwraps every field even when none were provided. Ensure config declarations are only built when all required fields exist (or provide sane defaults) so deriving `NetdataSchema` on ordinary structs doesn’t crash.</violation>
</file>
<file name="src/crates/journal-core/src/file/offset_array.rs">
<violation number="1" location="src/crates/journal-core/src/file/offset_array.rs:312">
When copying subsequent arrays, use `next_node.remaining_items` instead of the previous node’s value; otherwise the last array is read past its valid entries.</violation>
</file>
<file name="src/crates/journal-core/src/file/index_filter.rs">
<violation number="1" location="src/crates/journal-core/src/file/index_filter.rs:53">
`has_matches` returns true for conjunctions even when their intersection is empty, so AND filters can yield false positives.</violation>
<violation number="2" location="src/crates/journal-core/src/file/index_filter.rs:65">
`end + 1` overflows when `end == u32::MAX`, so filtering the final bucket panics.</violation>
</file>
<file name="src/crates/journal-engine/src/logs/query.rs">
<violation number="1" location="src/crates/journal-engine/src/logs/query.rs:202">
`Vec::with_capacity(limit)` tries to reserve `usize::MAX` elements whenever the caller leaves the limit unset, so the default path panics or OOMs before processing any logs. Use a bounded capacity (or start with an empty Vec) instead of allocating `usize::MAX`.</violation>
</file>
<file name="src/crates/netdata-log-viewer/log-viewer-plugin/src/catalog.rs">
<violation number="1" location="src/crates/netdata-log-viewer/log-viewer-plugin/src/catalog.rs:234">
Backward pagination subtracts 1 from a `u64` anchor without checking for zero, causing underflow when the anchor is at the beginning of the log.</violation>
<violation number="2" location="src/crates/netdata-log-viewer/log-viewer-plugin/src/catalog.rs:520">
Log entries are fetched without applying the user’s selection filter, so the returned table ignores the requested filters.</violation>
</file>
<file name="src/crates/journal-core/src/file/guarded_cell.rs">
<violation number="1" location="src/crates/journal-core/src/file/guarded_cell.rs:225">
`with_guarded` keeps the guard mutably borrowed while invoking the user closure, so any re-entrant guard check panics via `RefCell` instead of returning `ValueGuardInUse`. Release the borrow before calling `f` and reacquire it only to mark the guard as held after `f` succeeds.</violation>
</file>
<file name="src/crates/journal-core/src/file/file.rs">
<violation number="1" location="src/crates/journal-core/src/file/file.rs:670">
`JournalFile::sync()` flushes only the header map and window manager but ignores the writable data/field hash table mmaps, so hash table updates are not persisted despite calling `sync()`.</violation>
<violation number="2" location="src/crates/journal-core/src/file/file.rs:839">
`object_header_mut` also skips locking the GuardedCell guard, allowing concurrent remapping while a mutable header reference is live and causing UB.</violation>
</file>
<file name="src/crates/journal-engine/src/histogram.rs">
<violation number="1" location="src/crates/journal-engine/src/histogram.rs:694">
Existing partial bucket responses are never resumed because an empty file list is returned when no new buckets are created, so histograms can get stuck partially indexed after any interrupted indexing run.</violation>
</file>
<file name="src/crates/journal-registry/src/registry/mod.rs">
<violation number="1" location="src/crates/journal-registry/src/registry/mod.rs:184">
Starting the watcher after scanning the directory introduces a race window where files created between those two operations are permanently missed; initialize the watcher before (or re-scan after) the directory walk to avoid dropping journal files.</violation>
</file>
<file name="src/crates/netdata-plugin/rt/src/lib.rs">
<violation number="1" location="src/crates/netdata-plugin/rt/src/lib.rs:334">
The 400 response emitted for an empty payload is JSON but `format` is set to `"text/plain"`, so the content type is wrong. Use `application/json` to match the actual payload.</violation>
<violation number="2" location="src/crates/netdata-plugin/rt/src/lib.rs:866">
When a function name is not registered the runtime just logs an error and returns, leaving the transaction without any `FunctionResult` so Netdata never receives completion and the request hangs. Send an explicit error result (e.g., 404) before returning.</violation>
</file>
<file name="src/crates/netdata-plugin/protocol/src/tokio_codec.rs">
<violation number="1" location="src/crates/netdata-plugin/protocol/src/tokio_codec.rs:203">
Encoding `Message::FunctionProgress` panics because the encoder uses `unimplemented!()`, so sending a progress update will crash the codec instead of emitting a protocol frame.</violation>
</file>
<file name="src/crates/journal-common/src/time.rs">
<violation number="1" location="src/crates/journal-common/src/time.rs:162">
Unchecked Seconds subtraction wraps instead of panicking on underflow, contradicting the API/tests and producing bogus timestamps in release builds.</violation>
<violation number="2" location="src/crates/journal-common/src/time.rs:187">
Unchecked Microseconds subtraction wraps on underflow in release builds, so the operator violates its own panic contract and can emit wildly incorrect timestamps.</violation>
</file>
<file name="src/crates/journal-engine/src/logs/table.rs">
<violation number="1" location="src/crates/journal-engine/src/logs/table.rs:144">
Truncating cell text with `&display[..*width]` can cut through a UTF-8 code point, panicking whenever a non-ASCII value needs truncation. Ensure truncation only happens on valid character boundaries.</violation>
</file>
<file name="src/crates/netdata-plugin/types/src/http_access.rs">
<violation number="1" location="src/crates/netdata-plugin/types/src/http_access.rs:47">
`from_hex` only recognizes a lowercase `0x` prefix, so uppercase hex values (`0X…`) that were valid before now parse as zero permissions.</violation>
</file>
<file name="src/crates/journal-engine/src/indexing.rs">
<violation number="1" location="src/crates/journal-engine/src/indexing.rs:512">
Propagate iterator errors from `collect_indexes` instead of logging and returning `Ok`, otherwise callers are told indexing succeeded even when it aborted early.</violation>
</file>
<file name="src/crates/journal-core/src/file/reader.rs">
<violation number="1" location="src/crates/journal-core/src/file/reader.rs:71">
`field_data_restart` never resets `self.field_data_iterator`, so calling it does not actually restart field data enumeration and later calls keep skipping previously returned objects.</violation>
<violation number="2" location="src/crates/journal-core/src/file/reader.rs:332">
`load_remappings` ignores all errors from `field_data_objects`, so real I/O or corruption failures while loading remapping data are silently turned into success and remappings never load.</violation>
</file>
<file name="src/crates/netdata-plugin/rt/src/charts/writer.rs">
<violation number="1" location="src/crates/netdata-plugin/rt/src/charts/writer.rs:35">
Escape apostrophes (and other special characters) before wrapping chart and dimension metadata in single quotes so user-provided titles/names cannot break the generated Netdata commands.</violation>
</file>
<file name="src/crates/netdata-log-viewer/journal-function/src/netdata/transformations.rs">
<violation number="1" location="src/crates/netdata-log-viewer/journal-function/src/netdata/transformations.rs:335">
Hex `_CAP_EFFECTIVE` values without a `0x` prefix are parsed as decimal, so no capability names are ever shown.</violation>
<violation number="2" location="src/crates/netdata-log-viewer/journal-function/src/netdata/transformations.rs:580">
`MessageIdTransformation` drops the actual MESSAGE_ID and returns only the description, contradicting the documented `"UUID (description)"` output.</violation>
</file>
<file name="src/crates/netdata-plugin/docs/README.md">
<violation number="1" location="src/crates/netdata-plugin/docs/README.md:494">
The example for payload handling ends with `FUNCTION PAYLOAD END`, but the Netdata protocol actually sends `FUNCTION_PAYLOAD_END`; using the wrong command name will prevent plugins from detecting the end of payload data.</violation>
</file>
<file name="src/crates/netdata-plugin/docs/FUNCTION_UI_DEVELOPER_GUIDE.md">
<violation number="1" location="src/crates/netdata-plugin/docs/FUNCTION_UI_DEVELOPER_GUIDE.md:371">
Sample log explorer row inserts `rowOptions` in column index 1, which misaligns the `priority` column and contradicts the documented data layout. Move `rowOptions` to the end of the row.</violation>
</file>
<file name="src/crates/netdata-plugin/docs/DYNCFG.md">
<violation number="1" location="src/crates/netdata-plugin/docs/DYNCFG.md:53">
DynCfg documentation should state that the `cmds` field must be space-separated (and examples should use spaces), because the parser ignores `|` characters and otherwise registers zero supported commands.</violation>
</file>
<file name="src/crates/netdata-log-viewer/journal-function/src/netdata/types.rs">
<violation number="1" location="src/crates/netdata-log-viewer/journal-function/src/netdata/types.rs:13">
`info` defaults to `true` in `JournalRequest::default()`, but serde deserialization uses `false` because `#[serde(default)]` falls back to `bool::default()`. Requests that omit this flag will incorrectly suppress metadata. Provide an explicit serde default that returns `true`.</violation>
</file>
<file name="src/crates/netdata-plugin/schema/examples/simple_usage.rs">
<violation number="1" location="src/crates/netdata-plugin/schema/examples/simple_usage.rs:14">
The Server Settings tab lists a `workers` field, but `WebServerConfig` only defines `host` and `port`, so the generated schema advertises a nonexistent control. Add the missing field or update the metadata to match the struct.</violation>
<violation number="2" location="src/crates/netdata-plugin/schema/examples/simple_usage.rs:18">
The Security tab lists fields (`enable_tls`, `tls_cert_path`, `api_key`) that are not defined on `WebServerConfig`, so the schema describes controls that do not exist. Define these fields or remove them from the metadata.</violation>
</file>
<file name="src/crates/journal-registry/src/registry/monitor.rs">
<violation number="1" location="src/crates/journal-registry/src/registry/monitor.rs:21">
The notify watcher callback drops `Err` results silently, so filesystem monitoring failures are never reported; log or propagate the error instead of ignoring it.</violation>
</file>
<file name="src/crates/journal-registry/README.md">
<violation number="1" location="src/crates/journal-registry/README.md:29">
`main` returns immediately after spawning the background event processor, so the Tokio runtime shuts down and cancels the loop before it can handle any events. Keep the runtime alive (e.g., await the join handle or block on a signal) instead of returning immediately.</violation>
</file>
<file name="src/crates/journal-common/src/compat.rs">
<violation number="1" location="src/crates/journal-common/src/compat.rs:38">
The compatibility helper panics on a zero divisor because it performs `value % divisor` without guarding against zero, unlike the standard `is_multiple_of` which returns `false` when the divisor is zero. Add a zero check before performing the modulo to preserve compatibility.</violation>
</file>
<file name="src/crates/journal-engine/src/logs/transformations.rs">
<violation number="1" location="src/crates/journal-engine/src/logs/transformations.rs:395">
Negative realtime timestamps are split incorrectly because `/` and `%` yield mismatched seconds and a negative remainder, producing invalid nanoseconds for chrono. Use `div_euclid` and `rem_euclid` so seconds and sub-second parts stay consistent even when the timestamp is before the Unix epoch.</violation>
</file>
<file name="src/crates/netdata-plugin/rt/Cargo.toml">
<violation number="1" location="src/crates/netdata-plugin/rt/Cargo.toml:2">
Crate name `rt` does not match the workspace dependency alias `netdata-plugin-rt`, so any dependency on `netdata-plugin-rt` will fail to resolve.</violation>
</file>
<file name="src/crates/rdp/src/main.rs">
<violation number="1" location="src/crates/rdp/src/main.rs:307">
Unchecked exponentiation of the test count overflows for fuzz lengths ≥11, causing the CLI to panic or wrap before fuzzing. Guard the input or use checked/saturating arithmetic when summing `total_tests`.</violation>
</file>
<file name="src/crates/netdata-otel/otel-plugin/src/lib.rs">
<violation number="1" location="src/crates/netdata-otel/otel-plugin/src/lib.rs:79">
`run(args)` accepts CLI arguments but `async_run` ignores them, so callers cannot configure the plugin via the provided argument vector despite the documented contract.</violation>
</file>
<file name="src/crates/netdata-log-viewer/README.md">
<violation number="1" location="src/crates/netdata-log-viewer/README.md:242">
The Jaeger connectivity check probes port 4317 even though the Quick Start container mapping exposes Jaeger on host port 4318, so the documented troubleshooting command will always fail.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| info!("Catalog function initialized"); | ||
|
|
||
| // let path = "/var/log/journal"; | ||
| let path = "/home/vk/repos/tmp/aws-4320"; |
Contributor
There was a problem hiding this comment.
Hard-coding a developer-specific path prevents the plugin from watching the system journal. Point it back to the real journald directory (e.g., /var/log/journal) or make it configurable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/crates/netdata-log-viewer/log-viewer-plugin/src/lib.rs, line 83:
<comment>Hard-coding a developer-specific path prevents the plugin from watching the system journal. Point it back to the real journald directory (e.g., /var/log/journal) or make it configurable.</comment>
<file context>
@@ -0,0 +1,108 @@
+ info!("Catalog function initialized");
+
+ // let path = "/var/log/journal";
+ let path = "/home/vk/repos/tmp/aws-4320";
+ match catalog_function.watch_directory(path) {
+ Ok(()) => {}
</file context>
Suggested change
| let path = "/home/vk/repos/tmp/aws-4320"; | |
| let path = "/var/log/journal"; |
| } | ||
|
|
||
| // Build config declaration | ||
| self.config_declaration = Some(config_props.build()); |
Contributor
There was a problem hiding this comment.
netdata_schema() now panics for types without x-config-* metadata because ConfigDeclarationBuilder::build unwraps every field even when none were provided. Ensure config declarations are only built when all required fields exist (or provide sane defaults) so deriving NetdataSchema on ordinary structs doesn’t crash.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/crates/netdata-plugin/schema/src/lib.rs, line 185:
<comment>`netdata_schema()` now panics for types without `x-config-*` metadata because `ConfigDeclarationBuilder::build` unwraps every field even when none were provided. Ensure config declarations are only built when all required fields exist (or provide sane defaults) so deriving `NetdataSchema` on ordinary structs doesn’t crash.</comment>
<file context>
@@ -0,0 +1,300 @@
+ }
+
+ // Build config declaration
+ self.config_declaration = Some(config_props.build());
+ }
+}
</file context>
| loop { | ||
| { | ||
| let array = journal_file.offset_array_ref(node.offset())?; | ||
| let remaining_items = node.remaining_items.get(); |
Contributor
There was a problem hiding this comment.
When copying subsequent arrays, use next_node.remaining_items instead of the previous node’s value; otherwise the last array is read past its valid entries.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/crates/journal-core/src/file/offset_array.rs, line 312:
<comment>When copying subsequent arrays, use `next_node.remaining_items` instead of the previous node’s value; otherwise the last array is read past its valid entries.</comment>
<file context>
@@ -294,10 +295,37 @@ impl List {
+ loop {
+ {
+ let array = journal_file.offset_array_ref(node.offset())?;
+ let remaining_items = node.remaining_items.get();
+ array.collect_offsets(0, remaining_items, offsets)?;
+ }
</file context>
Suggested change
| let remaining_items = node.remaining_items.get(); | |
| let remaining_items = next_node.remaining_items.get(); |
| IndexFilterExpr::None => false, | ||
| IndexFilterExpr::Match(bitmap) => !bitmap.is_empty(), | ||
| IndexFilterExpr::Conjunction(filter_exprs) => { | ||
| filter_exprs.iter().all(|expr| expr.has_matches()) |
Contributor
There was a problem hiding this comment.
has_matches returns true for conjunctions even when their intersection is empty, so AND filters can yield false positives.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/crates/journal-core/src/file/index_filter.rs, line 53:
<comment>`has_matches` returns true for conjunctions even when their intersection is empty, so AND filters can yield false positives.</comment>
<file context>
@@ -0,0 +1,369 @@
+ IndexFilterExpr::None => false,
+ IndexFilterExpr::Match(bitmap) => !bitmap.is_empty(),
+ IndexFilterExpr::Conjunction(filter_exprs) => {
+ filter_exprs.iter().all(|expr| expr.has_matches())
+ }
+ IndexFilterExpr::Disjunction(filter_exprs) => {
</file context>
| } | ||
|
|
||
| // Initialize result vector with capacity for efficiency | ||
| let mut collected_entries: Vec<LogEntryId> = Vec::with_capacity(limit); |
Contributor
There was a problem hiding this comment.
Vec::with_capacity(limit) tries to reserve usize::MAX elements whenever the caller leaves the limit unset, so the default path panics or OOMs before processing any logs. Use a bounded capacity (or start with an empty Vec) instead of allocating usize::MAX.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/crates/journal-engine/src/logs/query.rs, line 202:
<comment>`Vec::with_capacity(limit)` tries to reserve `usize::MAX` elements whenever the caller leaves the limit unset, so the default path panics or OOMs before processing any logs. Use a bounded capacity (or start with an empty Vec) instead of allocating `usize::MAX`.</comment>
<file context>
@@ -0,0 +1,414 @@
+ }
+
+ // Initialize result vector with capacity for efficiency
+ let mut collected_entries: Vec<LogEntryId> = Vec::with_capacity(limit);
+
+ for file_index in relevant_indexes {
</file context>
Suggested change
| let mut collected_entries: Vec<LogEntryId> = Vec::with_capacity(limit); | |
| let mut collected_entries: Vec<LogEntryId> = Vec::new(); |
| runtime.block_on(async_run(args)) | ||
| } | ||
|
|
||
| async fn async_run(_args: Vec<String>) -> i32 { |
Contributor
There was a problem hiding this comment.
run(args) accepts CLI arguments but async_run ignores them, so callers cannot configure the plugin via the provided argument vector despite the documented contract.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/crates/netdata-otel/otel-plugin/src/lib.rs, line 79:
<comment>`run(args)` accepts CLI arguments but `async_run` ignores them, so callers cannot configure the plugin via the provided argument vector despite the documented contract.</comment>
<file context>
@@ -0,0 +1,192 @@
+ runtime.block_on(async_run(args))
+}
+
+async fn async_run(_args: Vec<String>) -> i32 {
+ initialize_tracing();
+
</file context>
| } | ||
|
|
||
| pub fn step(&mut self, journal_file: &'a JournalFile<M>, direction: Direction) -> Result<bool> { | ||
| self.drop_guards(); |
Contributor
There was a problem hiding this comment.
field_data_restart never resets self.field_data_iterator, so calling it does not actually restart field data enumeration and later calls keep skipping previously returned objects.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/crates/journal-core/src/file/reader.rs, line 71:
<comment>`field_data_restart` never resets `self.field_data_iterator`, so calling it does not actually restart field data enumeration and later calls keep skipping previously returned objects.</comment>
<file context>
@@ -0,0 +1,466 @@
+ }
+
+ pub fn step(&mut self, journal_file: &'a JournalFile<M>, direction: Direction) -> Result<bool> {
+ self.drop_guards();
+
+ if let Some(filter) = self.filter.as_mut() {
</file context>
| docker ps | grep jaeger | ||
|
|
||
| # Check connectivity | ||
| nc -zv localhost 4317 |
Contributor
There was a problem hiding this comment.
The Jaeger connectivity check probes port 4317 even though the Quick Start container mapping exposes Jaeger on host port 4318, so the documented troubleshooting command will always fail.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/crates/netdata-log-viewer/README.md, line 242:
<comment>The Jaeger connectivity check probes port 4317 even though the Quick Start container mapping exposes Jaeger on host port 4318, so the documented troubleshooting command will always fail.</comment>
<file context>
@@ -0,0 +1,258 @@
+docker ps | grep jaeger
+
+# Check connectivity
+nc -zv localhost 4317
+```
+
</file context>
Suggested change
| nc -zv localhost 4317 | |
| +nc -zv localhost 4318 |
| transaction, | ||
| status: 400, | ||
| expires: 0, | ||
| format: "text/plain".to_string(), |
Contributor
There was a problem hiding this comment.
The 400 response emitted for an empty payload is JSON but format is set to "text/plain", so the content type is wrong. Use application/json to match the actual payload.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/crates/netdata-plugin/rt/src/lib.rs, line 334:
<comment>The 400 response emitted for an empty payload is JSON but `format` is set to `"text/plain"`, so the content type is wrong. Use `application/json` to match the actual payload.</comment>
<file context>
@@ -0,0 +1,1015 @@
+ transaction,
+ status: 400,
+ expires: 0,
+ format: "text/plain".to_string(),
+ payload: format!("Invalid request: {}", e).as_bytes().to_vec(),
+ };
</file context>
Suggested change
| format: "text/plain".to_string(), | |
| format: "application/json".to_string(), |
| _ => return raw_value.to_string(), | ||
| }; | ||
|
|
||
| description.to_string() |
Contributor
There was a problem hiding this comment.
MessageIdTransformation drops the actual MESSAGE_ID and returns only the description, contradicting the documented "UUID (description)" output.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/crates/netdata-log-viewer/journal-function/src/netdata/transformations.rs, line 580:
<comment>`MessageIdTransformation` drops the actual MESSAGE_ID and returns only the description, contradicting the documented `"UUID (description)"` output.</comment>
<file context>
@@ -0,0 +1,613 @@
+ _ => return raw_value.to_string(),
+ };
+
+ description.to_string()
+ }
+}
</file context>
Suggested change
| description.to_string() | |
| format!("{} ({})", raw_value, description) |
github-actions
bot
added
the
area/packaging
Ferroin
reviewed
Dec 5, 2025
netdata.spec.in
Outdated
| %caps(cap_dac_read_search=ep) %attr(0750,root,netdata) %{_libexecdir}/%{name}/plugins.d/systemd-journal.plugin | ||
| # CAP_DAC_READ_SEARCH required for reading journal files. | ||
| %caps(cap_dac_read_search=ep) %attr(0750,root,netdata) %{_libexecdir}/%{name}/plugins.d/journal-viewer-plugin | ||
| %config(noreplace) %{_libdir}/%{name}/conf.d/journal-viewer.yml |
Member
There was a problem hiding this comment.
This shouldn’t be flagged as a config file, because it’s not exepcted to be edited by the user.
This significnatly shrinks the Docker build cache size and also improves overall disk space requirements during the build process.
Member
|
Static build failures will need a rebase to include #21401. |
Contributor
There was a problem hiding this comment.
9 issues found across 244 files (changes from recent commits).
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="docs/ai-powered-alert-configuration/ai-powered-alert-configuration.md">
<violation number="1">
P3: Typo: "masterfull" should be "masterful" (single 'l').</violation>
</file>
<file name="src/crates/journal-index/src/file_index.rs">
<violation number="1" location="src/crates/journal-index/src/file_index.rs:97">
P2: Potential integer underflow: if `indexed_at` is in the future relative to `now` (e.g., due to clock skew), the subtraction will panic in debug mode or wrap around in release mode. Consider using `saturating_sub` to handle this edge case gracefully.</violation>
</file>
<file name="packaging/PLATFORM_SUPPORT.md">
<violation number="1">
P2: Incorrect EOL date for Ubuntu 24.10. The EOL date '2024-07-01' predates Ubuntu 24.10's release (October 2024). Based on Ubuntu's 9-month non-LTS support cycle, the EOL should be around July 2025.</violation>
</file>
<file name="src/crates/journal-engine/examples/index.rs">
<violation number="1" location="src/crates/journal-engine/examples/index.rs:58">
P2: Hardcoded developer-specific path in example code. Consider using a more generic placeholder path like `PathBuf::from("./test-data")` or requiring the command-line argument instead of defaulting to a user-specific path.</violation>
</file>
<file name="src/collectors/windows.plugin/perflib-adcs.c">
<violation number="1">
P1: Data source mismatch: The function fetches `ADCSRetrievalsProcessingTime` via `perflibGetObjectCounter` but this line reads from `ADCSRequestCryptoSigningTime` which is never populated. This will report uninitialized data. The `perflibGetObjectCounter` call should also be changed to `&ac->ADCSRequestCryptoSigningTime`.</violation>
</file>
<file name="src/collectors/windows.plugin/GetHardwareInfo.c">
<violation number="1">
P2: Incomplete error handling for `ExpandEnvironmentStringsA`. The function returns the required buffer size (> 0) when the buffer is too small, not 0. The current check only catches complete failures, potentially allowing truncated/undefined data when the expanded path exceeds `MAX_PATH`.</violation>
</file>
<file name="src/collectors/windows.plugin/GetSensors.c">
<violation number="1">
P2: Resource leak: `PropVariantClear(&pv)` is not called before returning `NULL` when `wcstombs` fails. The PROPVARIANT contains allocated memory (`VT_LPWSTR`) that will leak.</violation>
</file>
<file name="src/crates/journal-engine/src/indexing.rs">
<violation number="1" location="src/crates/journal-engine/src/indexing.rs:208">
P1: Cache lookup errors silently drop keys from processing. When cache lookup fails, the key should be added to `keys_to_compute` to allow recomputation, otherwise the result set will be incomplete without the caller knowing which keys were lost.</violation>
<violation number="2" location="src/crates/journal-engine/src/indexing.rs:276">
P2: Computation errors are silently dropped in Phase 4. When file indexing fails, there's no logging and the key is lost from the response. Consider logging the error to aid debugging.</violation>
</file>
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
| pub fn is_fresh(&self) -> bool { | ||
| if self.was_online { | ||
| let now = Seconds::now(); | ||
| let age = now.get() - self.indexed_at.get(); |
Contributor
There was a problem hiding this comment.
P2: Potential integer underflow: if indexed_at is in the future relative to now (e.g., due to clock skew), the subtraction will panic in debug mode or wrap around in release mode. Consider using saturating_sub to handle this edge case gracefully.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/crates/journal-index/src/file_index.rs, line 97:
<comment>Potential integer underflow: if `indexed_at` is in the future relative to `now` (e.g., due to clock skew), the subtraction will panic in debug mode or wrap around in release mode. Consider using `saturating_sub` to handle this edge case gracefully.</comment>
<file context>
@@ -87,6 +86,22 @@ impl FileIndex {
+ pub fn is_fresh(&self) -> bool {
+ if self.was_online {
+ let now = Seconds::now();
+ let age = now.get() - self.indexed_at.get();
+ age < 1
+ } else {
</file context>
Suggested change
| let age = now.get() - self.indexed_at.get(); | |
| let age = now.get().saturating_sub(self.indexed_at.get()); |
| PathBuf::from(arg) | ||
| } else { | ||
| // PathBuf::from("/mnt/slow-disk/otel-aws") | ||
| PathBuf::from("/home/vk/repos/tmp/otel-aws") |
Contributor
There was a problem hiding this comment.
P2: Hardcoded developer-specific path in example code. Consider using a more generic placeholder path like PathBuf::from("./test-data") or requiring the command-line argument instead of defaulting to a user-specific path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/crates/journal-engine/examples/index.rs, line 58:
<comment>Hardcoded developer-specific path in example code. Consider using a more generic placeholder path like `PathBuf::from("./test-data")` or requiring the command-line argument instead of defaulting to a user-specific path.</comment>
<file context>
@@ -0,0 +1,135 @@
+ PathBuf::from(arg)
+ } else {
+ // PathBuf::from("/mnt/slow-disk/otel-aws")
+ PathBuf::from("/home/vk/repos/tmp/otel-aws")
+ };
+
</file context>
| // Phase 4: Update registry and cache, then collect responses | ||
| for (key, response) in computed_results { | ||
| // Update registry and cache on success | ||
| if let Ok(index) = response { |
Contributor
There was a problem hiding this comment.
P2: Computation errors are silently dropped in Phase 4. When file indexing fails, there's no logging and the key is lost from the response. Consider logging the error to aid debugging.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/crates/journal-engine/src/indexing.rs, line 276:
<comment>Computation errors are silently dropped in Phase 4. When file indexing fails, there's no logging and the key is lost from the response. Consider logging the error to aid debugging.</comment>
<file context>
@@ -339,187 +109,183 @@ impl IndexingEngineBuilder {
+ // Phase 4: Update registry and cache, then collect responses
+ for (key, response) in computed_results {
+ // Update registry and cache on success
+ if let Ok(index) = response {
+ let _ = registry.update_time_range(
+ &key.file,
</file context>
Comment on lines
+208
to
+210
| Err(e) => { | ||
| error!("cached file index lookup error {}", e); | ||
| } |
Contributor
There was a problem hiding this comment.
P1: Cache lookup errors silently drop keys from processing. When cache lookup fails, the key should be added to keys_to_compute to allow recomputation, otherwise the result set will be incomplete without the caller knowing which keys were lost.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/crates/journal-engine/src/indexing.rs, line 208:
<comment>Cache lookup errors silently drop keys from processing. When cache lookup fails, the key should be added to `keys_to_compute` to allow recomputation, otherwise the result set will be incomplete without the caller knowing which keys were lost.</comment>
<file context>
@@ -339,187 +109,183 @@ impl IndexingEngineBuilder {
+ cache_misses += 1;
+ keys_to_compute.push(key);
+ }
+ Err(e) => {
+ error!("cached file index lookup error {}", e);
+ }
</file context>
Suggested change
| Err(e) => { | |
| error!("cached file index lookup error {}", e); | |
| } | |
| Err(e) => { | |
| // Cache error - need to compute | |
| error!("cached file index lookup error {}", e); | |
| keys_to_compute.push(key); | |
| } |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
journal-viewerplugin replacing the existingsystemd-journalplugin.otelplugin to ingest Otel logs in a systemd-compatible way.