test: improve coverage for telemetry package and exclude extensions from build targets

Add comprehensive tests for events, metrics, provider, shutdown, span,
and exporter in the telemetry package. Update Makefile to exclude the
extensions directory from test, fmt, vet, and check targets.

Makefile

@@ -31,8 +31,10 @@ endef
 build:
 	$(GO) build -ldflags "$(LD_FLAGS)" -o $(DIST_DIR)/$(BINARY_NAME) ./cmd/opencodereview
 
+PACKAGES := $(shell $(GO) list ./... | grep -v /extensions/)
+
 test:
-	LC_ALL=C $(GO) test -v -race -count=1 ./...
+	LC_ALL=C $(GO) test -v -race -count=1 $(PACKAGES)
 
 clean:
 	rm -rf $(DIST_DIR)
@@ -44,15 +46,15 @@ help: build
 	$(DIST_DIR)/$(BINARY_NAME) -h
 
 fmt:
-	$(GO) fmt ./...
+	$(GO) fmt $(PACKAGES)
 
 vet:
-	LC_ALL=C $(GO) vet ./...
+	LC_ALL=C $(GO) vet $(PACKAGES)
 
 check:
 	$(GO) mod tidy
-	$(GO) fmt ./...
-	LC_ALL=C $(GO) vet ./...
+	$(GO) fmt $(PACKAGES)
+	LC_ALL=C $(GO) vet $(PACKAGES)
 	@echo "check passed"
 
 # ── Cross-platform targets ───────────────────────────────────────────────────

internal/telemetry/events_test.go

@@ -1,10 +1,162 @@
 package telemetry
 
 import (
+	"bytes"
+	"context"
+	"errors"
+	"fmt"
+	"io"
+	"os"
+	"strings"
 	"testing"
 	"time"
+
+	"go.opentelemetry.io/otel"
+	"go.opentelemetry.io/otel/attribute"
+	sdkmetric "go.opentelemetry.io/otel/sdk/metric"
+	"go.opentelemetry.io/otel/sdk/resource"
+	sdktrace "go.opentelemetry.io/otel/sdk/trace"
 )
 
