open-code-review/internal/telemetry/exporter_test.go
xujiejie 46dde274d8
Feat/telemetry http exporter (#314)
* feat(telemetry): add OTLP HTTP exporter and print TraceID

- Add HTTP/protobuf exporter support alongside existing gRPC exporter
- Route based on OTEL_EXPORTER_OTLP_PROTOCOL config (http/protobuf vs grpc)
- Print TraceID to stderr when telemetry is enabled for easier correlation
- Add corresponding unit tests

* feat(telemetry): add span coverage for LLM calls, tool execution, plan/filter phases

- Add StartLLMSpan / RecordLLMResult helpers (span.go), symmetric with
  existing StartToolSpan / RecordToolResult
- Wrap LLM completion calls in llmloop.RunPerFile with llm.request spans
- Wrap all three tool execution paths in executeToolCall with
  tool.execute.* spans (dynamic tools, code_comment sync/async, other tools)
- Add plan.execute span around executePlanPhase
- Add main.loop span around RunPerFile call in executeSubtask
- Add review_filter.execute span around executeReviewFilter, with
  comments.before / comments.filtered attributes
- Record llm.error attribute on LLM failures for diagnosability
- Record review.repo / review.from / review.to / review.model on the
  top-level review.run span
- Metrics (RecordLLMRequest / RecordToolCall) are preserved alongside
  the new spans — they serve different purposes (aggregate dashboards
  vs per-run diagnosis)

Verified end-to-end against Sunfire (OTLP HTTP gateway): full span tree
observed for review.run -> subtask.execute -> plan.execute/main.loop/
review_filter.execute -> llm.request/tool.execute.*

* fix(telemetry): address CR findings — span error handling, async span lifecycle, protocol robustness

- Add span.RecordError(err) to RecordLLMResult and RecordToolResult for
  consistency with EndSpan
- Use OTel standard pattern (span.SetStatus + span.RecordError) in error
  paths of review.run, plan.execute, main.loop, review_filter.execute
- Move async code_comment span end into pool.Submit callback so span
  duration reflects actual execution time
- Unify time.Since(startTime) in code_comment error path to a single dur
- Remove http/json from supported OTLP protocols (not actually implemented)
- Add stderr warning when unknown OTLP protocol falls back to gRPC

* feat(telemetry): include trace_id in JSON output, restrict stderr to text format

- Add trace_id as top-level field in jsonOutput struct (omitempty)
- JSON format: trace_id in structured response for programmatic extraction
- Text format: TraceID printed to stderr for human debugging
- Telemetry disabled: trace_id field omitted entirely

* fix: address PR review findings

- loop.go: wrap async span lifecycle in defer to prevent leak on panic
- exporter.go: update parseOTLPEndpoint comment to reflect gRPC+HTTP usage
- scan_cmd.go: align traceID extraction and OTel error handling with review_cmd
- output.go/shared.go: propagate traceID to outputJSONNoFiles for consistency
- agent.go: move comments.filtered attribute before early return so 0 is
  distinguishable from not-executed

* feat(telemetry): address PR review — http/json routing, LLM span coverage, trace_id tests

- Route http/json to HTTP exporter (Go OTel SDK HTTP transport only
  supports protobuf serialization; users need HTTP transport, not JSON encoding)
- Add llm.request spans to executePlanPhase, executeReviewFilter, and
  ReLocateComment with Usage nil-safety consistent with loop.go
- Add trace_id assertions to output helper tests and emitRunResult
  end-to-end tests using real TracerProvider

* docs: add OTLP protocol selection and endpoint format to telemetry section

Sync across all 5 README language versions (en, zh-CN, ja-JP, ko-KR, ru-RU).

* fix: unify time.Since in async code_comment defer to single dur variable
2026-07-08 13:12:21 +08:00

200 lines
5.2 KiB
Go

package telemetry
import (
"bytes"
"context"
"io"
"os"
"strings"
"testing"
"go.opentelemetry.io/otel/sdk/resource"
)
func TestParseOTLPEndpoint(t *testing.T) {
cases := []struct {
name string
endpoint string
wantAddr string
wantInsecure bool
}{
{"http scheme strips and is insecure", "http://192.0.2.1:4317", "192.0.2.1:4317", true},
{"https scheme strips and keeps TLS", "https://otel.example.com:4317", "otel.example.com:4317", false},
{"bare host:port unchanged and keeps TLS", "localhost:4317", "localhost:4317", false},
{"uppercase HTTP scheme strips and is insecure", "HTTP://192.0.2.1:4317", "192.0.2.1:4317", true},
{"mixed-case Https scheme strips and keeps TLS", "Https://otel.example.com:4317", "otel.example.com:4317", false},
{"endpoint shorter than scheme prefix is unchanged", "ht", "ht", false},
{"http scheme with trailing slash is trimmed", "http://192.0.2.1:4317/", "192.0.2.1:4317", true},
{"https scheme with trailing slash is trimmed", "https://otel.example.com:4317/", "otel.example.com:4317", false},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
addr, insecure := parseOTLPEndpoint(tc.endpoint)
if addr != tc.wantAddr {
t.Errorf("addr = %q, want %q", addr, tc.wantAddr)
}
if insecure != tc.wantInsecure {
t.Errorf("insecure = %v, want %v", insecure, tc.wantInsecure)
}
})
}
}
func TestNewStdoutTraceExporter(t *testing.T) {
exp, err := newStdoutTraceExporter()
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if exp == nil {
t.Error("expected non-nil exporter")
}
}
func TestNewStdoutMetricExporter(t *testing.T) {
exp, err := newStdoutMetricExporter()
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if exp == nil {
t.Error("expected non-nil exporter")
}
}
func TestInitConsoleProviders(t *testing.T) {
tracerProvider = nil
meterProvider = nil
shutdownFuncs = nil
defer func() {
for _, fn := range shutdownFuncs {
_ = fn(context.Background())
}
tracerProvider = nil
meterProvider = nil
shutdownFuncs = nil
}()
initConsoleProviders(resource.Default())
if tracerProvider == nil {
t.Error("expected tracerProvider to be set after initConsoleProviders")
}
if meterProvider == nil {
t.Error("expected meterProvider to be set after initConsoleProviders")
}
if len(shutdownFuncs) != 2 {
t.Errorf("expected 2 shutdown funcs, got %d", len(shutdownFuncs))
}
}
func TestInitOTLPProviders_InvalidEndpoint(t *testing.T) {
tracerProvider = nil
meterProvider = nil
shutdownFuncs = nil
defer func() {
for _, fn := range shutdownFuncs {
_ = fn(context.Background())
}
tracerProvider = nil
meterProvider = nil
shutdownFuncs = nil
}()
cfg := Config{
Exporter: "otlp",
OTLPEndpoint: "localhost:0",
}
initOTLPProviders(context.Background(), resource.Default(), cfg)
if tracerProvider == nil {
t.Error("expected tracerProvider to be set (OTLP exporter creation is lazy)")
}
}
func TestInitOTLPHTTPProviders_InvalidEndpoint(t *testing.T) {
tracerProvider = nil
meterProvider = nil
shutdownFuncs = nil
defer func() {
for _, fn := range shutdownFuncs {
_ = fn(context.Background())
}
tracerProvider = nil
meterProvider = nil
shutdownFuncs = nil
}()
cfg := Config{
Exporter: "otlp",
OTLPEndpoint: "localhost:0",
OTLPProtocol: "http/protobuf",
}
initOTLPHTTPProviders(context.Background(), resource.Default(), cfg)
if tracerProvider == nil {
t.Error("expected tracerProvider to be set (OTLP HTTP exporter creation is lazy)")
}
}
func TestInitOTLPProviders_ProtocolRouting(t *testing.T) {
cases := []struct {
name string
protocol string
wantWarning bool // default branch emits a gRPC fallback warning
}{
{"grpc default", "grpc", false},
{"empty defaults to grpc", "", false},
{"http/protobuf routes to http", "http/protobuf", false},
{"http/json routes to http", "http/json", false},
{"unknown falls back to grpc", "foo", true},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
tracerProvider = nil
meterProvider = nil
shutdownFuncs = nil
// Capture stderr to assert on the fallback warning.
oldStderr := os.Stderr
r, w, _ := os.Pipe()
os.Stderr = w
cfg := Config{
Exporter: "otlp",
OTLPEndpoint: "localhost:0",
OTLPProtocol: tc.protocol,
}
initOTLPProviders(context.Background(), resource.Default(), cfg)
w.Close()
os.Stderr = oldStderr
defer func() {
for _, fn := range shutdownFuncs {
_ = fn(context.Background())
}
tracerProvider = nil
meterProvider = nil
shutdownFuncs = nil
}()
if tracerProvider == nil {
t.Error("expected tracerProvider to be set")
}
if meterProvider == nil {
t.Error("expected meterProvider to be set")
}
if len(shutdownFuncs) != 2 {
t.Errorf("expected 2 shutdown funcs, got %d", len(shutdownFuncs))
}
var buf bytes.Buffer
io.Copy(&buf, r)
stderrOut := buf.String()
if tc.wantWarning {
if !strings.Contains(stderrOut, "falling back to gRPC") {
t.Errorf("expected gRPC fallback warning in stderr, got %q", stderrOut)
}
} else {
if strings.Contains(stderrOut, "falling back to gRPC") {
t.Errorf("expected no fallback warning, got %q", stderrOut)
}
}
})
}
}