POC: use OTELcol instead of vector#3334
Conversation
📝 WalkthroughWalkthroughAdds initial dual-collector support to CLO, allowing selection between Vector and an OpenTelemetry (OTEL) collector via a new annotation. Introduces OTEL constants, factory/config wiring, pod visitor helpers, a data-path utility, functional test framework support, Makefile image plumbing, and migration design documents. ChangesOTEL Collector Migration Support
Estimated code review effort: 3 (Moderate) | ~25 minutes Related PRs: None identified. Suggested labels: enhancement, documentation, area/collector Suggested reviewers: None identified. 🐰 A hop from Vector's steady track, 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
midstream/Dockerfile
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pavolloffay The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
pavolloffay
force-pushed
the
otelcol-switch
branch
from
559f60a to
b9df853
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
239-246: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSet
COLLECTOR_TYPEin the functional test targets The framework switches OTEL vs Vector fromCOLLECTOR_TYPE, buttest-functionalonly forwardsRELATED_IMAGE_OTEL_COLLECTOR. That makes the OTEL path unreachable frommakeunless callers export the env var manually.🤖 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 `@Makefile` around lines 239 - 246, The functional test target is not forwarding the collector selection variable, so the OTEL path cannot be reached from make. Update the test-functional target to pass COLLECTOR_TYPE alongside the existing RELATED_IMAGE_* env vars, using the same pattern as test-functional-benchmarker-vector, so the framework can switch between OTEL and Vector without manual exports.
🧹 Nitpick comments (3)
internal/constants/annotations.go (1)
22-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotation domain inconsistent with sibling constants.
AnnotationSecretHashandAnnotationConfigMapHashin this same file use theobservability.openshift.io/domain, but this new annotation useslogging.openshift.io/. Confirm this domain choice is intentional (e.g., deliberately separate from the observability API group) rather than a copy-paste inconsistency.🤖 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 `@internal/constants/annotations.go` around lines 22 - 24, The new AnnotationCollectorType key uses a different domain than the other annotation constants in this file, so verify that logging.openshift.io is intentional. If this annotation should follow the same API-group naming pattern as AnnotationSecretHash and AnnotationConfigMapHash, update the constant in the annotations constants block to use the consistent observability.openshift.io domain; otherwise keep it as-is only if the separate domain is deliberate.internal/collector/otel/utils.go (1)
9-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
GetDataPathlogic withvector/utils.go.This mirrors
vector.GetDataPathalmost exactly (same singleton-namespace check, samepath.Joinfallback), differing only in the base path constant. Consider extracting a shared helper parameterized by base path to avoid two copies drifting apart.♻️ Example consolidation
// internal/collector/common/paths.go func GetDataPath(basePath, namespace, forwarderName string) string { if constants.OpenshiftNS == namespace && constants.SingletonName == forwarderName { return basePath } return path.Join(basePath, namespace, forwarderName) }🤖 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 `@internal/collector/otel/utils.go` around lines 9 - 20, `GetDataPath` in `internal/collector/otel/utils.go` duplicates the same namespace/singleton path logic used in `vector.GetDataPath`, so consolidate the shared behavior into one helper. Extract a common path helper (for example in a shared `paths` utility) that accepts the base path plus `namespace` and `forwarderName`, then update both `GetDataPath` implementations to delegate to it while keeping their respective base path constants. Use the existing `constants.OpenshiftNS` and `constants.SingletonName` checks in the shared helper so the singleton-case behavior stays identical.Makefile (1)
239-246: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick wincheckmake: recipe body exceeds
maxbodylength.Static analysis flags
test-functional(line 240) andtest-e2e(line 333) for exceeding the configured max recipe body length after adding theRELATED_IMAGE_OTEL_COLLECTORline.Also applies to: 332-339
🤖 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 `@Makefile` around lines 239 - 246, The `test-functional` and `test-e2e` Makefile recipes now exceed checkmake’s `maxbodylength` because of the added `RELATED_IMAGE_OTEL_COLLECTOR` environment line. Refactor the recipes by moving the shared environment variable setup into a reusable Makefile variable or helper target, then reference that from `test-functional` and `test-e2e` so their recipe bodies stay short while preserving the `go test` invocations.Source: Linters/SAST tools
🤖 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 `@internal/controller/observability/collector.go`:
- Around line 199-232: The OTEL collector self-exclusion is hardcoded to
openshift-logging, so collector pods in other namespaces are not excluded.
Update generateOTELConfig to derive the exclude path from the
ClusterLogForwarder spec’s namespace (the namespace used by the
forwarder/collector) instead of a fixed string, and wire that value into the
returned config so the collector excludes its own pod logs regardless of
deployment namespace.
In `@opentelemetry-collector-migration.md`:
- Around line 116-123: The compatibility table has a header/body column
mismatch: the rows in the template usage table contain an extra trailing notes
cell that is not represented in the header. Update the table in
opentelemetry-collector-migration.md so the header matches the row structure,
either by adding a Notes column or by merging the note text into the Status
cell, and keep the existing entries for Elasticsearch, Kafka, CloudWatch, S3,
Splunk, and Syslog consistent.
- Line 35: The HTTP receiver row is pointing to the wrong OpenTelemetry
component; OpenTelemetry Collector Contrib does not provide an `httpreceiver`
for generic JSON ingestion. Update the migration table entry that currently
references `httpreceiver` to use `webhookeventreceiver` instead, since that is
the appropriate receiver for arbitrary HTTP/webhook JSON input in this context.
In `@test/framework/functional/otel/deploy.go`:
- Around line 66-69: The regexp in OTELCollector.ModifyConfig is using the U
flag, which makes the non-greedy journalctl block match too broad and can
consume more than intended. Update the regex used in ModifyConfig so it matches
only the journalctl section up to the first blank line, and keep the replacement
localized to that block while preserving the rest of the config.
---
Outside diff comments:
In `@Makefile`:
- Around line 239-246: The functional test target is not forwarding the
collector selection variable, so the OTEL path cannot be reached from make.
Update the test-functional target to pass COLLECTOR_TYPE alongside the existing
RELATED_IMAGE_* env vars, using the same pattern as
test-functional-benchmarker-vector, so the framework can switch between OTEL and
Vector without manual exports.
---
Nitpick comments:
In `@internal/collector/otel/utils.go`:
- Around line 9-20: `GetDataPath` in `internal/collector/otel/utils.go`
duplicates the same namespace/singleton path logic used in `vector.GetDataPath`,
so consolidate the shared behavior into one helper. Extract a common path helper
(for example in a shared `paths` utility) that accepts the base path plus
`namespace` and `forwarderName`, then update both `GetDataPath` implementations
to delegate to it while keeping their respective base path constants. Use the
existing `constants.OpenshiftNS` and `constants.SingletonName` checks in the
shared helper so the singleton-case behavior stays identical.
In `@internal/constants/annotations.go`:
- Around line 22-24: The new AnnotationCollectorType key uses a different domain
than the other annotation constants in this file, so verify that
logging.openshift.io is intentional. If this annotation should follow the same
API-group naming pattern as AnnotationSecretHash and AnnotationConfigMapHash,
update the constant in the annotations constants block to use the consistent
observability.openshift.io domain; otherwise keep it as-is only if the separate
domain is deliberate.
In `@Makefile`:
- Around line 239-246: The `test-functional` and `test-e2e` Makefile recipes now
exceed checkmake’s `maxbodylength` because of the added
`RELATED_IMAGE_OTEL_COLLECTOR` environment line. Refactor the recipes by moving
the shared environment variable setup into a reusable Makefile variable or
helper target, then reference that from `test-functional` and `test-e2e` so
their recipe bodies stay short while preserving the `go test` invocations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b639248f-4f41-473a-a5f6-7bf76e4b929c
📒 Files selected for processing (14)
.claude/projects/-home-ploffay-projects-openshift-cluster-logging-operator/memory/MEMORY.md.claude/projects/-home-ploffay-projects-openshift-cluster-logging-operator/memory/project_clo_otel_migration.mdMakefileinternal/collector/collector.gointernal/collector/config.gointernal/collector/otel/utils.gointernal/collector/otel/visitors.gointernal/constants/annotations.gointernal/constants/constants.gointernal/controller/observability/collector.gointernal/utils/utils.goopentelemetry-collector-migration.mdtest/framework/functional/framework.gotest/framework/functional/otel/deploy.go
| // generateOTELConfig returns a minimal OTEL collector configuration. | ||
| // TODO: Replace with full config generation from CLF spec. | ||
| func generateOTELConfig(_ obs.ClusterLogForwarderSpec) (string, error) { | ||
| return `receivers: | ||
| filelog: | ||
| include: | ||
| - /var/log/pods/*/*/*.log | ||
| exclude: | ||
| - /var/log/pods/openshift-logging_collector-*/*/*.log | ||
| start_at: end | ||
| include_file_path: true | ||
| include_file_name: false | ||
| operators: | ||
| - type: container | ||
| id: container-parser | ||
|
|
||
| processors: | ||
| batch: | ||
| send_batch_size: 8192 | ||
| timeout: 2s | ||
|
|
||
| exporters: | ||
| debug: | ||
| verbosity: basic | ||
|
|
||
| service: | ||
| pipelines: | ||
| logs: | ||
| receivers: [filelog] | ||
| processors: [batch] | ||
| exporters: [debug] | ||
| `, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map relevant files and inspect the collector config generation plus the functional test namespace setup.
git ls-files | rg '^(internal/controller/observability/collector\.go|test/framework/functional/framework\.go|test/framework/functional/otel/deploy\.go|test/framework/functional/.*\.go)$'
echo '--- collector.go outline ---'
ast-grep outline internal/controller/observability/collector.go --view expanded || true
echo '--- framework.go outline ---'
ast-grep outline test/framework/functional/framework.go --view expanded || true
echo '--- otel deploy.go outline ---'
ast-grep outline test/framework/functional/otel/deploy.go --view expanded || true
echo '--- search for collector namespace/exclude/start_at references ---'
rg -n --context 3 'openshift-logging_collector-|start_at: end|generateOTELConfig|ClusterLogForwarderSpec|Debug exporter|collector' internal/controller/observability/collector.go test/framework/functional test/framework -g '!**/vendor/**'Repository: openshift/cluster-logging-operator
Length of output: 50390
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the smallest relevant slices around the config generator and functional namespace creation.
sed -n '180,260p' internal/controller/observability/collector.go
echo '--- framework.go relevant slice ---'
rg -n 'type .*Namespace|t\.NS|Namespace|CreateNamespace|openshift-logging' test/framework/functional/framework.go -A 4 -B 4
echo '--- otel deploy.go relevant slice ---'
sed -n '1,240p' test/framework/functional/otel/deploy.goRepository: openshift/cluster-logging-operator
Length of output: 10626
Make the self-exclusion namespace dynamic. The exclude path only matches openshift-logging, but the functional framework deploys the collector in t.NS.Name, so its own pod logs won’t be excluded there. Derive this from the forwarder namespace instead of hardcoding it.
🤖 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 `@internal/controller/observability/collector.go` around lines 199 - 232, The
OTEL collector self-exclusion is hardcoded to openshift-logging, so collector
pods in other namespaces are not excluded. Update generateOTELConfig to derive
the exclude path from the ClusterLogForwarder spec’s namespace (the namespace
used by the forwarder/collector) instead of a fixed string, and wire that value
into the returned config so the collector excludes its own pod logs regardless
of deployment namespace.
| | `infrastructure` (container logs) | `kubernetes_logs` (filtered) | `filelog` + `k8sattributes` processor | ✅ Supported | Same as application but filtered to `openshift-*`, `kube-*`, `default` namespaces via include glob patterns. | | ||
| | `infrastructure` (node/journal) | `journald` | `journalctl` receiver | ✅ Supported | OTEL contrib `journalctl` receiver reads systemd journal. Supports directory config, unit filtering. | | ||
| | `audit` (auditd, kube-api, openshift-api, ovn) | `file` (multiple paths) | `filelog` (multiple instances) | ✅ Supported | `filelog` receiver watches specific file paths (`/var/log/audit/audit.log`, `/var/log/kube-apiserver/audit.log`, etc.). Supports `start_at`, `max_log_size`, glob patterns. | | ||
| | `receiver` (HTTP) | `http_server` (JSON decoding) | `httpreceiver` (contrib) | ⚠️ Needs verification | CLO's HTTP receiver accepts arbitrary JSON (specifically `kubeAPIAudit` format). The OTEL `httpreceiver` (contrib) or a webhook receiver would work. Alternatively, could use `otlpreceiver` if clients send OTLP. | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n "httpreceiver|http_server|Needs verification|kubeAPIAudit|otlpreceiver" opentelemetry-collector-migration.md .Repository: openshift/cluster-logging-operator
Length of output: 10410
🌐 Web query:
OpenTelemetry Collector contrib HTTP receiver component name httpreceiver webhook receiver
💡 Result:
In the OpenTelemetry Collector Contrib repository, there is no official component named "httpreceiver." The confusion often arises from the following components: 1. Webhook Event Receiver: The official component for receiving generic webhook-style data is named "webhookeventreceiver" [1][2]. It is designed to ingest generic events as logs into the OpenTelemetry Collector [1][3]. 2. HTTP Check Receiver: There is an official component named "httpcheckreceiver" (which replaced the older "httpcheck" alias) [4]. This component is used for synthetic checks against HTTP endpoints and is not a general-purpose HTTP log or event receiver [4][5][6]. Some third-party distributions or custom implementations have used the name "httpreceiver" [7][8][9]. Notably, discussions in community pull requests have highlighted that "httpreceiver" is often considered a confusing or generic name because it can be mistaken for the "httpcheckreceiver" [8]. It is recommended to use the official "webhookeventreceiver" if you require a generic HTTP/webhook-based ingestion component [1][10].
Citations:
- 1: https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/receiver/webhookeventreceiver
- 2: https://pkg.go.dev/github.com/open-telemetry/opentelemetry-collector-contrib/receiver/webhookeventreceiver
- 3: https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/v0.148.0/receiver/webhookeventreceiver
- 4: https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/receiver/httpcheckreceiver
- 5: https://pkg.go.dev/github.com/open-telemetry/opentelemetry-collector-contrib/receiver/httpcheckreceiver
- 6: https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/v0.147.0/receiver/httpcheckreceiver
- 7: https://github.com/observIQ/bindplane-otel-contrib
- 8: feat: inital version of httpreceiver SigNoz/signoz-otel-collector#193
- 9: https://pkg.go.dev/github.com/observiq/bindplane-otel-collector
- 10: New component: Generic Webhook Receiver open-telemetry/opentelemetry-collector-contrib#18101
Use webhookeventreceiver here. OpenTelemetry Collector Contrib doesn’t ship an httpreceiver; for generic HTTP/webhook JSON ingestion, point this row at webhookeventreceiver instead.
🤖 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 `@opentelemetry-collector-migration.md` at line 35, The HTTP receiver row is
pointing to the wrong OpenTelemetry component; OpenTelemetry Collector Contrib
does not provide an `httpreceiver` for generic JSON ingestion. Update the
migration table entry that currently references `httpreceiver` to use
`webhookeventreceiver` instead, since that is the appropriate receiver for
arbitrary HTTP/webhook JSON input in this context.
| | Template Usage | Vector | OTEL | Status | | ||
| |---------------|--------|------|--------| | ||
| | Elasticsearch index name | `{{ field }}` in index template | `elasticsearch.index` attribute on log records | ✅ Supported | Since [v0.122.0](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/exporter/elasticsearchexporter), dynamic routing via `elasticsearch.index` attribute is the default. Use `transform` processor to set the attribute per-record based on log fields. | | ||
| | Kafka topic name | `{{ field }}` in topic template | Kafka exporter `topic_from_attribute` | ✅ Supported | Kafka exporter can derive topic from log attributes. | | ||
| | CloudWatch log group / stream | `{{ field }}` templates | Exporter config | ⚠️ Needs verification | | | ||
| | S3 key prefix | `{{ field }}` template | Exporter config or `routing` | ⚠️ Needs verification | | | ||
| | Splunk index/source/sourcetype | `{{ field }}` templates | Exporter mapping config | ⚠️ Needs design | | | ||
| | Syslog facility/severity/appname | VRL field extraction + syslog codec | `transform` processor + exporter config | ✅ Supported | | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the compatibility table column mismatch.
These rows have five cells, but the header only defines four columns, so markdownlint will drop the trailing notes. Add a Notes column or fold the extra text into Status.
Proposed fix
-| Template Usage | Vector | OTEL | Status |
+| Template Usage | Vector | OTEL | Status | Notes |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | Template Usage | Vector | OTEL | Status | | |
| |---------------|--------|------|--------| | |
| | Elasticsearch index name | `{{ field }}` in index template | `elasticsearch.index` attribute on log records | ✅ Supported | Since [v0.122.0](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/exporter/elasticsearchexporter), dynamic routing via `elasticsearch.index` attribute is the default. Use `transform` processor to set the attribute per-record based on log fields. | | |
| | Kafka topic name | `{{ field }}` in topic template | Kafka exporter `topic_from_attribute` | ✅ Supported | Kafka exporter can derive topic from log attributes. | | |
| | CloudWatch log group / stream | `{{ field }}` templates | Exporter config | ⚠️ Needs verification | | | |
| | S3 key prefix | `{{ field }}` template | Exporter config or `routing` | ⚠️ Needs verification | | | |
| | Splunk index/source/sourcetype | `{{ field }}` templates | Exporter mapping config | ⚠️ Needs design | | | |
| | Syslog facility/severity/appname | VRL field extraction + syslog codec | `transform` processor + exporter config | ✅ Supported | | | |
| | Template Usage | Vector | OTEL | Status | Notes | | |
| |---------------|--------|------|--------|--------| | |
| | Elasticsearch index name | `{{ field }}` in index template | `elasticsearch.index` attribute on log records | ✅ Supported | Since [v0.122.0](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/exporter/elasticsearchexporter), dynamic routing via `elasticsearch.index` attribute is the default. Use `transform` processor to set the attribute per-record based on log fields. | | |
| | Kafka topic name | `{{ field }}` in topic template | Kafka exporter `topic_from_attribute` | ✅ Supported | Kafka exporter can derive topic from log attributes. | | |
| | CloudWatch log group / stream | `{{ field }}` templates | Exporter config | ⚠️ Needs verification | | | |
| | S3 key prefix | `{{ field }}` template | Exporter config or `routing` | ⚠️ Needs verification | | | |
| | Splunk index/source/sourcetype | `{{ field }}` templates | Exporter mapping config | ⚠️ Needs design | | | |
| | Syslog facility/severity/appname | VRL field extraction + syslog codec | `transform` processor + exporter config | ✅ Supported | | |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 118-118: Table column count
Expected: 4; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
[warning] 119-119: Table column count
Expected: 4; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
[warning] 120-120: Table column count
Expected: 4; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
[warning] 121-121: Table column count
Expected: 4; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
[warning] 122-122: Table column count
Expected: 4; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
[warning] 123-123: Table column count
Expected: 4; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 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 `@opentelemetry-collector-migration.md` around lines 116 - 123, The
compatibility table has a header/body column mismatch: the rows in the template
usage table contain an extra trailing notes cell that is not represented in the
header. Update the table in opentelemetry-collector-migration.md so the header
matches the row structure, either by adding a Notes column or by merging the
note text into the Status cell, and keep the existing entries for Elasticsearch,
Kafka, CloudWatch, S3, Splunk, and Syslog consistent.
Source: Linters/SAST tools
| func (c *OTELCollector) ModifyConfig(conf string) string { | ||
| re := regexp.MustCompile(`(?msU) journalctl.*?^\n`) | ||
| return string(re.ReplaceAll([]byte(conf), []byte(fakeJournal))) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target file =="
git ls-files test/framework/functional/otel/deploy.go
wc -l test/framework/functional/otel/deploy.go
cat -n test/framework/functional/otel/deploy.go | sed -n '1,140p'
echo
echo "== standalone regexp probe (Go) =="
cat >/tmp/otel_regex_probe.go <<'EOF'
package main
import (
"fmt"
"regexp"
)
const conf = `pre
journalctl
a
b
mid
x
y
post
`
func probe(pattern string) {
re := regexp.MustCompile(pattern)
m := re.FindStringIndex(conf)
fmt.Printf("pattern=%q match=%v\n", pattern, m)
if m != nil {
fmt.Printf("matched:\n---\n%s---\n", conf[m[0]:m[1]])
}
}
func main() {
probe(`(?msU) journalctl.*?^\n`)
probe(`(?ms) journalctl.*?^\n`)
}
EOF
go run /tmp/otel_regex_probe.goRepository: openshift/cluster-logging-operator
Length of output: 3205
U makes .*? greedy here, so the replacement can swallow more than the journalctl block.
Drop U so the match stops at the first blank line after journalctl.
Proposed fix
func (c *OTELCollector) ModifyConfig(conf string) string {
- re := regexp.MustCompile(`(?msU) journalctl.*?^\n`)
+ re := regexp.MustCompile(`(?ms) journalctl.*?^\n`)
return string(re.ReplaceAll([]byte(conf), []byte(fakeJournal)))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (c *OTELCollector) ModifyConfig(conf string) string { | |
| re := regexp.MustCompile(`(?msU) journalctl.*?^\n`) | |
| return string(re.ReplaceAll([]byte(conf), []byte(fakeJournal))) | |
| } | |
| func (c *OTELCollector) ModifyConfig(conf string) string { | |
| re := regexp.MustCompile(`(?ms) journalctl.*?^\n`) | |
| return string(re.ReplaceAll([]byte(conf), []byte(fakeJournal))) | |
| } |
🤖 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 `@test/framework/functional/otel/deploy.go` around lines 66 - 69, The regexp in
OTELCollector.ModifyConfig is using the U flag, which makes the non-greedy
journalctl block match too broad and can consume more than intended. Update the
regex used in ModifyConfig so it matches only the journalctl section up to the
first blank line, and keep the replacement localized to that block while
preserving the rest of the config.
|
@pavolloffay: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@pavolloffay started this investigation here https://github.com/jcantrill/cluster-logging-operator/tree/otelc/internal/generator a few months ago. My goal is to collect application logs and forward them to lokistack as a beginning |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
2 participants
This is ShiftWeek POC, do not merge!
The PR adds option to run the OTELcol instead of vector. In the first phase I am interested in passing the same tests with the OTELcol.
Description
/cc
/assign
Links
Summary by CodeRabbit