+func setupEnabledTelemetry(t *testing.T) {
+	t.Helper()
+	tp := sdktrace.NewTracerProvider(sdktrace.WithResource(resource.Default()))
+	mp := sdkmetric.NewMeterProvider(sdkmetric.WithResource(resource.Default()))
+	otel.SetTracerProvider(tp)
+	otel.SetMeterProvider(mp)
+	tracerProvider = tp
+	meterProvider = mp
+	initialized = true
+	shutdownFuncs = []func(context.Context) error{
+		func(ctx context.Context) error { return tp.Shutdown(ctx) },
+		func(ctx context.Context) error { return mp.Shutdown(ctx) },
+	}
+	initMetricsOnce = false
+	t.Cleanup(func() {
+		_ = tp.Shutdown(context.Background())
+		_ = mp.Shutdown(context.Background())
+		tracerProvider = nil
+		meterProvider = nil
+		initialized = false
+		shutdownFuncs = nil
+		initMetricsOnce = false
+	})
+}
+
+func captureStderr(t *testing.T, fn func()) string {
+	t.Helper()
+	r, w, err := os.Pipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+	oldStderr := os.Stderr
+	os.Stderr = w
+	defer func() { os.Stderr = oldStderr }()
+
+	fn()
+	w.Close()
+
+	var buf bytes.Buffer
+	_, _ = io.Copy(&buf, r)
+	return buf.String()
+}
+
+func TestEvent_Enabled(t *testing.T) {
+	setupEnabledTelemetry(t)
+	ctx := context.Background()
+	Event(ctx, "test.event", attribute.String("key", "value"))
+}
+
+func TestEvent_Disabled(t *testing.T) {
+	initialized = false
+	shutdownFuncs = nil
+	defer func() { initialized = false }()
+	Event(context.Background(), "test.event")
+}
+
+func TestEvent_NilCtx(t *testing.T) {
+	setupEnabledTelemetry(t)
+	Event(nil, "test.event") //nolint:staticcheck
+}
+
+func TestEventf_Enabled(t *testing.T) {
+	setupEnabledTelemetry(t)
+	ctx := context.Background()
+	Eventf(ctx, "test.eventf", "hello world", attribute.Int("count", 5))
+}
+
+func TestErrorEvent_Enabled(t *testing.T) {
+	setupEnabledTelemetry(t)
+	ctx := context.Background()
+	ErrorEvent(ctx, "test.error", errors.New("something failed"), attribute.String("detail", "extra"))
+}
+
+func TestErrorEvent_NilErr(t *testing.T) {
+	setupEnabledTelemetry(t)
+	ctx := context.Background()
+	ErrorEvent(ctx, "test.error", nil)
+}
+
+func TestErrorEvent_NilCtx(t *testing.T) {
+	setupEnabledTelemetry(t)
+	ErrorEvent(nil, "test.error", errors.New("fail")) //nolint:staticcheck
+}
+
+func TestErrorEvent_Disabled(t *testing.T) {
+	initialized = false
+	shutdownFuncs = nil
+	defer func() { initialized = false }()
+	ErrorEvent(context.Background(), "test.error", errors.New("fail"))
+}
+
+func TestPhaseEvent_Success(t *testing.T) {
+	setupEnabledTelemetry(t)
+	ctx := context.Background()
+	PhaseEvent(ctx, "scan", "main.go", 500*time.Millisecond, nil)
+}
+
+func TestPhaseEvent_WithError(t *testing.T) {
+	setupEnabledTelemetry(t)
+	ctx := context.Background()
+	PhaseEvent(ctx, "scan", "main.go", 500*time.Millisecond, errors.New("parse error"))
+}
+
+func TestPrintTraceSummary_WithTokenDetails(t *testing.T) {
+	PrintTraceSummary(5, 10, 1000, 200, 1200, 0, 0, 3*time.Second)
+}
+
+func TestPrintTraceSummary_WithCacheTokens(t *testing.T) {
+	PrintTraceSummary(3, 2, 500, 100, 600, 200, 50, 2*time.Second)
+}
+
+func TestPrintTraceSummary_NoTokenDetails(t *testing.T) {
+	PrintTraceSummary(2, 1, 0, 0, 500, 0, 0, 1*time.Second)
+}
+
+func TestPrintToolCallStarted_WithArgs(t *testing.T) {
+	PrintToolCallStarted("file_read", map[string]any{"path": "main.go"})
+}
+
+func TestPrintToolCallStarted_NoArgs(t *testing.T) {
+	PrintToolCallStarted("list_files", nil)
+}
+
+func TestPrintToolCallFinished(t *testing.T) {
+	PrintToolCallFinished("file_read", 123*time.Millisecond)
+}
+
+func TestPrintToolCallError(t *testing.T) {
+	out := captureStderr(t, func() {
+		PrintToolCallError("file_read", fmt.Errorf("permission denied"))
+	})
+	if !strings.Contains(out, "✘ file_read") {
+		t.Errorf("expected tool name with X mark, got %q", out)
+	}
+	if !strings.Contains(out, "permission denied") {
+		t.Errorf("expected error message, got %q", out)
+	}
+}
+
 func TestFormatDuration(t *testing.T) {
 	tests := []struct {
 		dur  time.Duration

internal/telemetry/exporter_test.go

@@ -0,0 +1,76 @@
+package telemetry
+
+import (
+	"context"
+	"testing"
+
+	"go.opentelemetry.io/otel/sdk/resource"
+)
+
+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)")
+	}
+}

internal/telemetry/metrics_test.go

@@ -24,7 +24,39 @@ func TestRecordFunctions_DisabledTelemetry(t *testing.T) {
 }
 
 func TestCheckMetricErr(t *testing.T) {
-	// Should not panic
 	checkMetricErr(nil)
 	checkMetricErr(fmt.Errorf("some error"))
 }
+
+func TestRecordFunctions_EnabledTelemetry(t *testing.T) {
+	setupEnabledTelemetry(t)
+	ctx := context.Background()
+
+	RecordReviewDuration(ctx, 5*time.Second)
+	RecordFilesReviewed(ctx, 10)
+	RecordCommentsGenerated(ctx, 3)
+	RecordLLMRequest(ctx, "gpt-4", 2*time.Second, 1000, "ok")
+	RecordLLMRequest(ctx, "gpt-4", 1*time.Second, 0, "error")
+	RecordToolCall(ctx, "file_read", 100*time.Millisecond, true)
+	RecordToolCall(ctx, "file_read", 200*time.Millisecond, false)
+}
+
+func TestEnsureMetrics_Idempotent(t *testing.T) {
+	setupEnabledTelemetry(t)
+	ensureMetrics()
+	ensureMetrics()
+	if mReviewDuration == nil {
+		t.Error("expected mReviewDuration to be initialized")
+	}
+	if mFilesReviewed == nil {
+		t.Error("expected mFilesReviewed to be initialized")
+	}
+}
+
+func TestGetMeter(t *testing.T) {
+	setupEnabledTelemetry(t)
+	m := getMeter()
+	if m == nil {
+		t.Error("expected non-nil meter")
+	}
+}

