fix(helm): Enable log_ingestor by default (fixes #1912); gate log_ingestor templates to S3 logs_input only.#1913
Conversation
…e `log_ingestor` templates to S3 `logs_input` only.
WalkthroughThis pull request increments the Helm chart version, tightens rendering conditions for log-ingestor resources to require both Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tools/deployment/package-helm/values.yaml`:
- Line 166: The inline comment on the buffer_flush_threshold setting currently
has only one space before the comment; update the line defining
buffer_flush_threshold so there are exactly two spaces before the inline comment
(e.g., ensure "buffer_flush_threshold: 268435456 # 256 MiB") to satisfy
linting; edit the value line in values.yaml where buffer_flush_threshold is
declared.
hoophalab
left a comment
There was a problem hiding this comment.
LGTM. One comment
Validation: log ingestor starts correctly
| log_ingestor: | ||
| port: 30302 | ||
| # The timeout (in seconds) after which the log buffer is flushed for compression if no new input | ||
| # arrives. | ||
| buffer_flush_timeout: 300 | ||
| # The log buffer size (in bytes) that triggers a flush for compression. | ||
| buffer_flush_threshold: 268435456 # 256 MiB | ||
| # The capacity of the internal channel used for communication between an ingestion job and the | ||
| # log buffer. | ||
| channel_capacity: 10 | ||
| logging_level: "INFO" |
There was a problem hiding this comment.
Setting up log_ingestor and setting log_inputs as s3 might cause confusion. Maybe we should still keep those values as commented?
There was a problem hiding this comment.
if we want to change the default config of the log ingestor in the Helm chart, it might be better to change the config in the package (docker compose flow) as well.
i think we can add some log message in the Helm chart with some printf template function. do you think it will help with the confusion that a user has configured the log ingestor but didn't end up seeing any log ingestor deployment?
either way, i think we can merge this PR first and add more such log messages in a future PR. @coderabbitai please create a feature request issue about enhancing the logging in the Helm chart with printf and fail template functions
There was a problem hiding this comment.
🧩 Analysis chain
@junhaoliao, I'll create a feature request issue for enhancing the Helm chart logging.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for Helm chart logging enhancement
gh issue create \
--repo y-scope/clp \
--title "Enhance Helm chart with template logging using printf and fail functions" \
--body "## Description
Add template logging and validation messages to the Helm chart using \`printf\` and \`fail\` template functions to provide better user feedback and prevent configuration confusion.
## Context
Currently, when a user configures \`log_ingestor\` in the Helm chart but sets \`logs_input.type\` to something other than \"s3\", no log-ingestor deployment is created. This can cause confusion as users may expect to see the log-ingestor pods running.
## Proposed Enhancement
Add informative messages using Helm template functions:
- Use \`printf\` to log warnings or informational messages when certain configurations are set but their corresponding resources won't be deployed
- Use \`fail\` to validate incompatible configurations and provide clear error messages
Example scenarios:
- Warn when \`log_ingestor\` is configured but \`logs_input.type\` is not \"s3\"
- Validate other configuration dependencies and provide helpful error messages
## Related Discussion
- PR: https://github.com/y-scope/clp/pull/1913
- Comment: https://github.com/y-scope/clp/pull/1913#discussion_r2739010265
- Requested by: `@junhaoliao`
## Benefits
- Improved user experience with clearer feedback
- Reduced confusion about why certain resources aren't deployed
- Better configuration validation at deployment time" \
--assignee junhaoliao
Length of output: 42
@junhaoliao, I've created issue #1941 to track the enhancement of Helm chart logging with printf and fail template functions. The issue includes the context from this discussion and has been assigned to you.
🧠 Learnings used
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1856
File: tools/deployment/package-helm/values.yaml:94-94
Timestamp: 2026-01-26T22:19:16.092Z
Learning: In the Helm chart's `values.yaml`, the default configuration has `log_ingestor: null`, which causes the v0.8.0 chart to deploy an API server that continuously crashes and restarts due to Rust parsing issues.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1818
File: tools/deployment/package-helm/templates/configmap.yaml:12-12
Timestamp: 2025-12-19T05:03:42.629Z
Learning: In the y-scope/clp Helm chart, the `api_server.host` configuration field in the ConfigMap (tools/deployment/package-helm/templates/configmap.yaml) is unused because no other k8s internal services need to reach the API server—it's only accessed from outside the cluster via NodePort.
…e `log_ingestor` templates to S3 `logs_input` only. (y-scope#1913)
Description
Note
This PR is part of the ongoing work for #1309. More PRs will be submitted to complete the Helm chart.
Enables default
clpConfig.log_ingestorvalues in Helmvalues.yamlso the chart aligns with thedefault CLP configs and avoids the API crash loop when
log_ingestoris null. Log-ingestor Helmresources now render only when
logs_input.typeiss3.This PR updates:
clpConfig.log_ingestorvalues intools/deployment/package-helm/values.yaml.logs_input.type == "s3".Checklist
breaking change.
Validation performed
Test with fs input
Observed:
Test with S3 input
Updated
tools/deployment/package-helm/values.yamlto set:Ran:
Observed:
Ran:
Observed log-ingestor logs:
Summary by CodeRabbit
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.