feat: [ add tracing ] #13
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive OpenTelemetry tracing support to the faceit-cli application, enabling distributed tracing and observability through OTLP collectors and Zipkin/Jaeger backends. The implementation includes telemetry instrumentation throughout the application layers, configuration management through both YAML files and environment variables, and complete infrastructure setup via Docker Compose.
Key changes include:
- Complete OpenTelemetry integration with OTLP gRPC/HTTP exporters
- YAML-based configuration system with environment variable overrides
- Telemetry instrumentation in repository and application layers
Reviewed Changes
Copilot reviewed 28 out of 33 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
otel-collector-config.yaml |
OpenTelemetry Collector configuration for trace processing and export to Zipkin |
main.go |
Updated module path, added telemetry initialization and configuration command handling |
internal/telemetry/telemetry.go |
Core telemetry implementation with OTLP exporters and span management |
internal/telemetry/telemetry_test.go |
Unit tests for telemetry functionality |
internal/config/config.go |
Enhanced configuration system with YAML support and telemetry settings |
internal/config/yaml_config.go |
YAML configuration file management and default config creation |
internal/repository/faceit.go |
Added telemetry instrumentation to all repository methods |
internal/app/app.go |
Integrated telemetry spans for application lifecycle tracking |
docker-compose.yml |
Added OpenTelemetry Collector and Zipkin services |
docs/TRACING.md |
Comprehensive tracing documentation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| receivers: [otlp] | ||
| processors: [memory_limiter, batch, resource] | ||
| exporters: [zipkin] | ||
|
|
There was a problem hiding this comment.
There is a trailing whitespace character on line 49. This should be removed to maintain clean YAML formatting.
| // Using OTLP gRPC endpoint | ||
|
|
There was a problem hiding this comment.
[nitpick] This comment is redundant as it doesn't add meaningful information beyond what's already clear from the surrounding context and variable naming. Consider removing or making it more descriptive about the gRPC configuration specifics.
| // Using OTLP gRPC endpoint | |
internal/repository/faceit.go
Outdated
| if r.telemetry != nil && span != nil { | ||
| span.RecordError(err) | ||
| span.SetStatus(codes.Error, err.Error()) | ||
| } |
There was a problem hiding this comment.
The pattern if r.telemetry != nil && span != nil is repeated throughout the file. Consider extracting this into a helper method like recordError(span trace.Span, err error) to reduce code duplication and improve maintainability.
internal/config/config.go
Outdated
| telemetryEnabled := os.Getenv("TELEMETRY_ENABLED") == "true" | ||
| otlpEndpoint := os.Getenv("OTLP_ENDPOINT") | ||
| if otlpEndpoint == "" { | ||
| otlpEndpoint = "http://localhost:4318/v1/traces" |
There was a problem hiding this comment.
The default OTLP endpoint includes the /v1/traces path, but line 215 shows the YAML default is localhost:4317 without the path. This inconsistency could cause configuration issues. The OTLP endpoint should be consistent between environment variable defaults and YAML defaults.
| otlpEndpoint = "http://localhost:4318/v1/traces" | |
| otlpEndpoint = "localhost:4317" |
No description provided.