feat: add OpenTelemetry tracing for agent execution#89
Merged
Conversation
Add distributed tracing across the full agent request lifecycle: LLM API calls, agentic tool loop iterations, tool execution, HTTP request handling, and A2A delegation. Conversation content (messages, tool args/results) is captured as span events with 10KB truncation. Token usage is recorded as span attributes. Tracing is zero-overhead when disabled (OTel no-op provider). Three export modes: OTLP gRPC (Jaeger/Tempo), stdout JSON (kubectl logs), or both simultaneously. New flags: --otel-enabled, --otel-collector-endpoint, --otel-stdout Closes #34 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Pavel Anni <panni@redhat.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📝 WalkthroughWalkthroughThis PR integrates OpenTelemetry distributed tracing across the service: adds a telemetry package, HTTP middleware, instruments LLM providers, the A2A bridge, and the agent tool loop; wires telemetry init/shutdown and config/CLI; adds tests; and updates go.mod for OTel v1.44.0. ChangesOpenTelemetry tracing implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Rename the logger component from the legacy "ZT-AGENT" (Zero Trust demo) to "DOCSCLAW". Auto-detect output format based on TTY: color text for local dev terminals, structured JSON for K8s pods and pipes. Override with LOG_FORMAT=json|text. SPIFFE_DEMO_LOG_FORMAT kept as legacy alias. NO_COLOR still respected in text mode. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Pavel Anni <panni@redhat.com>
- Fix statusWriter to implement http.Flusher (and Unwrap) so SSE streaming continues to work through the OTel HTTP middleware - Remove codes.Error from denied tool calls — policy denial is expected behavior, not an operational error - Set error status on loopSpan for max iterations and context cancellation so these are distinguishable from success in traces - Add response content and token usage as span events/attributes on both providers' Complete methods (closes observability gap) - Extract W3C baggage header alongside traceparent in A2A executor Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Pavel Anni <panni@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cmd/serve.go (1)
610-617:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGive telemetry shutdown its own timeout budget.
server.ShutdownandotelShutdownshare the same 5s context. If the server drain uses most of that window, tracer shutdown gets an expired context and drops the final batch of spans.Suggested fix
log.Info("Shutting down docsclaw...") - shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defer cancel() - if err := server.Shutdown(shutdownCtx); err != nil { + serverShutdownCtx, serverCancel := context.WithTimeout(context.Background(), 5*time.Second) + defer serverCancel() + if err := server.Shutdown(serverShutdownCtx); err != nil { log.Error("Shutdown error", "error", err) } - if err := otelShutdown(shutdownCtx); err != nil { + + otelShutdownCtx, otelCancel := context.WithTimeout(context.Background(), 5*time.Second) + defer otelCancel() + if err := otelShutdown(otelShutdownCtx); err != nil { log.Error("OTel shutdown error", "error", err) }🤖 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/cmd/serve.go` around lines 610 - 617, server.Shutdown and otelShutdown are sharing the same shutdownCtx so if server.Shutdown consumes the 5s budget tracer shutdown may receive an expired context and drop spans; change the sequence to use one dedicated timeout context for the HTTP server (e.g., shutdownCtx from context.WithTimeout(...)) for server.Shutdown, then create a fresh context.WithTimeout (and defer its cancel) solely for otelShutdown and call otelShutdown with that new context so telemetry has its own timeout budget.
🤖 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/anthropic/anthropic.go`:
- Around line 63-68: Both Complete and StreamWithTools create an OpenTelemetry
span but do not consistently mark failures; update these methods (methods:
(*AnthropicProvider).Complete and (*AnthropicProvider).StreamWithTools) so that
every error return records the error and sets the span status before span.End():
call span.RecordError(err) and span.SetStatus(codes.Error, err.Error()) for any
non-nil err. Implement this by adding a deferred error-check wrapper immediately
after span creation that inspects a named return error (or a local err variable)
and, if non-nil, calls span.RecordError and span.SetStatus, ensuring the span is
marked failed for all early returns (also apply the same pattern to the stream
error paths and JSON/tooling error returns).
In `@internal/cmd/serve.go`:
- Around line 335-341: When cfg.OTel.Enabled is true but no exporter is
configured (StdoutExporter, CollectorEndpoint, or OTEL_EXPORTER_OTLP_ENDPOINT),
telemetry.Init will create a no-op tracer and silently drop spans; change the
startup path around the telemetry.Init call to detect this misconfiguration and
fail fast: before calling telemetry.Init (or by inspecting its return/status),
check cfg.OTel.Enabled and whether cfg.OTel.StdoutExporter or
cfg.OTel.CollectorEndpoint or the OTEL_EXPORTER_OTLP_ENDPOINT env var is set,
and if none are present log a clear error and exit; reference the telemetry.Init
call and cfg.OTel.Enabled/StdoutExporter/CollectorEndpoint symbols to locate
where to add this validation.
In `@internal/telemetry/content.go`:
- Around line 53-63: truncate currently slices to maxBytes then appends the
suffix, causing outputs to exceed the intended maxEventBytes; update truncate(s
string, maxBytes int) to reserve space for the suffix ("...[truncated]") by
computing suffix := "...[truncated]" and if maxBytes <= len(suffix) return
suffix[:maxBytes]; otherwise slice to maxBytes-len(suffix), walk back to a UTF-8
boundary (as before), trim trailing NULs and then append the suffix so the total
byte length never exceeds maxBytes; this change in truncate will automatically
fix AddMessageEvent, AddToolArgsEvent and AddToolResultEvent which call it.
In `@internal/telemetry/telemetry.go`:
- Around line 64-68: The current telemetry setup in
internal/telemetry/telemetry.go appends otlptracegrpc.WithInsecure() to grpcOpts
whenever OTEL_EXPORTER_OTLP_INSECURE is unset; change this to require an
explicit true value to enable plaintext (i.e., only append
otlptracegrpc.WithInsecure() when os.Getenv("OTEL_EXPORTER_OTLP_INSECURE") ==
"true"), and otherwise initialize secure TLS transport for the OTLP gRPC
exporter (remove the default insecure branch and ensure grpcOpts uses TLS
credentials by default); update the code paths that build grpcOpts (the grpcOpts
variable and the creation of the OTLP gRPC exporter) so secure/TLS is the
default, and document/validate the env check accordingly.
- Around line 56-76: The current condition prevents creating the OTLP exporter
when no endpoint env/flag is set, causing spans to be dropped; update the
creation logic so otlptracegrpc.New(...) is called when traces should be
exported even if no endpoint was provided: change the if that guards exporter
creation to true when cfg.Enabled && !cfg.StdoutExporter OR when
cfg.CollectorEndpoint != "" OR env OTEL_EXPORTER_OTLP_ENDPOINT != "" OR env
OTEL_EXPORTER_OTLP_TRACES_ENDPOINT != ""; keep the existing grpcOpts handling
(append WithEndpoint when cfg.CollectorEndpoint set, keep WithInsecure default
behavior), call otlptracegrpc.New(ctx, grpcOpts...) in that branch, and preserve
the error handling and slog.Info call (adjust log to include which env/flag was
used if you like).
In `@pkg/tools/loop_test.go`:
- Around line 437-443: The cleanup in TestRunToolLoopTracing unconditionally
sets the global tracer provider to noop.NewTracerProvider(), which can interfere
with other tests; before calling otel.SetTracerProvider(tp) capture the current
provider (e.g., prev := otel.GetTracerProvider()), and in the t.Cleanup restore
it via otel.SetTracerProvider(prev) while still shutting down tp (tp.Shutdown).
Update the t.Cleanup closure to restore the saved previous provider instead of
always setting noop.NewTracerProvider(), referencing the tp,
otel.SetTracerProvider, otel.GetTracerProvider, and t.Cleanup symbols.
In `@pkg/tools/loop.go`:
- Around line 63-67: When provider.CompleteWithTools returns an error in
agent.loop (the call to provider.CompleteWithTools), set the higher-level
loopSpan status to codes.Error before returning (in addition to
iterSpan.SetStatus) so loopSpan reflects the failure; similarly, in executeTool
where the "tool.execute" span marks a denied tool call via attributes, call
span.SetStatus(codes.Error, "<reason>") (or include err.Error() when available)
to mark the span as an error; update the code paths around
provider.CompleteWithTools, iterSpan, loopSpan, and executeTool/"tool.execute"
to call span.SetStatus(codes.Error, ...) with a concise error message before
ending/returning.
- Around line 160-167: The denied-tool path in executeTool sets
telemetry.AttrToolDenied and AttrToolDenyReason but does not mark the span as
failed; update the hook-denied branch (the block that calls span.SetAttributes
and slog.Warn for tc.Name) to also call span.SetStatus(codes.Error, reason)
(using the same status code pattern as other failure paths) so the span is
recorded as an error when a tool call is denied; ensure the codes symbol used
matches existing imports where other failures set span status.
---
Outside diff comments:
In `@internal/cmd/serve.go`:
- Around line 610-617: server.Shutdown and otelShutdown are sharing the same
shutdownCtx so if server.Shutdown consumes the 5s budget tracer shutdown may
receive an expired context and drop spans; change the sequence to use one
dedicated timeout context for the HTTP server (e.g., shutdownCtx from
context.WithTimeout(...)) for server.Shutdown, then create a fresh
context.WithTimeout (and defer its cancel) solely for otelShutdown and call
otelShutdown with that new context so telemetry has its own timeout budget.
🪄 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: Enterprise
Run ID: f13001e5-f5d0-47fd-83f6-19d19914436a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
go.modinternal/anthropic/anthropic.gointernal/bridge/client.gointernal/bridge/executor.gointernal/cmd/serve.gointernal/config/config.gointernal/logger/logger.gointernal/openai/openai.gointernal/telemetry/attributes.gointernal/telemetry/content.gointernal/telemetry/http.gointernal/telemetry/telemetry.gointernal/telemetry/telemetry_test.gopkg/tools/loop.gopkg/tools/loop_test.go
Comment on lines
+63
to
+68
| ctx, span := otel.Tracer(telemetry.TracerName).Start(ctx, "llm.api.call", | ||
| trace.WithAttributes( | ||
| telemetry.AttrLLMProvider.String(p.ProviderName()), | ||
| telemetry.AttrLLMModel.String(p.model), | ||
| )) | ||
| defer span.End() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In OpenTelemetry Go v1.44.0, what is the recommended way to mark a span as failed on error returns? Please verify whether spans should call both RecordError(err) and SetStatus(codes.Error, ...) before End().
💡 Result:
In OpenTelemetry Go v1.44.0, the recommended way to mark a span as failed when an operation returns an error is to explicitly set the span status to Error and record the error [1][2]. To achieve this, you should perform both of the following steps before ending the span [1][3]: 1. Use span.SetStatus(codes.Error, description) to set the span's status code to Error [1][4]. This identifies the span as a failed operation in telemetry backends [1][3]. 2. Use span.RecordError(err) to record the error as a span event [1][4]. This attaches the error details (such as the error type and message) to the span [5][3]. It is important to call both methods because they serve different purposes and operate independently [5][4][3]. The RecordError function does not automatically set the span's status to Error [1][5][3]. Decoupling these actions provides flexibility, allowing you to record errors (e.g., transient errors or retried operations) without necessarily marking the entire span as failed [6][3][7]. Note that while these are the standard tools for custom instrumentation, recent updates to OpenTelemetry semantic conventions have shifted focus for automatic instrumentation toward using span attributes (such as error.type) instead of recording errors as span events in certain contexts [8][9][10]. However, for manual error handling where you intend to surface a failure, the combination of SetStatus and RecordError remains the correct and recommended approach [1][2][7].
Citations:
- 1: https://opentelemetry.io/docs/languages/go/instrumentation/
- 2: https://stepinto.dev/posts/dotnet-to-go-observability/
- 3: https://opentelemetry.io/blog/2024/otel-errors/
- 4: https://github.com/open-telemetry/opentelemetry-go/blob/trace/v1.42.0/trace/span.go
- 5: https://github.com/open-telemetry/opentelemetry-go/blob/v1.25.0/sdk/trace/span.go
- 6: Do not set error status when recording an error with RecordError open-telemetry/opentelemetry-go#1661
- 7: otelhttp: Cannot record error for spans when producing new semantic convention open-telemetry/opentelemetry-go-contrib#7254
- 8: https://github.com/open-telemetry/semantic-conventions/blob/main/docs/general/recording-errors.md
- 9: https://opentelemetry.io/docs/specs/semconv/general/recording-errors/
- 10: instrumentation: update error handling to set attributes instead of recording errors open-telemetry/opentelemetry-go-contrib#8386
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the relevant portions of the file with line numbers
FILE="internal/anthropic/anthropic.go"
if [ ! -f "$FILE" ]; then
echo "Missing $FILE"
exit 1
fi
echo "---- $FILE (lines 1-220) ----"
sed -n '1,220p' "$FILE" | nl -ba
echo "---- Extract AnthropicProvider methods ----"
# Show Complete and StreamWithTools blocks (best-effort via line ranges around their defs)
# First find function line numbers
rg -n "func \\(p \\*AnthropicProvider\\) (Complete|StreamWithTools)\\b" "$FILE"
# Print around those functions
for pat in "func (p \\*AnthropicProvider) Complete" "func (p \\*AnthropicProvider) StreamWithTools"; do
ln=$(rg -n "$pat" "$FILE" | head -n1 | cut -d: -f1)
start=$((ln-40)); end=$((ln+220))
[ "$start" -lt 1 ] && start=1
sed -n "${start},${end}p" "$FILE" | nl -ba
doneRepository: redhat-et/docsclaw
Length of output: 180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="internal/anthropic/anthropic.go"
echo "---- FILE: $FILE (first 260 lines) ----"
sed -n '1,260p' "$FILE" | cat -n
echo "---- Tracing primitives usage ----"
echo "RecordError occurrences:"
rg -n "RecordError\\(" "$FILE" || true
echo "SetStatus occurrences:"
rg -n "SetStatus\\(" "$FILE" || true
echo "---- Find otel tracer Start sites ----"
rg -n "otel\\.Tracer\\(telemetry\\.TracerName\\)\\.Start" "$FILE" || true
echo "---- AnthropicProvider methods ----"
rg -n "func \\(p \\*AnthropicProvider\\) (Complete|StreamWithTools)\\b" "$FILE"
for fn in "Complete" "StreamWithTools"; do
# Find function start line
start=$(rg -n "func \\(p \\*AnthropicProvider\\) $fn\\b" "$FILE" | head -n1 | cut -d: -f1)
echo "---- $fn: approx body (start at $start) ----"
# Print a reasonable window; adjust if needed
sed -n "$start,$((start+220))p" "$FILE" | cat -n
doneRepository: redhat-et/docsclaw
Length of output: 25394
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="internal/anthropic/anthropic.go"
echo "---- tail of file (last 220 lines) ----"
tail -n 220 "$FILE" | cat -n
echo "---- span.SetStatus occurrences ----"
rg -n "span\.SetStatus\\(" "$FILE" || true
echo "---- Complete() return error paths ----"
# show returns in Complete
awk '
$0 ~ /^func \\(p \\*AnthropicProvider\\) Complete\\(/ {in=1}
in && $0 ~ /^func \\(p \\*AnthropicProvider\\) / && $0 !~ /^func \\(p \\*AnthropicProvider\\) Complete\\(/ {in=0}
in {print}
' "$FILE" | rg -n "return .*fmt\\.Errorf|return .*err" || true
echo "---- StreamWithTools() error-return paths (fmt.Errorf) ----"
awk '
$0 ~ /^func \\(p \\*AnthropicProvider\\) StreamWithTools\\(/ {in=1}
in && $0 ~ /^func \\(p \\*AnthropicProvider\\) / && $0 !~ /^func \\(p \\*AnthropicProvider\\) StreamWithTools\\(/ {in=0}
in {print}
' "$FILE" | rg -n "return .*fmt\\.Errorf|return .*err" || trueRepository: redhat-et/docsclaw
Length of output: 8484
Mark failed Anthropic calls as failed spans.
(*AnthropicProvider).Complete returns multiple errors (API request failed, empty response from API) without setting any span status/error—so the deferred span.End() records them as successful spans.
(*AnthropicProvider).StreamWithTools only calls span.SetStatus(codes.Error, ...) on the stream.Err() path; earlier failure returns (e.g., accumulated.Accumulate(...) and tool json.Unmarshal(...)) end the span without error status (and without recording the error).
For manual OpenTelemetry Go instrumentation, mark failures with both span.RecordError(err) and span.SetStatus(codes.Error, ...) before ending the span, ideally via a deferred wrapper that runs on every return.
Suggested pattern
-func (p *AnthropicProvider) Complete(ctx context.Context, systemPrompt, userPrompt string) (string, error) {
+func (p *AnthropicProvider) Complete(ctx context.Context, systemPrompt, userPrompt string) (_ string, err error) {
ctx, span := otel.Tracer(telemetry.TracerName).Start(ctx, "llm.api.call",
trace.WithAttributes(
telemetry.AttrLLMProvider.String(p.ProviderName()),
telemetry.AttrLLMModel.String(p.model),
))
- defer span.End()
+ defer func() {
+ if err != nil {
+ span.RecordError(err)
+ span.SetStatus(codes.Error, err.Error())
+ }
+ span.End()
+ }()Apply the same pattern to StreamWithTools so every error return is covered.
📝 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.
Suggested change
| ctx, span := otel.Tracer(telemetry.TracerName).Start(ctx, "llm.api.call", | |
| trace.WithAttributes( | |
| telemetry.AttrLLMProvider.String(p.ProviderName()), | |
| telemetry.AttrLLMModel.String(p.model), | |
| )) | |
| defer span.End() | |
| func (p *AnthropicProvider) Complete(ctx context.Context, systemPrompt, userPrompt string) (_ string, err error) { | |
| ctx, span := otel.Tracer(telemetry.TracerName).Start(ctx, "llm.api.call", | |
| trace.WithAttributes( | |
| telemetry.AttrLLMProvider.String(p.ProviderName()), | |
| telemetry.AttrLLMModel.String(p.model), | |
| )) | |
| defer func() { | |
| if err != nil { | |
| span.RecordError(err) | |
| span.SetStatus(codes.Error, err.Error()) | |
| } | |
| span.End() | |
| }() |
🤖 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/anthropic/anthropic.go` around lines 63 - 68, Both Complete and
StreamWithTools create an OpenTelemetry span but do not consistently mark
failures; update these methods (methods: (*AnthropicProvider).Complete and
(*AnthropicProvider).StreamWithTools) so that every error return records the
error and sets the span status before span.End(): call span.RecordError(err) and
span.SetStatus(codes.Error, err.Error()) for any non-nil err. Implement this by
adding a deferred error-check wrapper immediately after span creation that
inspects a named return error (or a local err variable) and, if non-nil, calls
span.RecordError and span.SetStatus, ensuring the span is marked failed for all
early returns (also apply the same pattern to the stream error paths and
JSON/tooling error returns).
Comment on lines
+335
to
+341
| // Initialize OpenTelemetry tracing (no-op when disabled) | ||
| otelShutdown, err := telemetry.Init(context.Background(), telemetry.Config{ | ||
| Enabled: cfg.OTel.Enabled, | ||
| CollectorEndpoint: cfg.OTel.CollectorEndpoint, | ||
| StdoutExporter: cfg.OTel.StdoutExporter, | ||
| ServiceVersion: version, | ||
| }) |
There was a problem hiding this comment.
Fail fast when tracing is enabled without any exporter.
telemetry.Init only attaches a processor when StdoutExporter, CollectorEndpoint, or OTEL_EXPORTER_OTLP_ENDPOINT is set. With cfg.OTel.Enabled=true and none of those configured, startup succeeds but every span is silently dropped.
Suggested fix
// Initialize OpenTelemetry tracing (no-op when disabled)
+ if cfg.OTel.Enabled &&
+ !cfg.OTel.StdoutExporter &&
+ cfg.OTel.CollectorEndpoint == "" &&
+ os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT") == "" {
+ return fmt.Errorf(
+ "OpenTelemetry enabled but no exporter configured; set --otel-stdout, --otel-collector-endpoint, or OTEL_EXPORTER_OTLP_ENDPOINT",
+ )
+ }
+
otelShutdown, err := telemetry.Init(context.Background(), telemetry.Config{
Enabled: cfg.OTel.Enabled,
CollectorEndpoint: cfg.OTel.CollectorEndpoint,
StdoutExporter: cfg.OTel.StdoutExporter,
ServiceVersion: version,📝 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.
Suggested change
| // Initialize OpenTelemetry tracing (no-op when disabled) | |
| otelShutdown, err := telemetry.Init(context.Background(), telemetry.Config{ | |
| Enabled: cfg.OTel.Enabled, | |
| CollectorEndpoint: cfg.OTel.CollectorEndpoint, | |
| StdoutExporter: cfg.OTel.StdoutExporter, | |
| ServiceVersion: version, | |
| }) | |
| // Initialize OpenTelemetry tracing (no-op when disabled) | |
| if cfg.OTel.Enabled && | |
| !cfg.OTel.StdoutExporter && | |
| cfg.OTel.CollectorEndpoint == "" && | |
| os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT") == "" { | |
| return fmt.Errorf( | |
| "OpenTelemetry enabled but no exporter configured; set --otel-stdout, --otel-collector-endpoint, or OTEL_EXPORTER_OTLP_ENDPOINT", | |
| ) | |
| } | |
| otelShutdown, err := telemetry.Init(context.Background(), telemetry.Config{ | |
| Enabled: cfg.OTel.Enabled, | |
| CollectorEndpoint: cfg.OTel.CollectorEndpoint, | |
| StdoutExporter: cfg.OTel.StdoutExporter, | |
| ServiceVersion: version, | |
| }) |
🤖 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/cmd/serve.go` around lines 335 - 341, When cfg.OTel.Enabled is true
but no exporter is configured (StdoutExporter, CollectorEndpoint, or
OTEL_EXPORTER_OTLP_ENDPOINT), telemetry.Init will create a no-op tracer and
silently drop spans; change the startup path around the telemetry.Init call to
detect this misconfiguration and fail fast: before calling telemetry.Init (or by
inspecting its return/status), check cfg.OTel.Enabled and whether
cfg.OTel.StdoutExporter or cfg.OTel.CollectorEndpoint or the
OTEL_EXPORTER_OTLP_ENDPOINT env var is set, and if none are present log a clear
error and exit; reference the telemetry.Init call and
cfg.OTel.Enabled/StdoutExporter/CollectorEndpoint symbols to locate where to add
this validation.
Comment on lines
+56
to
+76
| // OTLP gRPC exporter — sends to Jaeger, Tempo, or any OTLP collector. | ||
| // When CollectorEndpoint is empty, otlptracegrpc falls back to the | ||
| // standard OTEL_EXPORTER_OTLP_ENDPOINT env var (default: localhost:4317). | ||
| if cfg.CollectorEndpoint != "" || os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT") != "" { | ||
| grpcOpts := []otlptracegrpc.Option{} | ||
| if cfg.CollectorEndpoint != "" { | ||
| grpcOpts = append(grpcOpts, otlptracegrpc.WithEndpoint(cfg.CollectorEndpoint)) | ||
| } | ||
| // Default to insecure for local dev; production should use TLS | ||
| // via OTEL_EXPORTER_OTLP_INSECURE=false or mTLS certs. | ||
| if os.Getenv("OTEL_EXPORTER_OTLP_INSECURE") == "" { | ||
| grpcOpts = append(grpcOpts, otlptracegrpc.WithInsecure()) | ||
| } | ||
|
|
||
| otlpExp, otlpErr := otlptracegrpc.New(ctx, grpcOpts...) | ||
| if otlpErr != nil { | ||
| return noop, fmt.Errorf("creating OTLP exporter: %w", otlpErr) | ||
| } | ||
| opts = append(opts, sdktrace.WithBatcher(otlpExp)) | ||
| slog.Info("OTel OTLP exporter configured", "endpoint", cfg.CollectorEndpoint) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In OpenTelemetry Go v1.44.0, what endpoint does otlptracegrpc.New(...)use when no endpoint option and noOTEL_EXPORTER_OTLP_ENDPOINT env var are provided?
💡 Result:
In OpenTelemetry Go v1.44.0, when no endpoint option and no OTEL_EXPORTER_OTLP_ENDPOINT (or OTEL_EXPORTER_OTLP_TRACES_ENDPOINT) environment variable are provided, the otlptracegrpc.New(...) function defaults to localhost:4317 [1][2]. While some documentation and community discussions may refer to this default as http://localhost:4317 [3], the exporter's internal implementation defaults to the host and port localhost:4317 [1][2]. Because it is a gRPC exporter, it does not use an HTTP/HTTPS scheme in the same way as an HTTP-based exporter; rather, it connects to this target using gRPC [1]. If no scheme is provided, gRPC defaults to using the dns scheme [4].
Citations:
- 1: https://godoc.org/go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc
- 2: https://github.com/open-telemetry/opentelemetry-go/blob/main/exporters/otlp/otlptrace/otlptracegrpc/options.go
- 3: https://opentelemetry.io/docs/languages/sdk-configuration/otlp-exporter/
- 4: otlpmetricgrpc.New and otlptracegrpc.New Fail to use default environment variables for endpoint open-telemetry/opentelemetry-go#5562
--otel-enabled can silently drop every span (OTLP default never used)
internal/telemetry/telemetry.go only creates the OTLP exporter when cfg.CollectorEndpoint or OTEL_EXPORTER_OTLP_ENDPOINT is set. However, otlptracegrpc.New(...) defaults to localhost:4317 when no endpoint option and no relevant env var are provided, so OTLP-only configuration (e.g., Enabled=true and StdoutExporter=false) never exports spans—contradicting the inline fallback comment.
Suggested fix
// OTLP gRPC exporter — sends to Jaeger, Tempo, or any OTLP collector.
// When CollectorEndpoint is empty, otlptracegrpc falls back to the
// standard OTEL_EXPORTER_OTLP_ENDPOINT env var (default: localhost:4317).
- if cfg.CollectorEndpoint != "" || os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT") != "" {
+ useOTLP := !cfg.StdoutExporter ||
+ cfg.CollectorEndpoint != "" ||
+ os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT") != ""
+ if useOTLP {
grpcOpts := []otlptracegrpc.Option{}
if cfg.CollectorEndpoint != "" {
grpcOpts = append(grpcOpts, otlptracegrpc.WithEndpoint(cfg.CollectorEndpoint))
}Also consider gating on OTEL_EXPORTER_OTLP_TRACES_ENDPOINT so traces-specific endpoint config isn’t ignored.
📝 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.
Suggested change
| // OTLP gRPC exporter — sends to Jaeger, Tempo, or any OTLP collector. | |
| // When CollectorEndpoint is empty, otlptracegrpc falls back to the | |
| // standard OTEL_EXPORTER_OTLP_ENDPOINT env var (default: localhost:4317). | |
| if cfg.CollectorEndpoint != "" || os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT") != "" { | |
| grpcOpts := []otlptracegrpc.Option{} | |
| if cfg.CollectorEndpoint != "" { | |
| grpcOpts = append(grpcOpts, otlptracegrpc.WithEndpoint(cfg.CollectorEndpoint)) | |
| } | |
| // Default to insecure for local dev; production should use TLS | |
| // via OTEL_EXPORTER_OTLP_INSECURE=false or mTLS certs. | |
| if os.Getenv("OTEL_EXPORTER_OTLP_INSECURE") == "" { | |
| grpcOpts = append(grpcOpts, otlptracegrpc.WithInsecure()) | |
| } | |
| otlpExp, otlpErr := otlptracegrpc.New(ctx, grpcOpts...) | |
| if otlpErr != nil { | |
| return noop, fmt.Errorf("creating OTLP exporter: %w", otlpErr) | |
| } | |
| opts = append(opts, sdktrace.WithBatcher(otlpExp)) | |
| slog.Info("OTel OTLP exporter configured", "endpoint", cfg.CollectorEndpoint) | |
| } | |
| // OTLP gRPC exporter — sends to Jaeger, Tempo, or any OTLP collector. | |
| // When CollectorEndpoint is empty, otlptracegrpc falls back to the | |
| // standard OTEL_EXPORTER_OTLP_ENDPOINT env var (default: localhost:4317). | |
| useOTLP := !cfg.StdoutExporter || | |
| cfg.CollectorEndpoint != "" || | |
| os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT") != "" | |
| if useOTLP { | |
| grpcOpts := []otlptracegrpc.Option{} | |
| if cfg.CollectorEndpoint != "" { | |
| grpcOpts = append(grpcOpts, otlptracegrpc.WithEndpoint(cfg.CollectorEndpoint)) | |
| } | |
| // Default to insecure for local dev; production should use TLS | |
| // via OTEL_EXPORTER_OTLP_INSECURE=false or mTLS certs. | |
| if os.Getenv("OTEL_EXPORTER_OTLP_INSECURE") == "" { | |
| grpcOpts = append(grpcOpts, otlptracegrpc.WithInsecure()) | |
| } | |
| otlpExp, otlpErr := otlptracegrpc.New(ctx, grpcOpts...) | |
| if otlpErr != nil { | |
| return noop, fmt.Errorf("creating OTLP exporter: %w", otlpErr) | |
| } | |
| opts = append(opts, sdktrace.WithBatcher(otlpExp)) | |
| slog.Info("OTel OTLP exporter configured", "endpoint", cfg.CollectorEndpoint) | |
| } |
🤖 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/telemetry/telemetry.go` around lines 56 - 76, The current condition
prevents creating the OTLP exporter when no endpoint env/flag is set, causing
spans to be dropped; update the creation logic so otlptracegrpc.New(...) is
called when traces should be exported even if no endpoint was provided: change
the if that guards exporter creation to true when cfg.Enabled &&
!cfg.StdoutExporter OR when cfg.CollectorEndpoint != "" OR env
OTEL_EXPORTER_OTLP_ENDPOINT != "" OR env OTEL_EXPORTER_OTLP_TRACES_ENDPOINT !=
""; keep the existing grpcOpts handling (append WithEndpoint when
cfg.CollectorEndpoint set, keep WithInsecure default behavior), call
otlptracegrpc.New(ctx, grpcOpts...) in that branch, and preserve the error
handling and slog.Info call (adjust log to include which env/flag was used if
you like).
Comment on lines
+63
to
67
| resp, err := provider.CompleteWithTools(iterCtx, messages, toolDefs) | ||
| if err != nil { | ||
| iterSpan.SetStatus(codes.Error, err.Error()) | ||
| iterSpan.End() | ||
| return "", fmt.Errorf("LLM call failed: %w", err) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="pkg/tools/loop.go"
echo "=== File: $file ==="
echo "--- Lines 1-120 ---"
nl -ba "$file" | sed -n '1,120p'
echo "--- Lines 120-240 ---"
nl -ba "$file" | sed -n '120,240p'Repository: redhat-et/docsclaw
Length of output: 175
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="pkg/tools/loop.go"
python3 - <<'PY'
import itertools
path="pkg/tools/loop.go"
with open(path,'r',encoding='utf-8') as f:
lines=f.readlines()
def show(a,b):
for i in range(a-1,b):
if 0 <= i < len(lines):
print(f"{i+1:4d}\t{lines[i].rstrip()}")
print("=== span creation area (approx 35-90) ===")
show(35,90)
print("\n=== tool denial path area (approx 130-210) ===")
show(130,210)
PYRepository: redhat-et/docsclaw
Length of output: 5087
Set error status on tracing spans for both LLM failures and denied tool calls
pkg/tools/loop.go: whenprovider.CompleteWithToolsfails, onlyiterSpangetscodes.Error;loopSpanends viadefer loopSpan.End()with no error status, soagent.loopcan look successful (63-67).pkg/tools/loop.go: on tool denial inexecuteTool, thetool.executespan sets denial attributes but never callsspan.SetStatus(codes.Error, ...)(158-167).
Suggested fix
resp, err := provider.CompleteWithTools(iterCtx, messages, toolDefs)
if err != nil {
iterSpan.SetStatus(codes.Error, err.Error())
+ loopSpan.SetStatus(codes.Error, err.Error())
iterSpan.End()
return "", fmt.Errorf("LLM call failed: %w", err)
}📝 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.
Suggested change
| resp, err := provider.CompleteWithTools(iterCtx, messages, toolDefs) | |
| if err != nil { | |
| iterSpan.SetStatus(codes.Error, err.Error()) | |
| iterSpan.End() | |
| return "", fmt.Errorf("LLM call failed: %w", err) | |
| resp, err := provider.CompleteWithTools(iterCtx, messages, toolDefs) | |
| if err != nil { | |
| iterSpan.SetStatus(codes.Error, err.Error()) | |
| loopSpan.SetStatus(codes.Error, err.Error()) | |
| iterSpan.End() | |
| return "", fmt.Errorf("LLM call failed: %w", err) |
🤖 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 `@pkg/tools/loop.go` around lines 63 - 67, When provider.CompleteWithTools
returns an error in agent.loop (the call to provider.CompleteWithTools), set the
higher-level loopSpan status to codes.Error before returning (in addition to
iterSpan.SetStatus) so loopSpan reflects the failure; similarly, in executeTool
where the "tool.execute" span marks a denied tool call via attributes, call
span.SetStatus(codes.Error, "<reason>") (or include err.Error() when available)
to mark the span as an error; update the code paths around
provider.CompleteWithTools, iterSpan, loopSpan, and executeTool/"tool.execute"
to call span.SetStatus(codes.Error, ...) with a concise error message before
ending/returning.
Comment on lines
160
to
167
| if !allow { | ||
| span.SetAttributes( | ||
| telemetry.AttrToolDenied.Bool(true), | ||
| telemetry.AttrToolDenyReason.String(reason), | ||
| ) | ||
| slog.Warn("tool call denied by hook", | ||
| "tool", tc.Name, "reason", reason) | ||
| return Errorf("Tool call denied: %s", reason) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and print the relevant region with line numbers
if [ -f "pkg/tools/loop.go" ]; then
echo "=== pkg/tools/loop.go (lines 130-220) ==="
nl -ba pkg/tools/loop.go | sed -n '130,220p'
else
echo "pkg/tools/loop.go not found"
exit 1
fi
# Also search for span.SetStatus usages in the file
echo "=== span.SetStatus occurrences in pkg/tools/loop.go ==="
rg -n "span\.SetStatus" pkg/tools/loop.go || true
# Confirm whether otel codes is imported
echo "=== imports referencing otel codes ==="
rg -n "otel/codes" pkg/tools/loop.go || trueRepository: redhat-et/docsclaw
Length of output: 165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Locate pkg/tools/loop.go ==="
ls -la pkg/tools/loop.go 2>/dev/null || true
echo "=== Find loop.go files ==="
fd -t f -n "loop.go" . || true
# Use awk for line numbers (nl not guaranteed)
FILE="pkg/tools/loop.go"
if [ -f "$FILE" ]; then
echo "=== Showing $FILE around lines 140-210 (best-effort) ==="
awk 'NR>=120 && NR<=230 {printf "%6d\t%s\n", NR, $0}' "$FILE" | sed -n '1,200p'
echo "=== span.SetStatus occurrences in $FILE ==="
rg -n "span\.SetStatus" "$FILE" || true
echo "=== otel/codes occurrences in $FILE ==="
rg -n "otel/codes|codes\." "$FILE" || true
else
echo "=== $FILE not found; printing tree for pkg/tools (if exists) ==="
ls -la pkg/tools 2>/dev/null || true
fiRepository: redhat-et/docsclaw
Length of output: 4096
Mark denied tool calls as failed spans too
The hook-denied path sets denial attributes but never calls span.SetStatus(codes.Error, ...), unlike other failure paths in executeTool.
Suggested fix
if !allow {
span.SetAttributes(
telemetry.AttrToolDenied.Bool(true),
telemetry.AttrToolDenyReason.String(reason),
)
+ span.SetStatus(codes.Error, reason)
slog.Warn("tool call denied by hook",
"tool", tc.Name, "reason", reason)
return Errorf("Tool call denied: %s", reason)
}🤖 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 `@pkg/tools/loop.go` around lines 160 - 167, The denied-tool path in
executeTool sets telemetry.AttrToolDenied and AttrToolDenyReason but does not
mark the span as failed; update the hook-denied branch (the block that calls
span.SetAttributes and slog.Warn for tc.Name) to also call
span.SetStatus(codes.Error, reason) (using the same status code pattern as other
failure paths) so the span is recorded as an error when a tool call is denied;
ensure the codes symbol used matches existing imports where other failures set
span status.
- Fix truncate to account for suffix length in byte budget so result never exceeds maxBytes - Default OTLP gRPC to TLS; only use insecure when explicitly set to OTEL_EXPORTER_OTLP_INSECURE=true - Save/restore global TracerProvider in test instead of always setting noop - Set loopSpan error status when CompleteWithTools fails so parent span reflects child failure - Add span error status to all error paths in both providers' Complete methods - Give OTel shutdown its own timeout context separate from HTTP server shutdown Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Pavel Anni <panni@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
1 participant
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
internal/telemetry/package with OTel SDK initialization, span attribute constants, and conversation content capture helpers (10KB truncation)pkg/tools/loop.go) with parent/child spans for loop, iterations, and tool executionllm.api.callspans capturing token usage and latencytraceparentfrom A2A ServiceParams for distributed multi-agent tracing--otel-stdoutflag for JSON span export to stdout (forkubectl logs/ log pipelines)Trace hierarchy
New flags
--otel-enabled--otel-collector-endpoint--otel-stdoutZero overhead when disabled — OTel no-op provider, no
if enabledchecks in instrumentation code.Closes #34
Test plan
golangci-lintclean on all modified packages--otel-stdout— trace hierarchy confirmed locallydocker run jaegertracing/all-in-one) and--otel-collector-endpointSummary by CodeRabbit
New Features
Configuration
Tests