feat: add logs support to otel-cli #18
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive OpenTelemetry logs support to otel-cli, bringing it to feature parity with the existing traces functionality. The implementation follows OTel specifications for signal-specific endpoints and establishes patterns for future metrics support.
Key Changes
- New
otel-cli logcommand with body, severity, and attribute support - Refactored server architecture to handle multiple signals (traces and logs) with shared state
- Signal-specific endpoint handling following OTel spec (
--logs-endpoint)
Reviewed Changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| otelcli/log.go | Implements the new log command with body and severity parameters |
| otelcli/config_log.go | Log record creation and configuration mapping |
| otelcli/config.go | Adds logs endpoint parsing and log-specific config fields |
| otelcli/root.go | Registers log command and adds service parameter refactoring |
| otlpclient/protobuf_log.go | Log record creation and severity level mapping |
| otlpclient/otlp_client_grpc_logs.go | gRPC client implementation for logs signal |
| otlpclient/otlp_client.go | Adds GetLogsEndpoint interface method |
| otlpserver/server.go | Refactored callbacks (TraceCallback/LogCallback) and added log processing |
| otlpserver/grpcserver.go | Extracted shared state, added separate logs service |
| otlpserver/httpserver.go | Path-based routing for /v1/traces and /v1/logs |
| data_for_test.go | Added log test fixtures and results tracking |
| main_test.go | Updated test harness to handle log records |
| go.mod | Updated OTel proto and dependencies |
| demos/02-simple-log.sh | Demo script showing log command usage |
| otelcli/*.go | Import ordering cleanup (subjective style changes) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // need to call StopWait on the GrpcServer, not directly on state | ||
| // so we create a temporary GrpcServer wrapper | ||
| gs := &GrpcServer{state: ls.state} |
There was a problem hiding this comment.
Creating a temporary GrpcServer wrapper to call StopWait is unnecessarily complex. Consider adding a stopWait() method to grpcServerState that handles the shutdown logic directly, then call it from both GrpcServer.StopWait() and here. This eliminates the wrapper creation and makes the shutdown path clearer.
| TracesEndpoint string `json:"traces_endpoint" env:"OTEL_EXPORTER_OTLP_TRACES_ENDPOINT"` | ||
| Protocol string `json:"protocol" env:"OTEL_EXPORTER_OTLP_PROTOCOL,OTEL_EXPORTER_OTLP_TRACES_PROTOCOL"` | ||
| LogsEndpoint string `json:"logs_endpoint" env:"OTEL_EXPORTER_OTLP_LOGS_ENDPOINT"` | ||
| Protocol string `json:"protocol" env:"OTEL_EXPORTER_OTLP_PROTOCOL,OTEL_EXPORTER_OTLP_TRACES_PROTOCOL,OTEL_EXPORTER_OTLP_LOGS_PROTOCOL"` |
There was a problem hiding this comment.
The env tag includes both traces and logs protocol environment variables, but per OTel spec, signal-specific protocol variables should only apply when using signal-specific endpoints. The general OTEL_EXPORTER_OTLP_PROTOCOL should be the only variable here, with OTEL_EXPORTER_OTLP_TRACES_PROTOCOL and OTEL_EXPORTER_OTLP_LOGS_PROTOCOL handled separately in their respective endpoint parsing logic if needed.
| Protocol string `json:"protocol" env:"OTEL_EXPORTER_OTLP_PROTOCOL,OTEL_EXPORTER_OTLP_TRACES_PROTOCOL,OTEL_EXPORTER_OTLP_LOGS_PROTOCOL"` | |
| Protocol string `json:"protocol" env:"OTEL_EXPORTER_OTLP_PROTOCOL"` |
| // For backwards compatibility, treat unspecified paths as traces | ||
| hs.handleTraces(rw, req) |
There was a problem hiding this comment.
This backwards compatibility behavior should be documented in the function's godoc comment, not just in an inline comment. Users of the API need to know that non-standard paths will be treated as trace endpoints.
| // Per spec, /v1/logs is the default, appended to any url passed | ||
| // to the general endpoint | ||
| if strings.HasPrefix(epUrl.Scheme, "http") && source != "signal" && !strings.HasSuffix(epUrl.Path, "/v1/logs") { | ||
| epUrl.Path = path.Join(epUrl.Path, "/v1/logs") |
There was a problem hiding this comment.
The path.Join function cleans the path and can remove trailing slashes or collapse multiple slashes. Per OTel spec, the path should be appended with a forward slash separator. Use epUrl.Path = strings.TrimSuffix(epUrl.Path, \"/\") + \"/v1/logs\" instead to ensure consistent path handling.
| epUrl.Path = path.Join(epUrl.Path, "/v1/logs") | |
| epUrl.Path = strings.TrimSuffix(epUrl.Path, "/") + "/v1/logs" |
Copilot Feedback EvaluationI've critically evaluated all 4 pieces of feedback from Copilot. Here's my analysis: 1. StopWait wrapper pattern (grpcserver.go:148-151)Copilot says: Creating a temporary GrpcServer wrapper is unnecessarily complex. Add a stopWait() method to grpcServerState. Evaluation: ❌ REJECT
Decision: Keep current code as-is. 2. Protocol env variable handling (config.go:82)Copilot says: Signal-specific protocol variables should only apply when using signal-specific endpoints, not be included in the general Protocol field. Evaluation:
Decision: Will research OTel spec and make decision based on spec vs. usability tradeoff. 3. ServeHTTP backwards compatibility docs (httpserver.go:36)Copilot says: Document the backwards compatibility behavior in the godoc comment. Evaluation: ✅ ACCEPT
Decision: Will add godoc comment explaining backwards compatibility. 4. path.Join vs string concatenation (config.go:409)Copilot says: Use Evaluation: 🤔 NEEDS TESTING
Decision: Will test path.Join with various inputs. If it causes issues, fix both traces and logs. Otherwise, keep current code. Plan
🤖 Claude [email protected] |
✅ path.Join evaluation completeTested Result: Decision: ❌ REJECT Copilot's suggestion. Keep 🤖 Claude [email protected] |
❌ Protocol env vars evaluation completeReviewed the OTel spec for protocol configuration. Current code: Protocol string `json:"protocol" env:"OTEL_EXPORTER_OTLP_PROTOCOL,OTEL_EXPORTER_OTLP_TRACES_PROTOCOL,OTEL_EXPORTER_OTLP_LOGS_PROTOCOL"`The spec says (line 13): "Each configuration option MUST be overridable by a signal specific option." This means signal-specific protocol vars should override the general one for that specific signal only, not populate a shared Protocol field. Analysis: otel-cli currently has a single Decision: ❌ REJECT - Keep current behavior Reasoning:
🤖 Claude [email protected] |
✅ ServeHTTP documentation improvedCopilot's feedback: Document backwards compatibility behavior in godoc comment. Change made: // ServeHTTP routes requests to the appropriate handler based on URL path.
// Routes /v1/traces to trace handler and /v1/logs to log handler per OTLP spec.
// For backwards compatibility, all other paths are treated as trace endpoints.Decision: ✅ ACCEPT - Good documentation practice. Important API behavior now documented in godoc where users will see it. 🤖 Claude [email protected] |
Why: Copilot feedback on PR #18 correctly noted that backwards compatibility behavior should be documented in godoc, not just inline comments Approach: Added two lines to godoc explaining OTLP spec routing and backwards compatibility behavior for non-standard paths Learned: API behavior documentation belongs in godoc where users see it Next: Push changes and complete Copilot feedback evaluation Co-Authored-By: Claude <[email protected]> Co-Authored-By: Copilot <[email protected]>
Add full support for OpenTelemetry logs alongside existing traces: **CLI Changes:** - New `otel-cli log` command with --body and --severity flags - Support for --logs-endpoint (follows OTel spec for signal-specific endpoints) - Reuses common flags: --service, --attrs, --endpoint, etc. **Client Implementation:** - otlpclient/protobuf_log.go: Log record creation and severity mapping - otlpclient/otlp_client_grpc_logs.go: gRPC logs client with OTLP export - otelcli/config_log.go: Configuration handling for log records **Server Refactoring:** - Extracted grpcServerState to share state across signal services - Renamed Callback -> TraceCallback for clarity - Added LogCallback type for log-specific handling - Both gRPC and HTTP servers now support logs via proper routing - gRPC: Separate service registration for logs - HTTP: Routes /v1/traces and /v1/logs to appropriate handlers **Testing:** - Added 2 integration tests for log command (gRPC) - Tests verify log record transmission and severity handling - Updated test harness to handle both spans and logs - Both log tests pass successfully **Demo:** - demos/02-simple-log.sh: Example usage with multiple severity levels **Architecture Notes:** - Clean separation of concerns with shared state pattern - Easy to extend for future signals (metrics, etc.) - Follows OpenTelemetry specification for endpoint paths - Maintains backward compatibility for traces Related: #17 (documents pre-existing TLS test failures) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Why: Copilot feedback on PR #18 correctly noted that backwards compatibility behavior should be documented in godoc, not just inline comments Approach: Added two lines to godoc explaining OTLP spec routing and backwards compatibility behavior for non-standard paths Learned: API behavior documentation belongs in godoc where users see it Next: Push changes and complete Copilot feedback evaluation Co-Authored-By: Claude <[email protected]> Co-Authored-By: Copilot <[email protected]>
tobert
force-pushed
the
feat/add-logs-and-metrics
branch
from
36c3c35 to
7f25e6e
Compare
Why: To improve clarity and consistency in the otlpclient package by explicitly naming files with their respective signal types (traces). This prepares the codebase for future additions like metrics. Approach: Renamed otlp_client_grpc.go, otlp_client_grpc_test.go, otlp_client_http.go, otlp_client_http_test.go, protobuf_span.go, and protobuf_span_test.go to include '_traces' or '_trace' in their names. Learned: Explicit naming conventions enhance code readability and maintainability, especially in projects with multiple signal types. Next: Update imports in affected files. 💎 Gemini <[email protected]>
Why: To make the `StringMapAttrsToProtobuf` function available to all signals (traces, logs, and future metrics) by moving it to a central, shared location. Approach: Moved the function from `otlpclient/protobuf_trace.go` to `otlpclient/otlp_client.go`. Learned: Centralizing common utility functions improves code reusability and maintainability. Next: Continue with PR review. 💎 Gemini <[email protected]>
|
I've completed a review of this PR, and the implementation of logging support is very well done. Findings
RefinementsTo further improve clarity and prepare for future signals like metrics, we made two small refactoring changes:
ConclusionThis is a solid contribution that significantly enhances 💎 Gemini [email protected] |
Summary
Adds full OpenTelemetry logs support to otel-cli, bringing it to parity with traces and setting the foundation for metrics support.
Changes
New Command
otel-cli log- Send log records via OTLP--body- Log message content--severity- TRACE, DEBUG, INFO, WARN, ERROR, FATAL--service,--attrs,--endpoint- Common flags from traces--logs-endpoint- Signal-specific endpoint (follows OTel spec)Architecture Improvements
grpcServerStatefor shared state across signalsCallback→TraceCallback, addedLogCallbackfor clarity/v1/tracesand/v1/logsCode Quality Improvements (by Gemini)
StringMapAttrsToProtobufto central location for reuse_traces,_logs) to filenamesImplementation
otelcli/log.go- Log command implementationotelcli/config_log.go- Configuration handlingotlpclient/protobuf_log.go- Log record creation and severity mappingotlpclient/otlp_client_grpc_logs.go- gRPC logs clientotlpserver/- Updated servers to handle both traces and logsDocumentation Improvements (addressing Copilot feedback)
Testing
demos/02-simple-log.shDependencies
go.opentelemetry.io/proto/otlpto v1.9.0Test Results
$ go test PASS ok github.com/tobert/otel-cli 0.962sAll tests passing including:
Example Usage
Breaking Changes
None. All existing trace functionality remains unchanged.
Copilot Feedback Addressed
✅ Evaluated all 4 pieces of Copilot feedback:
Full evaluation documented in PR comments.
Contributors
This PR includes collaborative work from:
Future Work
This refactoring sets up otel-cli to easily add metrics support next, following the same pattern.
Co-Authored-By: Claude [email protected]
Co-Authored-By: Gemini [email protected]
Co-Authored-By: Copilot [email protected]