internal/telemetry/provider_test.go

@@ -2,6 +2,7 @@ package telemetry
 
 import (
 	"context"
+	"os"
 	"testing"
 )
 
@@ -50,3 +51,116 @@ func TestContentLogging_Disabled(t *testing.T) {
 		t.Error("expected ContentLogging()=false when telemetry disabled")
 	}
 }
+
+func TestContentLogging_EnabledWithEnv(t *testing.T) {
+	setupEnabledTelemetry(t)
+	t.Setenv("OCR_CONTENT_LOGGING", "1")
+	if !ContentLogging() {
+		t.Error("expected ContentLogging()=true when enabled and env var set")
+	}
+}
+
+func TestContentLogging_EnabledWithoutEnv(t *testing.T) {
+	setupEnabledTelemetry(t)
+	t.Setenv("OCR_CONTENT_LOGGING", "")
+	os.Unsetenv("OCR_CONTENT_LOGGING")
+	if ContentLogging() {
+		t.Error("expected ContentLogging()=false when enabled but env var not set")
+	}
+}
+
+func TestInit_AlreadyInitialized(t *testing.T) {
+	initialized = true
+	shutdownFuncs = nil
+	defer func() {
+		initialized = false
+		shutdownFuncs = nil
+	}()
+
+	result := Init(context.Background())
+	if result {
+		t.Error("expected false when already initialized with no shutdown funcs")
+	}
+}
+
+func TestInit_AlreadyInitializedWithShutdowns(t *testing.T) {
+	initialized = true
+	shutdownFuncs = []func(context.Context) error{
+		func(ctx context.Context) error { return nil },
+	}
+	defer func() {
+		initialized = false
+		shutdownFuncs = nil
+	}()
+
+	result := Init(context.Background())
+	if !result {
+		t.Error("expected true when already initialized with shutdown funcs")
+	}
+}
+
+func TestInit_DisabledByDefault(t *testing.T) {
+	initialized = false
+	shutdownFuncs = nil
+	tracerProvider = nil
+	meterProvider = nil
+
+	envKeys := []string{
+		"OCR_ENABLE_TELEMETRY", "OTEL_SERVICE_NAME",
+		"OTEL_EXPORTER_OTLP_ENDPOINT", "OTEL_EXPORTER_OTLP_PROTOCOL",
+		"OCR_CONTENT_LOGGING",
+	}
+	for _, k := range envKeys {
+		t.Setenv(k, "")
+		os.Unsetenv(k)
+	}
+
+	defer func() {
+		initialized = false
+		shutdownFuncs = nil
+		tracerProvider = nil
+		meterProvider = nil
+	}()
+
+	result := Init(context.Background())
+	if result {
+		t.Error("expected false when telemetry is not enabled via env")
+	}
+}
+
+func TestInit_EnabledConsole(t *testing.T) {
+	initialized = false
+	shutdownFuncs = nil
+	tracerProvider = nil
+	meterProvider = nil
+
+	t.Setenv("OCR_ENABLE_TELEMETRY", "1")
+	envKeys := []string{
+		"OTEL_EXPORTER_OTLP_ENDPOINT", "OTEL_EXPORTER_OTLP_PROTOCOL",
+	}
+	for _, k := range envKeys {
+		t.Setenv(k, "")
+		os.Unsetenv(k)
+	}
+
+	defer func() {
+		for _, fn := range shutdownFuncs {
+			_ = fn(context.Background())
+		}
+		initialized = false
+		shutdownFuncs = nil
+		tracerProvider = nil
+		meterProvider = nil
+	}()
+
+	result := Init(context.Background())
+	if !result {
+		t.Error("expected true when telemetry is enabled")
+	}
+	if tracerProvider == nil {
+		t.Error("expected tracerProvider to be set")
+	}
+	if meterProvider == nil {
+		t.Error("expected meterProvider to be set")
+	}
+}

internal/telemetry/shutdown_test.go

@@ -69,3 +69,12 @@ func TestShutdownWithTimeout(t *testing.T) {
 		t.Error("expected shutdown function to be called via ShutdownWithTimeout")
 	}
 }
+
+func TestShutdownWithTimeout_Error(t *testing.T) {
+	shutdownFuncs = []func(context.Context) error{
+		func(ctx context.Context) error { return errors.New("shutdown fail") },
+	}
+	defer func() { shutdownFuncs = nil }()
+
+	ShutdownWithTimeout(context.Background(), 5*time.Second)
+}

internal/telemetry/span_test.go

@@ -1,10 +1,13 @@
 package telemetry
 
 import (
+	"context"
+	"errors"
 	"fmt"
 	"testing"
 
 	"go.opentelemetry.io/otel/attribute"
+	"go.opentelemetry.io/otel/trace"
 )
 
 func TestAnyToAttr(t *testing.T) {
@@ -31,13 +34,107 @@ func TestAnyToAttr(t *testing.T) {
 	}
 }
 
+func TestStartSpan_Enabled(t *testing.T) {
+	setupEnabledTelemetry(t)
+	ctx := context.Background()
+	newCtx, span := StartSpan(ctx, "test.span")
+	defer span.End()
+	if newCtx == nil {
+		t.Error("expected non-nil context")
+	}
+	if !span.SpanContext().IsValid() {
+		t.Error("expected valid span context when telemetry is enabled")
+	}
+}
+
+func TestStartSpan_Disabled(t *testing.T) {
+	initialized = false
+	shutdownFuncs = nil
+	defer func() { initialized = false }()
+
+	ctx := context.Background()
+	newCtx, span := StartSpan(ctx, "test.span")
+	if newCtx != ctx {
+		t.Error("expected same context when disabled")
+	}
+	_ = span
+}
+
+func TestStartSpan_WithOptions(t *testing.T) {
+	setupEnabledTelemetry(t)
+	ctx := context.Background()
+	_, span := StartSpan(ctx, "test.span", trace.WithAttributes(attribute.String("key", "val")))
+	defer span.End()
+	if !span.SpanContext().IsValid() {
+		t.Error("expected valid span")
+	}
+}
+
+func TestEndSpan_NoError(t *testing.T) {
+	setupEnabledTelemetry(t)
+	_, span := StartSpan(context.Background(), "test.span")
+	EndSpan(span, nil)
+}
+
+func TestEndSpan_WithError(t *testing.T) {
+	setupEnabledTelemetry(t)
+	_, span := StartSpan(context.Background(), "test.span")
+	EndSpan(span, errors.New("something went wrong"))
+}
+
+func TestStartToolSpan_Enabled(t *testing.T) {
+	setupEnabledTelemetry(t)
+	ctx := context.Background()
+	newCtx, span := StartToolSpan(ctx, "file_read")
+	defer span.End()
+	if newCtx == nil {
+		t.Error("expected non-nil context")
+	}
+	if !span.SpanContext().IsValid() {
+		t.Error("expected valid span for tool")
+	}
+}
+
+func TestGetTracer(t *testing.T) {
+	setupEnabledTelemetry(t)
+	tracer := getTracer()
+	if tracer == nil {
+		t.Error("expected non-nil tracer")
+	}
+}
+
+func TestSetAttr_AllTypes(t *testing.T) {
+	setupEnabledTelemetry(t)
+	_, span := StartSpan(context.Background(), "test.setattr")
+	defer span.End()
+
+	SetAttr(span, "str", "hello")
+	SetAttr(span, "int", 42)
+	SetAttr(span, "int64", int64(100))
+	SetAttr(span, "bool", true)
+	SetAttr(span, "float64", 3.14)
+	SetAttr(span, "default", []int{1, 2})
+}
+
 func TestSetAttr_NilSpan(t *testing.T) {
-	// Should not panic
 	SetAttr(nil, "key", "value")
 }
 
+func TestRecordToolResult_Success(t *testing.T) {
+	setupEnabledTelemetry(t)
+	_, span := StartSpan(context.Background(), "test.tool")
+	RecordToolResult(span, "file_read", 123, nil)
+	span.End()
+}
+
+func TestRecordToolResult_Error(t *testing.T) {
+	setupEnabledTelemetry(t)
+	_, span := StartSpan(context.Background(), "test.tool")
+	RecordToolResult(span, "file_read", 456, errors.New("read failed"))
+	span.End()
+}
+
 func TestRecordToolResult_NilSpan(t *testing.T) {
-	// Should not panic
 	RecordToolResult(nil, "tool", 100, nil)
 	RecordToolResult(nil, "tool", 100, fmt.Errorf("err"))
 }