feat: add distributed tracing (Jaeger) to local-setup#922
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds configurable Jaeger tracing to the local test environment: new Make variables and conditional logic, an Istio tracing configuration target, Kuadrant operator/CR tracing wiring, reordered local setup sequencing, and user-facing documentation. ChangesJaeger Tracing Setup for Kuadrant Test Suite
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
make/istio.mk (1)
35-36: Consider extracting the complex JSON patch for maintainability.The single-line
kubectl patchcommand at line 36 is very long and difficult to read or modify. Consider using a heredoc or external JSON file to improve maintainability.Suggested refactor using heredoc
`@echo` "Configuring Istio for tracing with Jaeger..." @# Patch Istio CR to add tracing extension provider and JSON access logs - `@kubectl` patch istio default -n istio-system --type=merge -p '{"spec":{"values":{"meshConfig":{"accessLogFile":"/dev/stdout","accessLogEncoding":"JSON","accessLogFormat":"{\"start_time\":\"%START_TIME%\",\"method\":\"%REQ(:METHOD)%\",\"path\":\"%REQ(X-ENVOY-ORIGINAL-PATH?:PATH)%\",\"protocol\":\"%PROTOCOL%\",\"response_code\":\"%RESPONSE_CODE%\",\"response_flags\":\"%RESPONSE_FLAGS%\",\"bytes_received\":\"%BYTES_RECEIVED%\",\"bytes_sent\":\"%BYTES_SENT%\",\"duration\":\"%DURATION%\",\"upstream_service_time\":\"%RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)%\",\"x_forwarded_for\":\"%REQ(X-FORWARDED-FOR)%\",\"user_agent\":\"%REQ(USER-AGENT)%\",\"request_id\":\"%REQ(X-REQUEST-ID)%\",\"authority\":\"%REQ(:AUTHORITY)%\",\"upstream_host\":\"%UPSTREAM_HOST%\",\"upstream_cluster\":\"%UPSTREAM_CLUSTER%\",\"route_name\":\"%ROUTE_NAME%\"}","enableTracing":true,"defaultConfig":{"tracing":{}},"extensionProviders":[{"name":"jaeger-otlp","opentelemetry":{"port":4317,"service":"jaeger-collector.$(TOOLS_NAMESPACE).svc.cluster.local"}}]}}}}' + `@kubectl` patch istio default -n istio-system --type=merge -p "$$( \ + cat <<-EOF + { + "spec": { + "values": { + "meshConfig": { + "accessLogFile": "/dev/stdout", + "accessLogEncoding": "JSON", + "enableTracing": true, + "defaultConfig": {"tracing": {}}, + "extensionProviders": [{ + "name": "jaeger-otlp", + "opentelemetry": { + "port": 4317, + "service": "jaeger-collector.$(TOOLS_NAMESPACE).svc.cluster.local" + } + }] + } + } + } + } + EOF + )"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@make/istio.mk` around lines 35 - 36, The long single-line kubectl patch command (the line invoking "kubectl patch istio default -n istio-system --type=merge -p") embeds a large JSON payload (including keys like accessLogFile, accessLogEncoding, accessLogFormat, enableTracing, defaultConfig.tracing, and extensionProviders) which is hard to read and maintain; extract that JSON into a separate, well-formatted artifact (either a checked-in JSON/YAML file or a heredoc block) and update the kubectl invocation to read the payload from that source (e.g., pass the file contents or heredoc output into the -p argument), ensuring the accessLogFormat and extensionProviders JSON remains valid and quoting is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Around line 78-81: The docs and implementation disagree on Jaeger OTLP port:
update either the documentation text or the default JAEGER_COLLECTOR_ENDPOINT so
they match; specifically, change the documentation line that claims traces are
sent to port 4318 to reflect port 4317, or change the default value of
JAEGER_COLLECTOR_ENDPOINT (in make/vars.mk) to use 4318 if you intend to use
OTLP/HTTP. Locate the symbol JAEGER_COLLECTOR_ENDPOINT and the doc text
"Operator sends traces to jaeger-collector.tools.svc.cluster.local:4318" and
make them consistent (pick 4317 for OTLP/gRPC or 4318 for OTLP/HTTP) and update
any related README/CLAUDE.md references accordingly.
In `@make/vars.mk`:
- Around line 42-44: The JAEGER_COLLECTOR_ENDPOINT default uses an unsupported
"rpc://" scheme; update the JAEGER_COLLECTOR_ENDPOINT variable to use an http or
https scheme (e.g., change JAEGER_COLLECTOR_ENDPOINT ?= rpc://... to
JAEGER_COLLECTOR_ENDPOINT ?=
http://jaeger-collector.$(TOOLS_NAMESPACE).svc.cluster.local:4317) so the
OpenTelemetry OTLP exporter accepts the endpoint; ensure any related logic that
reads JAEGER_COLLECTOR_ENDPOINT continues to work with the http/https URL.
---
Nitpick comments:
In `@make/istio.mk`:
- Around line 35-36: The long single-line kubectl patch command (the line
invoking "kubectl patch istio default -n istio-system --type=merge -p") embeds a
large JSON payload (including keys like accessLogFile, accessLogEncoding,
accessLogFormat, enableTracing, defaultConfig.tracing, and extensionProviders)
which is hard to read and maintain; extract that JSON into a separate,
well-formatted artifact (either a checked-in JSON/YAML file or a heredoc block)
and update the kubectl invocation to read the payload from that source (e.g.,
pass the file contents or heredoc output into the -p argument), ensuring the
accessLogFormat and extensionProviders JSON remains valid and quoting is
preserved.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0131b018-c2a7-4ee2-8208-4a58fbca094a
📒 Files selected for processing (7)
CLAUDE.mdconfig/settings.local.yaml.tplmake/istio.mkmake/kuadrant.mkmake/local-setup.mkmake/tools.mkmake/vars.mk
averevki
reviewed
Apr 22, 2026
crstrn13
added
enhancement
silvi-t
reviewed
Apr 24, 2026
Contributor
silvi-t
left a comment
silvi-t
left a comment
There was a problem hiding this comment.
The OTEL env vars for the limitador-operator are missing. Without these, the limitador-operator service won't appear in Jaeger. You can check the helm-charts-olm post-install hook in charts/kuadrant-instances/templates/_helpers.tpl to see how it's done there.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
make/vars.mk (1)
42-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse an OTLP-compatible scheme for
JAEGER_COLLECTOR_ENDPOINT.Line 44 sets
rpc://..., which is likely unsupported forOTEL_EXPORTER_OTLP_ENDPOINT. Because this value is re-used later (for example Line 47), trace export can fail across multiple components.🔧 Proposed fix
-JAEGER_COLLECTOR_ENDPOINT ?= rpc://jaeger-collector.$(TOOLS_NAMESPACE).svc.cluster.local:4317 +JAEGER_COLLECTOR_ENDPOINT ?= http://jaeger-collector.$(TOOLS_NAMESPACE).svc.cluster.local:4317For OpenTelemetry OTLP exporter configuration, what URL schemes are valid for OTEL_EXPORTER_OTLP_ENDPOINT, and is rpc:// supported?🤖 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 `@make/vars.mk` around lines 42 - 44, The JAEGER_COLLECTOR_ENDPOINT currently uses an unsupported "rpc://" scheme for OTLP; update JAEGER_COLLECTOR_ENDPOINT to use an OTLP-compatible URL scheme (e.g., "http://" or "https://") so OTEL_EXPORTER_OTLP_ENDPOINT and any components that reuse this variable can successfully export traces; change the value of JAEGER_COLLECTOR_ENDPOINT (and any references to it) to a proper URL like "http://jaeger-collector.$(TOOLS_NAMESPACE).svc.cluster.local:4317" or use "https://" if TLS is required, and ensure INSTALL_TRACING remains unchanged.
🤖 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 `@make/kuadrant.mk`:
- Around line 35-38: The patch hardcodes the OTEL collector endpoint in the
kubectl env command for deployment/limitador-operator-controller-manager by
setting OTEL_EXPORTER_OTLP_ENDPOINT to the in-cluster URL; change this to use
the JAEGER_COLLECTOR_ENDPOINT variable (with a sensible fallback if needed) so
overrides take effect, i.e., replace the hardcoded URL with
$(JAEGER_COLLECTOR_ENDPOINT) (or
${JAEGER_COLLECTOR_ENDPOINT:-http://jaeger-collector.$(TOOLS_NAMESPACE).svc.cluster.local:4318})
while keeping OTEL_EXPORTER_OTLP_INSECURE intact and still using
KUADRANT_NAMESPACE and TOOLS_NAMESPACE variables.
---
Duplicate comments:
In `@make/vars.mk`:
- Around line 42-44: The JAEGER_COLLECTOR_ENDPOINT currently uses an unsupported
"rpc://" scheme for OTLP; update JAEGER_COLLECTOR_ENDPOINT to use an
OTLP-compatible URL scheme (e.g., "http://" or "https://") so
OTEL_EXPORTER_OTLP_ENDPOINT and any components that reuse this variable can
successfully export traces; change the value of JAEGER_COLLECTOR_ENDPOINT (and
any references to it) to a proper URL like
"http://jaeger-collector.$(TOOLS_NAMESPACE).svc.cluster.local:4317" or use
"https://" if TLS is required, and ensure INSTALL_TRACING remains unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 40a93e88-a354-4ea0-b17e-745d87693c2f
📒 Files selected for processing (7)
.claude/rules/tracing-setup.mdREADME.mdconfig/settings.local.yaml.tplmake/istio.mkmake/kuadrant.mkmake/local-setup.mkmake/vars.mk
✅ Files skipped from review due to trivial changes (3)
- .claude/rules/tracing-setup.md
- README.md
- config/settings.local.yaml.tpl
🚧 Files skipped from review as they are similar to previous changes (1)
- make/istio.mk
Signed-off-by: Alexander Cristurean <acristur@redhat.com> # Conflicts: # config/settings.local.yaml.tpl
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
crstrn13
force-pushed
the
cristu/issues/907
branch
from
9a653e5 to
9e12328
Compare
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
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
3 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
Implements #907 - Adds complete distributed tracing configuration to
make local-setupfor debugging control plane (operator) and data plane (gateway) flows locally.Changes
1. Infrastructure Setup
Tools namespace (
make/tools.mk):TOOLS_NAMESPACEvariable (default:tools)--namespace $(TOOLS_NAMESPACE)Tracing variables (
make/vars.mk):2. Control Plane Tracing (kuadrant-operator)
New target (
make/kuadrant.mk):configure-kuadrant-tracing-operatorOTEL_EXPORTER_OTLP_ENDPOINT=rpc://jaeger-collector.tools.svc.cluster.local:4317OTEL_EXPORTER_OTLP_INSECURE=trueLOG_LEVEL=debug3. Data Plane Tracing (gateway/envoy)
New target (
make/kuadrant.mk):configure-kuadrant-tracing-cr4. Istio Observability Configuration
New target (
make/istio.mk):configure-istio-tracingTelemetryresource with 100% sampling rateAccess log format includes:
5. Deployment Flow
Updated (
make/local-setup.mk):Key improvement: Tracing configuration happens AFTER Jaeger is deployed (no broken references)
6. Configuration & Documentation
Settings template (
config/settings.local.yaml.tpl):Documentation (
CLAUDE.md):Test Plan
Verify OTEL env vars on operator
Verify Kuadrant CR tracing config
Verify Istio tracing config
Run tracing tests
Access Jaeger UI
Expected Impact
Before:
After:
Configuration Options
Enable (default):
make local-setup # Tracing enabled by defaultDisable:
Custom tools namespace:
Technical Details
Endpoints:
rpc://jaeger-collector.tools.svc.cluster.local:4317(OTLP)http://jaeger-query.tools.svc.cluster.local:80Tracing components:
kuadrant-olm/tools-instancesHelm chartSampling:
randomSamplingPercentage: 100)Related
deploy-testsuite-tools🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation