diff --git a/README.ja-JP.md b/README.ja-JP.md index c6bb4e2..42a8cbf 100644 --- a/README.ja-JP.md +++ b/README.ja-JP.md @@ -762,6 +762,15 @@ ocr config set telemetry.otlp_endpoint localhost:4317 エクスポートデータにLLMのプロンプトとレスポンスを含めるには、`telemetry.content_logging`を設定してください。 +**プロトコル選択：** 環境変数 `OTEL_EXPORTER_OTLP_PROTOCOL` でエクスポートプロトコルを選択できます： + +| 値 | トランスポート | 説明 | +|---|---|---| +| `grpc`（デフォルト） | gRPC | デフォルトポート 4317 | +| `http/protobuf` | HTTP | デフォルトポート 4318 | + +**Endpoint 形式：** `telemetry.otlp_endpoint` は `host:port` または `http://host:port` 形式のベースURLを指定します。パスを含める必要はありません。SDKが [OTLP仕様](https://opentelemetry.io/docs/specs/otlp/#otlphttp-request)に従いシグナルパス（例：`/v1/traces`）を自動的に付加します。 + ## コントリビューション 開発環境のセットアップ、コーディングガイドライン、プルリクエストの提出方法については[CONTRIBUTING.md](CONTRIBUTING.md)を参照してください。 diff --git a/README.ko-KR.md b/README.ko-KR.md index 74c67d9..05f709e 100644 --- a/README.ko-KR.md +++ b/README.ko-KR.md @@ -719,6 +719,15 @@ ocr config set telemetry.otlp_endpoint localhost:4317 exported data에 LLM prompt와 response를 포함하려면 `telemetry.content_logging`을 설정합니다. +**프로토콜 선택:** 환경 변수 `OTEL_EXPORTER_OTLP_PROTOCOL`로 export 프로토콜을 선택할 수 있습니다: + +| 값 | 전송 방식 | 설명 | +|---|---|---| +| `grpc` (기본값) | gRPC | 기본 포트 4317 | +| `http/protobuf` | HTTP | 기본 포트 4318 | + +**Endpoint 형식:** `telemetry.otlp_endpoint`는 `host:port` 또는 `http://host:port` 형식의 base URL을 지정합니다. 경로를 포함할 필요가 없습니다. SDK가 [OTLP 사양](https://opentelemetry.io/docs/specs/otlp/#otlphttp-request)에 따라 signal 경로(예: `/v1/traces`)를 자동으로 추가합니다. + ## Contributing 개발 환경 설정, coding guideline, pull request 제출 방법은 [CONTRIBUTING.ko-KR.md](CONTRIBUTING.ko-KR.md)를 참고하세요. diff --git a/README.md b/README.md index ba42e2d..45463ed 100644 --- a/README.md +++ b/README.md @@ -767,6 +767,16 @@ ocr config set telemetry.otlp_endpoint localhost:4317 Set `telemetry.content_logging` to include LLM prompts and responses in exported data. +**Protocol selection:** Set the environment variable `OTEL_EXPORTER_OTLP_PROTOCOL` to choose the export protocol: + +| Value | Transport | Notes | +|---|---|---| +| `grpc` (default) | gRPC | Default port 4317 | +| `http/protobuf` | HTTP | Default port 4318 | + +**Endpoint format:** `telemetry.otlp_endpoint` expects a base URL in `host:port` or `http://host:port` format, without a path component. The SDK appends the signal path (e.g. `/v1/traces`) automatically per the [OTLP specification](https://opentelemetry.io/docs/specs/otlp/#otlphttp-request). + + ## Contributing See [CONTRIBUTING.md](CONTRIBUTING.md) for development setup, coding guidelines, and how to submit pull requests. diff --git a/README.ru-RU.md b/README.ru-RU.md index 6535ce5..21dbdff 100644 --- a/README.ru-RU.md +++ b/README.ru-RU.md @@ -764,6 +764,15 @@ ocr config set telemetry.otlp_endpoint localhost:4317 Установите `telemetry.content_logging`, чтобы включать промпты и ответы LLM в экспортируемые данные. +**Выбор протокола:** Переменная окружения `OTEL_EXPORTER_OTLP_PROTOCOL` определяет протокол экспорта: + +| Значение | Транспорт | Описание | +|---|---|---| +| `grpc` (по умолчанию) | gRPC | Порт по умолчанию 4317 | +| `http/protobuf` | HTTP | Порт по умолчанию 4318 | + +**Формат endpoint:** `telemetry.otlp_endpoint` принимает базовый URL в формате `host:port` или `http://host:port` без компонента пути. SDK автоматически добавляет путь сигнала (например, `/v1/traces`) в соответствии со [спецификацией OTLP](https://opentelemetry.io/docs/specs/otlp/#otlphttp-request). + ## Участие в разработке В [CONTRIBUTING.ru-RU.md](CONTRIBUTING.ru-RU.md) описаны настройка окружения разработки, рекомендации по коду и порядок отправки pull request'ов. diff --git a/README.zh-CN.md b/README.zh-CN.md index 1f2c2e4..ddf3c30 100644 --- a/README.zh-CN.md +++ b/README.zh-CN.md @@ -752,6 +752,15 @@ ocr config set telemetry.otlp_endpoint localhost:4317 设置 `telemetry.content_logging` 可在导出数据中包含 LLM 提示词和响应。 +**协议选择：** 通过环境变量 `OTEL_EXPORTER_OTLP_PROTOCOL` 选择导出协议： + +| 值 | 传输方式 | 说明 | +|---|---|---| +| `grpc`（默认） | gRPC | 默认端口 4317 | +| `http/protobuf` | HTTP | 默认端口 4318 | + +**Endpoint 格式：** `telemetry.otlp_endpoint` 的值为 `host:port` 或 `http://host:port`，无需包含路径。SDK 会根据 [OTLP 规范](https://opentelemetry.io/docs/specs/otlp/#otlphttp-request)自动追加信号路径（如 `/v1/traces`）。 + ## 贡献 参见 [CONTRIBUTING.zh-CN.md](CONTRIBUTING.zh-CN.md) 了解开发环境搭建、编码规范以及如何提交 Pull Request。 diff --git a/cmd/opencodereview/emit_run_result_test.go b/cmd/opencodereview/emit_run_result_test.go index 4082ca2..3770fee 100644 --- a/cmd/opencodereview/emit_run_result_test.go +++ b/cmd/opencodereview/emit_run_result_test.go @@ -7,6 +7,9 @@ import ( "testing" "time" + "go.opentelemetry.io/otel" + sdktrace "go.opentelemetry.io/otel/sdk/trace" + "github.com/open-code-review/open-code-review/internal/agent" "github.com/open-code-review/open-code-review/internal/model" ) @@ -175,3 +178,61 @@ func TestEmitRunResult_NilQuietHandle(t *testing.T) { }) _ = got } + +func TestEmitRunResult_JSONTraceIDFromContext(t *testing.T) { + tp := sdktrace.NewTracerProvider() + defer tp.Shutdown(context.Background()) + otel.SetTracerProvider(tp) + + ctx, span := tp.Tracer("test").Start(context.Background(), "test-root") + wantTraceID := span.SpanContext().TraceID().String() + defer span.End() + + ag := &mockResultProvider{ + filesReviewed: 2, + inputTokens: 10, + outputTokens: 5, + totalTokens: 15, + } + got := captureStdout(t, func() { + err := emitRunResult(ctx, ag, nil, time.Now(), "json", "developer", nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + var out jsonOutput + if err := json.Unmarshal([]byte(got), &out); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if out.TraceID != wantTraceID { + t.Errorf("trace_id = %q, want %q", out.TraceID, wantTraceID) + } +} + +func TestEmitRunResult_JSONNoFilesTraceID(t *testing.T) { + tp := sdktrace.NewTracerProvider() + defer tp.Shutdown(context.Background()) + otel.SetTracerProvider(tp) + + ctx, span := tp.Tracer("test").Start(context.Background(), "test-root") + wantTraceID := span.SpanContext().TraceID().String() + defer span.End() + + ag := &mockResultProvider{filesReviewed: 0} + got := captureStdout(t, func() { + err := emitRunResult(ctx, ag, nil, time.Now(), "json", "developer", nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + var out jsonOutput + if err := json.Unmarshal([]byte(got), &out); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if out.Status != "skipped" { + t.Errorf("status = %q, want skipped", out.Status) + } + if out.TraceID != wantTraceID { + t.Errorf("trace_id = %q, want %q", out.TraceID, wantTraceID) + } +} diff --git a/cmd/opencodereview/output.go b/cmd/opencodereview/output.go index c87b696..cabeb78 100644 --- a/cmd/opencodereview/output.go +++ b/cmd/opencodereview/output.go @@ -240,6 +240,7 @@ type jsonToolCalls struct { type jsonOutput struct { Status string `json:"status"` + TraceID string `json:"trace_id,omitempty"` Message string `json:"message,omitempty"` Summary *jsonSummary `json:"summary,omitempty"` ToolCalls *jsonToolCalls `json:"tool_calls"` @@ -263,9 +264,10 @@ func outputJSON(comments []model.LlmComment) error { func outputJSONWithWarnings(comments []model.LlmComment, warnings []agent.AgentWarning, filesReviewed, inputTokens, outputTokens, totalTokens, cacheReadTokens, cacheWriteTokens int64, - duration time.Duration, projectSummary string, toolCalls map[string]int64) error { + duration time.Duration, projectSummary string, toolCalls map[string]int64, traceID string) error { out := jsonOutput{ Status: "success", + TraceID: traceID, Comments: comments, Summary: &jsonSummary{ FilesReviewed: filesReviewed, @@ -311,9 +313,10 @@ func outputJSONWithWarnings(comments []model.LlmComment, warnings []agent.AgentW return enc.Encode(out) } -func outputJSONNoFiles() error { +func outputJSONNoFiles(traceID string) error { out := jsonOutput{ Status: "skipped", + TraceID: traceID, Message: "No supported files changed.", Comments: []model.LlmComment{}, ToolCalls: &jsonToolCalls{ diff --git a/cmd/opencodereview/output_helpers_test.go b/cmd/opencodereview/output_helpers_test.go index d0d25b2..f3d9127 100644 --- a/cmd/opencodereview/output_helpers_test.go +++ b/cmd/opencodereview/output_helpers_test.go @@ -168,8 +168,7 @@ func TestOutputJSONWithWarnings_NoCommentsSubtaskError(t *testing.T) { os.Stdout = w warnings := []agent.AgentWarning{{Type: "subtask_error", File: "x.go", Message: "fail"}} - err := outputJSONWithWarnings(nil, warnings, 1, 10, 5, 15, 0, 0, time.Second, "", nil) - + err := outputJSONWithWarnings(nil, warnings, 1, 10, 5, 15, 0, 0, time.Second, "", nil, "abc123trace") w.Close() os.Stdout = old @@ -188,6 +187,9 @@ func TestOutputJSONWithWarnings_NoCommentsSubtaskError(t *testing.T) { if !strings.Contains(out.Message, "errors") { t.Errorf("message = %q, expected to mention errors", out.Message) } + if out.TraceID != "abc123trace" { + t.Errorf("trace_id = %q, want abc123trace", out.TraceID) + } } func TestStatusBadge(t *testing.T) { @@ -275,8 +277,7 @@ func TestOutputJSONWithWarnings(t *testing.T) { comments := []model.LlmComment{{Path: "b.go", Content: "test"}} warnings := []agent.AgentWarning{{Type: "subtask_error", File: "c.go", Message: "failed"}} - err := outputJSONWithWarnings(comments, warnings, 5, 100, 50, 150, 10, 5, 3*time.Second, "summary", map[string]int64{"file_read": 3}) - + err := outputJSONWithWarnings(comments, warnings, 5, 100, 50, 150, 10, 5, 3*time.Second, "summary", map[string]int64{"file_read": 3}, "trace-xyz-789") w.Close() os.Stdout = old @@ -301,6 +302,9 @@ func TestOutputJSONWithWarnings(t *testing.T) { if out.ToolCalls == nil || out.ToolCalls.Total != 3 { t.Errorf("ToolCalls.Total = %v", out.ToolCalls) } + if out.TraceID != "trace-xyz-789" { + t.Errorf("trace_id = %q, want trace-xyz-789", out.TraceID) + } } func TestOutputJSONWithWarnings_NoCommentsNoErrors(t *testing.T) { @@ -309,8 +313,7 @@ func TestOutputJSONWithWarnings_NoCommentsNoErrors(t *testing.T) { os.Stdout = w warnings := []agent.AgentWarning{{Type: "warning", Message: "something"}} - err := outputJSONWithWarnings(nil, warnings, 2, 50, 20, 70, 0, 0, time.Second, "", nil) - + err := outputJSONWithWarnings(nil, warnings, 2, 50, 20, 70, 0, 0, time.Second, "", nil, "") w.Close() os.Stdout = old @@ -336,7 +339,7 @@ func TestOutputJSONNoFiles(t *testing.T) { r, w, _ := os.Pipe() os.Stdout = w - err := outputJSONNoFiles() + err := outputJSONNoFiles("test-trace-id-456") w.Close() os.Stdout = old @@ -353,6 +356,9 @@ func TestOutputJSONNoFiles(t *testing.T) { if out.Status != "skipped" { t.Errorf("status = %q, want skipped", out.Status) } + if out.TraceID != "test-trace-id-456" { + t.Errorf("trace_id = %q, want test-trace-id-456", out.TraceID) + } } func captureStdout(t *testing.T, fn func()) string { diff --git a/cmd/opencodereview/review_cmd.go b/cmd/opencodereview/review_cmd.go index 118d398..47a026d 100644 --- a/cmd/opencodereview/review_cmd.go +++ b/cmd/opencodereview/review_cmd.go @@ -13,6 +13,8 @@ import ( "github.com/open-code-review/open-code-review/internal/mcp" "github.com/open-code-review/open-code-review/internal/telemetry" "github.com/open-code-review/open-code-review/internal/tool" + + "go.opentelemetry.io/otel/codes" ) func runReview(args []string) error { @@ -104,11 +106,23 @@ func runReview(args []string) error { ctx, span := telemetry.StartSpan(context.Background(), "review.run") defer span.End() + telemetry.SetAttr(span, "review.repo", cc.RepoDir) + telemetry.SetAttr(span, "review.from", opts.from) + telemetry.SetAttr(span, "review.to", opts.to) + telemetry.SetAttr(span, "review.model", rt.Model) + var traceID string + if telemetry.IsEnabled() { + traceID = telemetry.TraceIDFromContext(ctx) + if opts.outputFormat != "json" { + fmt.Fprintf(os.Stderr, "[ocr] TraceID: %s\n", traceID) + } + } startTime := time.Now() comments, err := ag.Run(ctx) if err != nil { - telemetry.SetAttr(span, "error", err.Error()) + span.SetStatus(codes.Error, err.Error()) + span.RecordError(err) return fmt.Errorf("review failed: %w", err) } diff --git a/cmd/opencodereview/scan_cmd.go b/cmd/opencodereview/scan_cmd.go index 3757e98..c383579 100644 --- a/cmd/opencodereview/scan_cmd.go +++ b/cmd/opencodereview/scan_cmd.go @@ -3,6 +3,7 @@ package main import ( "context" "fmt" + "os" "strings" "time" @@ -11,6 +12,8 @@ import ( "github.com/open-code-review/open-code-review/internal/scan" "github.com/open-code-review/open-code-review/internal/telemetry" "github.com/open-code-review/open-code-review/internal/tool" + + "go.opentelemetry.io/otel/codes" ) // scanOptions mirrors reviewOptions for the full-scan subcommand. The two @@ -207,11 +210,19 @@ func runScan(args []string) error { ctx, span := telemetry.StartSpan(context.Background(), "scan.run") defer span.End() + var traceID string + if telemetry.IsEnabled() { + traceID = telemetry.TraceIDFromContext(ctx) + if opts.outputFormat != "json" { + fmt.Fprintf(os.Stderr, "[ocr] TraceID: %s\n", traceID) + } + } startTime := time.Now() comments, err := ag.Run(ctx) if err != nil { - telemetry.SetAttr(span, "error", err.Error()) + span.SetStatus(codes.Error, err.Error()) + span.RecordError(err) return fmt.Errorf("scan failed: %w", err) } diff --git a/cmd/opencodereview/shared.go b/cmd/opencodereview/shared.go index eaacf83..4d70c84 100644 --- a/cmd/opencodereview/shared.go +++ b/cmd/opencodereview/shared.go @@ -276,8 +276,10 @@ func emitRunResult( telemetry.RecordCommentsGenerated(ctx, int64(len(comments))) } + traceID := telemetry.TraceIDFromContext(ctx) + if outputFormat == "json" && len(comments) == 0 && ag.FilesReviewed() == 0 { - return outputJSONNoFiles() + return outputJSONNoFiles(traceID) } // Agent-text audiences need stdout back before PrintTraceSummary so the @@ -296,7 +298,7 @@ func emitRunResult( return outputJSONWithWarnings(comments, ag.Warnings(), ag.FilesReviewed(), ag.TotalInputTokens(), ag.TotalOutputTokens(), ag.TotalTokensUsed(), ag.TotalCacheReadTokens(), ag.TotalCacheWriteTokens(), duration, - ag.ProjectSummary(), ag.ToolCalls()) + ag.ProjectSummary(), ag.ToolCalls(), traceID) } outputTextWithWarnings(comments, ag.Warnings()) if summary := ag.ProjectSummary(); summary != "" { diff --git a/go.mod b/go.mod index 88d5e03..42a346c 100644 --- a/go.mod +++ b/go.mod @@ -13,7 +13,9 @@ require ( github.com/pkoukk/tiktoken-go v0.1.8 go.opentelemetry.io/otel v1.44.0 go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc v1.44.0 + go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp v1.44.0 go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.44.0 + go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.44.0 go.opentelemetry.io/otel/exporters/stdout/stdoutmetric v1.44.0 go.opentelemetry.io/otel/exporters/stdout/stdouttrace v1.44.0 go.opentelemetry.io/otel/metric v1.44.0 diff --git a/go.sum b/go.sum index cb21c84..071f834 100644 --- a/go.sum +++ b/go.sum @@ -109,10 +109,14 @@ go.opentelemetry.io/otel v1.44.0 h1:JjwHmHpA4iZ3wBxluu2fbbE7j4kqlE8jXyAyPXH7HqU= go.opentelemetry.io/otel v1.44.0/go.mod h1:BMgjTHL9WPRlRjL2oZCBTL4whCGtXch2H4BhOPIAyYc= go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc v1.44.0 h1:SUplec5dp06reu1zaXmOXdvqH398taqrDXqUl99jxSc= go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc v1.44.0/go.mod h1:ho2g4N+ane+swq5I/VBkKWnRDY4kUINH3FuqyZqX/Ug= +go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp v1.44.0 h1:RuynHbfU8JUEw7DyONgkVYg2SVtsoF28y0LGIr69jgA= +go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp v1.44.0/go.mod h1:qZF+/lBs71APw8mlnEZcqZHMzqrYrsFiJOv83lX1OGo= go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.44.0 h1:4YsVu3B8+3qtWYYrsUYgn0OG78pN0rnNPRGX4SbokQI= go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.44.0/go.mod h1:+wnlSn0mD1ADVMe3v9Z/WIaiz6q6gL2J/ejaAmdmv80= go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.44.0 h1:qazEJlUOQzhCpzQpFETGby7EdqjI1wsd0W+6Gg1SCTU= go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.44.0/go.mod h1:fOD2Yefuxixkx3ahVNf0O/PERb6r4OlbxfATVnYvzCo= +go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.44.0 h1:lgh3PiVrRUWMLOVSkQicxzZll5NjF1r+AtsX1XRIHw0= +go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.44.0/go.mod h1:5Cnhth3m/AgOeTgE3ex12pPmiu/gGtZit03kSzx9X7s= go.opentelemetry.io/otel/exporters/stdout/stdoutmetric v1.44.0 h1:hqxVTu/GtBF+vJ8d1fzW7fRxZFvgoDjWcxwwCaFDYpU= go.opentelemetry.io/otel/exporters/stdout/stdoutmetric v1.44.0/go.mod h1:z5fVEF4X5v0ESvlJqBrrFlBVoj5EQuefZpzsu7R+x5Q= go.opentelemetry.io/otel/exporters/stdout/stdouttrace v1.44.0 h1:bl2S7Ubua0Nms+D/gAmznQTd4dxxMA93aKbcpKqiTCs= diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 4be6504..e7e02c9 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -22,6 +22,8 @@ import ( "github.com/open-code-review/open-code-review/internal/stdout" "github.com/open-code-review/open-code-review/internal/telemetry" "github.com/open-code-review/open-code-review/internal/tool" + + "go.opentelemetry.io/otel/codes" ) // AgentWarning is re-exported from llmloop for backwards compatibility with @@ -487,7 +489,17 @@ func (a *Agent) executeSubtask(ctx context.Context, d model.Diff) error { return nil } - err := a.runner.RunPerFile(ctx, messages, newPath) + err := func() error { + ctx, mainSpan := telemetry.StartSpan(ctx, "main.loop") + defer mainSpan.End() + telemetry.SetAttr(mainSpan, "file.path", newPath) + if err := a.runner.RunPerFile(ctx, messages, newPath); err != nil { + mainSpan.SetStatus(codes.Error, err.Error()) + mainSpan.RecordError(err) + return err + } + return nil + }() if err == nil { // REVIEW_FILTER_TASK runs after the main loop and decides which of the // just-collected comments to drop. It needs to see comments produced by @@ -503,6 +515,10 @@ func (a *Agent) executeSubtask(ctx context.Context, d model.Diff) error { // executeReviewFilter runs the REVIEW_FILTER_TASK to remove comments that are // provably incorrect based solely on the diff. Errors are logged and silently ignored. func (a *Agent) executeReviewFilter(ctx context.Context, d model.Diff, newPath string) { + ctx, span := telemetry.StartSpan(ctx, "review_filter.execute") + defer span.End() + telemetry.SetAttr(span, "file.path", newPath) + ft := a.args.Template.ReviewFilterTask if ft == nil || len(ft.Messages) == 0 { return @@ -512,6 +528,7 @@ func (a *Agent) executeReviewFilter(ctx context.Context, d model.Diff, newPath s if len(comments) == 0 { return } + telemetry.SetAttr(span, "comments.before", len(comments)) commentsJSON := buildFilterCommentsJSON(comments) @@ -534,20 +551,33 @@ func (a *Agent) executeReviewFilter(ctx context.Context, d model.Diff, newPath s rec := fs.AppendTaskRecord(session.ReviewFilterTask, messages) startTime := time.Now() + _, llmSpan := telemetry.StartLLMSpan(ctx, a.args.Model) resp, err := a.args.LLMClient.CompletionsWithCtx(ctx, llm.ChatRequest{ Model: a.args.Model, Messages: messages, MaxTokens: a.args.Template.MaxTokens, }) + duration := time.Since(startTime) if err != nil { - rec.SetError(err, time.Since(startTime)) + telemetry.RecordLLMResult(llmSpan, duration, 0, err) + llmSpan.End() + rec.SetError(err, duration) fmt.Fprintf(stdout.Writer(), "[ocr] Review filter failed for %s: %v\n", newPath, err) + span.SetStatus(codes.Error, err.Error()) + span.RecordError(err) return } - rec.SetResponse(resp, time.Since(startTime)) + var totalTokens int64 + if resp.Usage != nil { + totalTokens = resp.Usage.TotalTokens + } + telemetry.RecordLLMResult(llmSpan, duration, totalTokens, nil) + llmSpan.End() + rec.SetResponse(resp, duration) a.runner.RecordUsage(resp.Usage) indices := parseFilterResponse(resp.Content(), len(comments)) + telemetry.SetAttr(span, "comments.filtered", len(indices)) if len(indices) == 0 { return } @@ -722,6 +752,10 @@ func (a *Agent) extFromPath(path string) string { // executePlanPhase runs the plan task for a single file, sending template messages // with resolved placeholders and collecting the LLM response as plan guidance. func (a *Agent) executePlanPhase(ctx context.Context, newPath, rawDiff, changeFiles, rule string) (string, error) { + ctx, span := telemetry.StartSpan(ctx, "plan.execute") + defer span.End() + telemetry.SetAttr(span, "file.path", newPath) + pt := a.args.Template.PlanTask messages := make([]llm.Message, 0, len(pt.Messages)) for _, m := range pt.Messages { @@ -740,16 +774,28 @@ func (a *Agent) executePlanPhase(ctx context.Context, newPath, rawDiff, changeFi rec := fs.AppendTaskRecord(session.PlanTask, messages) startTime := time.Now() + _, llmSpan := telemetry.StartLLMSpan(ctx, a.args.Model) resp, err := a.args.LLMClient.CompletionsWithCtx(ctx, llm.ChatRequest{ Model: a.args.Model, Messages: messages, MaxTokens: a.args.Template.MaxTokens, }) + duration := time.Since(startTime) if err != nil { - rec.SetError(err, time.Since(startTime)) + telemetry.RecordLLMResult(llmSpan, duration, 0, err) + llmSpan.End() + rec.SetError(err, duration) + span.SetStatus(codes.Error, err.Error()) + span.RecordError(err) return "", fmt.Errorf("plan request: %w", err) } - rec.SetResponse(resp, time.Since(startTime)) + var totalTokens int64 + if resp.Usage != nil { + totalTokens = resp.Usage.TotalTokens + } + telemetry.RecordLLMResult(llmSpan, duration, totalTokens, nil) + llmSpan.End() + rec.SetResponse(resp, duration) a.runner.RecordUsage(resp.Usage) fmt.Fprintf(stdout.Writer(), "[ocr] Plan completed for %s\n", newPath) return resp.Content(), nil diff --git a/internal/diff/relocation.go b/internal/diff/relocation.go index 1f296e3..d1afff5 100644 --- a/internal/diff/relocation.go +++ b/internal/diff/relocation.go @@ -10,6 +10,7 @@ import ( "github.com/open-code-review/open-code-review/internal/llm" "github.com/open-code-review/open-code-review/internal/model" "github.com/open-code-review/open-code-review/internal/stdout" + "github.com/open-code-review/open-code-review/internal/telemetry" ) // ReLocateComment calls the LLM to regenerate a precise existing_code snippet @@ -44,15 +45,26 @@ func ReLocateComment( messages = append(messages, llm.NewTextMessage(m.Role, content)) } + startTime := time.Now() + _, llmSpan := telemetry.StartLLMSpan(ctx, modelName) resp, err := client.CompletionsWithCtx(ctx, llm.ChatRequest{ Model: modelName, Messages: messages, MaxTokens: maxTokens, }) + duration := time.Since(startTime) if err != nil { + telemetry.RecordLLMResult(llmSpan, duration, 0, err) + llmSpan.End() fmt.Fprintf(stdout.Writer(), "[ocr] Re-location LLM call failed for %s: %v\n", cm.Path, err) return false, nil, messages } + var totalTokens int64 + if resp.Usage != nil { + totalTokens = resp.Usage.TotalTokens + } + telemetry.RecordLLMResult(llmSpan, duration, totalTokens, nil) + llmSpan.End() code := extractCodeBlock(resp.Content()) if code == "" { diff --git a/internal/llmloop/loop.go b/internal/llmloop/loop.go index d206a78..38d7b3f 100644 --- a/internal/llmloop/loop.go +++ b/internal/llmloop/loop.go @@ -163,6 +163,7 @@ func (r *Runner) RunPerFile(ctx context.Context, messages []llm.Message, newPath rec := fs.AppendTaskRecord(session.MainTask, append([]llm.Message(nil), messages...)) startTime := time.Now() + _, llmSpan := telemetry.StartLLMSpan(ctx, r.deps.Model) resp, err := r.deps.LLMClient.CompletionsWithCtx(ctx, llm.ChatRequest{ Model: r.deps.Model, Messages: messages, @@ -172,6 +173,8 @@ func (r *Runner) RunPerFile(ctx context.Context, messages []llm.Message, newPath duration := time.Since(startTime) if err != nil { rec.SetError(err, duration) + telemetry.RecordLLMResult(llmSpan, duration, 0, err) + llmSpan.End() telemetry.RecordLLMRequest(ctx, r.deps.Model, duration, 0, "error") return fmt.Errorf("LLM completion error: %w", err) } @@ -184,6 +187,8 @@ func (r *Runner) RunPerFile(ctx context.Context, messages []llm.Message, newPath atomic.AddInt64(&r.totalCacheReadTokens, resp.Usage.CacheReadTokens) atomic.AddInt64(&r.totalCacheWriteTokens, resp.Usage.CacheWriteTokens) } + telemetry.RecordLLMResult(llmSpan, duration, totalTokens, nil) + llmSpan.End() telemetry.RecordLLMRequest(ctx, r.deps.Model, duration, totalTokens, "ok") content := resp.Content() @@ -272,14 +277,19 @@ func (r *Runner) executeToolCall(ctx context.Context, newPath string, call llm.T return tool.Of(fmt.Sprintf("Error parsing tool arguments for %s: %v", call.Function.Name, err)) } telemetry.PrintToolCallStarted(call.Function.Name, dynArgs) + _, toolSpan := telemetry.StartToolSpan(ctx, call.Function.Name) startTime := time.Now() result, err := p.Execute(ctx, dynArgs) dur := time.Since(startTime) if err != nil { + telemetry.RecordToolResult(toolSpan, call.Function.Name, dur.Milliseconds(), err) + toolSpan.End() telemetry.RecordToolCall(ctx, call.Function.Name, dur, false) telemetry.PrintToolCallError(call.Function.Name, err) return tool.Of(fmt.Sprintf("Error executing tool %s: %v", call.Function.Name, err)) } + telemetry.RecordToolResult(toolSpan, call.Function.Name, dur.Milliseconds(), nil) + toolSpan.End() telemetry.RecordToolCall(ctx, call.Function.Name, dur, true) telemetry.PrintToolCallFinished(call.Function.Name, dur) if rec != nil { @@ -314,10 +324,14 @@ func (r *Runner) executeToolCall(ctx context.Context, newPath string, call llm.T if t == tool.CodeComment { telemetry.PrintToolCallStarted(t.Name(), args) + _, toolSpan := telemetry.StartToolSpan(ctx, t.Name()) comments, errMsg := tool.ParseComments(args) if errMsg != "" { - telemetry.RecordToolCall(ctx, t.Name(), time.Since(startTime), false) + dur := time.Since(startTime) + telemetry.RecordToolResult(toolSpan, t.Name(), dur.Milliseconds(), fmt.Errorf("%s", errMsg)) + toolSpan.End() + telemetry.RecordToolCall(ctx, t.Name(), dur, false) return tool.Of(errMsg) } @@ -361,8 +375,13 @@ func (r *Runner) executeToolCall(ctx context.Context, newPath string, call llm.T asyncCtx := context.WithoutCancel(ctx) toolName := t.Name() pool.Submit(func() ([]model.LlmComment, error) { + defer func() { + dur := time.Since(startTime) + telemetry.RecordToolResult(toolSpan, toolName, dur.Milliseconds(), nil) + toolSpan.End() + telemetry.PrintToolCallFinished(toolName, dur) + }() resolveAndCollect(asyncCtx) - telemetry.PrintToolCallFinished(toolName, time.Since(startTime)) return []model.LlmComment{}, nil }) telemetry.RecordToolCall(asyncCtx, toolName, time.Since(startTime), true) @@ -371,6 +390,8 @@ func (r *Runner) executeToolCall(ctx context.Context, newPath string, call llm.T resolveAndCollect(ctx) dur := time.Since(startTime) + telemetry.RecordToolResult(toolSpan, t.Name(), dur.Milliseconds(), nil) + toolSpan.End() telemetry.RecordToolCall(ctx, t.Name(), dur, true) telemetry.PrintToolCallFinished(t.Name(), dur) if rec != nil { @@ -381,9 +402,12 @@ func (r *Runner) executeToolCall(ctx context.Context, newPath string, call llm.T // Synchronous path for all other tools telemetry.PrintToolCallStarted(t.Name(), args) + _, toolSpan := telemetry.StartToolSpan(ctx, t.Name()) result, err := p.Execute(ctx, args) dur := time.Since(startTime) ok := err == nil + telemetry.RecordToolResult(toolSpan, t.Name(), dur.Milliseconds(), err) + toolSpan.End() telemetry.RecordToolCall(ctx, t.Name(), dur, ok) if err != nil { diff --git a/internal/telemetry/config_test.go b/internal/telemetry/config_test.go index 4edb8ce..1e4011a 100644 --- a/internal/telemetry/config_test.go +++ b/internal/telemetry/config_test.go @@ -65,7 +65,7 @@ func TestResolveEnv(t *testing.T) { }, }, { - name: "otlp protocol override", + name: "otlp protocol passthrough", envs: map[string]string{"OTEL_EXPORTER_OTLP_PROTOCOL": "http/json"}, check: func(t *testing.T, cfg Config) { if cfg.OTLPProtocol != "http/json" { diff --git a/internal/telemetry/exporter.go b/internal/telemetry/exporter.go index 58638c8..bcb2b45 100644 --- a/internal/telemetry/exporter.go +++ b/internal/telemetry/exporter.go @@ -11,7 +11,9 @@ import ( sdktrace "go.opentelemetry.io/otel/sdk/trace" otlpmetricgrpc "go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc" + otlpmetrichttp "go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp" otlptracegrpc "go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc" + otlptracehttp "go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp" stdoutmetric "go.opentelemetry.io/otel/exporters/stdout/stdoutmetric" stdouttrace "go.opentelemetry.io/otel/exporters/stdout/stdouttrace" ) @@ -27,11 +29,11 @@ func newStdoutMetricExporter() (sdkmetric.Exporter, error) { } // parseOTLPEndpoint strips a http:// or https:// scheme from the endpoint and -// reports whether the connection should be insecure (plaintext gRPC). +// reports whether the connection should be plaintext (insecure). // Scheme matching is case-insensitive per RFC 3986. A bare host:port (no // scheme) is left unchanged and defaults to TLS. Any trailing slash left // over from a URL-style endpoint (e.g. "http://localhost:4317/") is -// trimmed, since otlptracegrpc/otlpmetricgrpc's WithEndpoint expects a bare +// trimmed, since both gRPC and HTTP exporters' WithEndpoint expects a bare // host:port with no path. func parseOTLPEndpoint(endpoint string) (addr string, insecure bool) { switch { @@ -44,8 +46,21 @@ func parseOTLPEndpoint(endpoint string) (addr string, insecure bool) { } } -// initOTLPProviders sets up OTLP gRPC exporters for traces and metrics. +// initOTLPProviders dispatches to the gRPC or HTTP exporter based on cfg.OTLPProtocol. func initOTLPProviders(ctx context.Context, res *resource.Resource, cfg Config) { + switch cfg.OTLPProtocol { + case "http/protobuf", "http/json": + initOTLPHTTPProviders(ctx, res, cfg) + case "", "grpc": + initOTLPGRPCProviders(ctx, res, cfg) + default: + fmt.Fprintf(os.Stderr, "[ocr] WARNING: unsupported OTLP protocol %q, falling back to gRPC\n", cfg.OTLPProtocol) + initOTLPGRPCProviders(ctx, res, cfg) + } +} + +// initOTLPGRPCProviders sets up OTLP gRPC exporters for traces and metrics. +func initOTLPGRPCProviders(ctx context.Context, res *resource.Resource, cfg Config) { addr, insecure := parseOTLPEndpoint(cfg.OTLPEndpoint) traceOpts := []otlptracegrpc.Option{otlptracegrpc.WithEndpoint(addr)} @@ -83,6 +98,45 @@ func initOTLPProviders(ctx context.Context, res *resource.Resource, cfg Config) shutdownFuncs = append(shutdownFuncs, func(ctx context.Context) error { return mp.Shutdown(ctx) }) } +// initOTLPHTTPProviders sets up OTLP HTTP exporters for traces and metrics. +func initOTLPHTTPProviders(ctx context.Context, res *resource.Resource, cfg Config) { + addr, insecure := parseOTLPEndpoint(cfg.OTLPEndpoint) + + traceOpts := []otlptracehttp.Option{otlptracehttp.WithEndpoint(addr)} + if insecure { + traceOpts = append(traceOpts, otlptracehttp.WithInsecure()) + } + traceExp, err := otlptracehttp.New(ctx, traceOpts...) + if err != nil { + fmt.Fprintf(os.Stderr, "[ocr] WARNING: failed to create OTLP HTTP trace exporter: %v\n", err) + return + } + + tp := sdktrace.NewTracerProvider( + sdktrace.WithBatcher(traceExp), + sdktrace.WithResource(res), + ) + tracerProvider = tp + shutdownFuncs = append(shutdownFuncs, func(ctx context.Context) error { return tp.Shutdown(ctx) }) + + metricOpts := []otlpmetrichttp.Option{otlpmetrichttp.WithEndpoint(addr)} + if insecure { + metricOpts = append(metricOpts, otlpmetrichttp.WithInsecure()) + } + metricExp, err := otlpmetrichttp.New(ctx, metricOpts...) + if err != nil { + fmt.Fprintf(os.Stderr, "[ocr] WARNING: failed to create OTLP HTTP metric exporter: %v\n", err) + return + } + + mp := sdkmetric.NewMeterProvider( + sdkmetric.WithReader(sdkmetric.NewPeriodicReader(metricExp)), + sdkmetric.WithResource(res), + ) + meterProvider = mp + shutdownFuncs = append(shutdownFuncs, func(ctx context.Context) error { return mp.Shutdown(ctx) }) +} + // initConsoleProviders sets up stdout exporters for traces and metrics. func initConsoleProviders(res *resource.Resource) { traceExp, err := newStdoutTraceExporter() diff --git a/internal/telemetry/exporter_test.go b/internal/telemetry/exporter_test.go index 95efd4b..1a3c9be 100644 --- a/internal/telemetry/exporter_test.go +++ b/internal/telemetry/exporter_test.go @@ -1,7 +1,11 @@ package telemetry import ( + "bytes" "context" + "io" + "os" + "strings" "testing" "go.opentelemetry.io/otel/sdk/resource" @@ -103,3 +107,94 @@ func TestInitOTLPProviders_InvalidEndpoint(t *testing.T) { 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) + } + } + }) + } +} diff --git a/internal/telemetry/span.go b/internal/telemetry/span.go index 04c9d4d..0b412ed 100644 --- a/internal/telemetry/span.go +++ b/internal/telemetry/span.go @@ -3,6 +3,7 @@ package telemetry import ( "context" "fmt" + "time" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" @@ -23,6 +24,16 @@ func StartSpan(ctx context.Context, name string, opts ...trace.SpanStartOption) return getTracer().Start(ctx, name, opts...) } +// TraceIDFromContext returns the hex-encoded trace ID of the span carried by +// ctx, or "" if ctx carries no valid span (e.g. telemetry is disabled). +func TraceIDFromContext(ctx context.Context) string { + sc := trace.SpanContextFromContext(ctx) + if !sc.TraceID().IsValid() { + return "" + } + return sc.TraceID().String() +} + // EndSpan ends the span and records error status if present. func EndSpan(span trace.Span, err error) { if err != nil { @@ -69,11 +80,35 @@ func RecordToolResult(span trace.Span, toolName string, durationMs int64, err er SetAttr(span, "tool.status", "error") SetAttr(span, "tool.error", err.Error()) span.SetStatus(codes.Error, err.Error()) + span.RecordError(err) } else { SetAttr(span, "tool.status", "ok") } } +// StartLLMSpan creates a span for an LLM request with standard attributes. +func StartLLMSpan(ctx context.Context, model string) (context.Context, trace.Span) { + return StartSpan(ctx, "llm.request", + trace.WithAttributes(attribute.String("llm.model", model))) +} + +// RecordLLMResult sets the outcome of an LLM request on the span. +func RecordLLMResult(span trace.Span, duration time.Duration, totalTokens int64, err error) { + if span == nil { + return + } + SetAttr(span, "llm.duration_ms", duration.Milliseconds()) + SetAttr(span, "llm.total_tokens", totalTokens) + if err != nil { + SetAttr(span, "llm.status", "error") + SetAttr(span, "llm.error", err.Error()) + span.SetStatus(codes.Error, err.Error()) + span.RecordError(err) + } else { + SetAttr(span, "llm.status", "ok") + } +} + // AnyToAttr converts an arbitrary value to an OTel attribute.KeyValue. func AnyToAttr(k string, v interface{}) attribute.KeyValue { switch val := v.(type) { diff --git a/internal/telemetry/span_test.go b/internal/telemetry/span_test.go index a8ca9d1..089aac0 100644 --- a/internal/telemetry/span_test.go +++ b/internal/telemetry/span_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "testing" + "time" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/trace" @@ -138,3 +139,34 @@ func TestRecordToolResult_NilSpan(t *testing.T) { RecordToolResult(nil, "tool", 100, nil) RecordToolResult(nil, "tool", 100, fmt.Errorf("err")) } + +func TestStartLLMSpan(t *testing.T) { + setupEnabledTelemetry(t) + ctx, span := StartLLMSpan(context.Background(), "qwen-max") + if !span.SpanContext().IsValid() { + t.Error("expected valid span context") + } + if ctx == nil { + t.Error("expected non-nil context") + } + span.End() +} + +func TestRecordLLMResult_Success(t *testing.T) { + setupEnabledTelemetry(t) + _, span := StartSpan(context.Background(), "test.llm") + RecordLLMResult(span, 500*time.Millisecond, 1200, nil) + span.End() +} + +func TestRecordLLMResult_Error(t *testing.T) { + setupEnabledTelemetry(t) + _, span := StartSpan(context.Background(), "test.llm") + RecordLLMResult(span, 100*time.Millisecond, 0, errors.New("timeout")) + span.End() +} + +func TestRecordLLMResult_NilSpan(t *testing.T) { + RecordLLMResult(nil, 100*time.Millisecond, 0, nil) + RecordLLMResult(nil, 100*time.Millisecond, 0, fmt.Errorf("err")) +}