remove fluent bit, use otel-collector for ingestion scripts - #1745
Merged
nikhilsinhaparseable merged 3 commits intoAug 9, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughBoth ingestion scripts replace Fluent Bit with the OpenTelemetry Collector. They add pinned, checksum-verified installation, Collector lifecycle management, host-metrics YAML generation, OTLP/HTTP JSON export, configuration validation, and updated command dispatch. ChangesOpenTelemetry Collector ingestion
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SetupScript
participant OpenTelemetryCollector
participant Parseable
SetupScript->>SetupScript: parse endpoint and generate host-metrics YAML
SetupScript->>OpenTelemetryCollector: validate and start configuration
OpenTelemetryCollector->>Parseable: export metrics through OTLP/HTTP JSON
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
scripts/ingest.sh (2)
189-189: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd timeout and retry options to the download.
curl -fsSLhas no time limit. If the connection stalls, the setup command hangs with no output. Add bounded retries and a maximum time.♻️ Proposed refactor
- curl -fsSL "$download_url" -o "$archive_path" + if ! curl -fsSL --retry 3 --retry-delay 2 --connect-timeout 10 --max-time 300 \ + "$download_url" -o "$archive_path"; then + print_error "Failed to download OpenTelemetry Collector from $download_url" + rm -rf "$temp_dir" + exit 1 + fi🤖 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 `@scripts/ingest.sh` at line 189, Update the curl download in the ingest script to enforce a maximum request time and bounded retries, while preserving the existing fail-silently and redirect-following behavior of curl -fsSL.
63-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the process match from
$CONFIG_FILE.The
casepattern hardcodesotelcol.yaml. IfCONFIG_FILEchanges,is_runningreturns false for a live collector, andstart_collectorlaunches a second process against the same PID file. Build the pattern from the variable.♻️ Proposed refactor
is_running() { local process_command + local config_base + config_base=$(basename "$CONFIG_FILE") if [ -f "$PID_FILE" ]; then PID=$(cat "$PID_FILE") if [[ "$PID" =~ ^[0-9]+$ ]] && ps -p "$PID" > /dev/null 2>&1; then process_command=$(ps -p "$PID" -o command= 2>/dev/null || true) case "$process_command" in - *otelcol*otelcol.yaml*) return 0 ;; + *otelcol*"$config_base"*) return 0 ;; esac fi fi return 1 }🤖 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 `@scripts/ingest.sh` around lines 63 - 76, Update is_running to derive the process_command case pattern from CONFIG_FILE instead of hardcoding otelcol.yaml. Ensure the pattern matches the collector command using the configured file path or filename so live processes remain detected when CONFIG_FILE changes.
🤖 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 `@scripts/ingest.ps1`:
- Around line 370-402: Align the generated YAML in the PowerShell configuration
block with scripts/ingest.sh by using the same collection interval and scraper
set, including the processes scraper. Update the OTLP exporter settings to use
the selected longer interval and enable gzip compression instead of uncompressed
JSON, while preserving the existing endpoint and headers.
- Around line 84-95: Update Get-Architecture to read PROCESSOR_ARCHITEW6432
first and use it when present, falling back to PROCESSOR_ARCHITECTURE otherwise.
Preserve the existing AMD64 and ARM64 returns and unsupported-architecture error
handling.
- Around line 422-424: After writing $CONFIG_FILE in the
configuration-generation flow, restrict its permissions to the current owner
only, equivalent to ingest.sh’s chmod 600. Use the existing PowerShell file path
and ACL APIs to remove inherited permissions and grant read/write access solely
to the owner, while preserving the UTF-8 no-BOM write behavior.
In `@scripts/ingest.sh`:
- Line 347: Update the configuration-file creation flow around CONFIG_FILE so
the file is created with mode 600 before the here-document writes the API key.
Apply the permission change before the cat redirection, and retain the existing
final permission handling as needed.
---
Nitpick comments:
In `@scripts/ingest.sh`:
- Line 189: Update the curl download in the ingest script to enforce a maximum
request time and bounded retries, while preserving the existing fail-silently
and redirect-following behavior of curl -fsSL.
- Around line 63-76: Update is_running to derive the process_command case
pattern from CONFIG_FILE instead of hardcoding otelcol.yaml. Ensure the pattern
matches the collector command using the configured file path or filename so live
processes remain detected when CONFIG_FILE changes.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fbf0b932-e763-44d9-9ccb-ec5c3adf1bb8
📒 Files selected for processing (2)
scripts/ingest.ps1scripts/ingest.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/ingest.sh (2)
198-228: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClean up installer temporary files on every failure path.
The script uses
set -e. If extraction or replacement fails, execution exits before the finalrm -rf "$temp_dir". Repeated failed installations can leave archives, extracted binaries, and$COLLECTOR_BIN.newin temporary storage. Add a cleanup trap after creatingtemp_dir, and clear it only after a successful replacement. (raw.githubusercontent.com)🤖 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 `@scripts/ingest.sh` around lines 198 - 228, Add a cleanup trap immediately after creating temp_dir so every subsequent failure removes the temporary directory and any COLLECTOR_BIN.new replacement file. Clear the trap only after the archive is successfully replaced, preserving cleanup for extraction, copy, chmod, and mv failures in the installer flow.Source: MCP tools
312-317: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle bracketed IPv6 hosts before generic colon parsing.
For
https://[::1], the current branch treats the final colon as a port separator. It produces an invalid host and port instead of using the default port. Parse bracketed IPv6 separately, or reject IPv6 explicitly in the documented input format.🤖 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 `@scripts/ingest.sh` around lines 312 - 317, Update the host/port parsing logic around ingestor_host so bracketed IPv6 values such as [::1] are handled before the generic colon check, preserving the bracketed host and assigning default_port when no port is provided. Keep explicit host:port parsing for supported non-bracketed inputs unchanged, or reject IPv6 clearly if the documented format does not support it.
🤖 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 `@scripts/ingest.sh`:
- Around line 355-357: Update the configuration-writing flow in the ingest
script to write the generated content to a mode-600 temporary file instead of
truncating CONFIG_FILE. Validate the temporary file, and only after successful
validation atomically rename it over CONFIG_FILE; preserve the existing
CONFIG_FILE unchanged when writing, validation, or interruption fails, including
the corresponding flow around the additional CONFIG_FILE write.
---
Outside diff comments:
In `@scripts/ingest.sh`:
- Around line 198-228: Add a cleanup trap immediately after creating temp_dir so
every subsequent failure removes the temporary directory and any
COLLECTOR_BIN.new replacement file. Clear the trap only after the archive is
successfully replaced, preserving cleanup for extraction, copy, chmod, and mv
failures in the installer flow.
- Around line 312-317: Update the host/port parsing logic around ingestor_host
so bracketed IPv6 values such as [::1] are handled before the generic colon
check, preserving the bracketed host and assigning default_port when no port is
provided. Keep explicit host:port parsing for supported non-bracketed inputs
unchanged, or reject IPv6 clearly if the documented format does not support it.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e5565716-d648-4824-a26f-10116948b79e
📒 Files selected for processing (2)
scripts/ingest.ps1scripts/ingest.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/ingest.ps1
praveen5959
approved these changes
Aug 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit