Repository navigation
feat(go): add attributes to CLI - #638
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds OpenTelemetry tracing attributes to the execute and run CLI commands, and appends LIMIT clauses to the example SQL queries. However, the changes in root.go introduce a couple of issues: they discard the command context by using context.Background() and create a potential span leak if garf.New returns an error.
| ctx, _ := tracer.Start(context.Background(), "cli") | ||
| cmd.SetContext(ctx) | ||
| defer span.End() | ||
| garfEndpoint := viper.GetString("endpoint") | ||
| g, err := garf.New(ctx, garfEndpoint) | ||
| if err != nil { |
There was a problem hiding this comment.
There are two issues here:\n1. Context Propagation: Using context.Background() discards any context (including cancellation, deadlines, or parent metadata) that Cobra or the caller set up on the command. We should use cmd.Context() instead.\n2. Span Leak on Error: If garf.New returns an error, PersistentPreRunE exits early. Since PersistentPostRunE is not executed when PersistentPreRunE fails, the span created here will be leaked (never ended). We should capture the span and explicitly call span.End() in the error path.
| ctx, _ := tracer.Start(context.Background(), "cli") | |
| cmd.SetContext(ctx) | |
| defer span.End() | |
| garfEndpoint := viper.GetString("endpoint") | |
| g, err := garf.New(ctx, garfEndpoint) | |
| if err != nil { | |
| ctx, span := tracer.Start(cmd.Context(), "cli") | |
| cmd.SetContext(ctx) | |
| garfEndpoint := viper.GetString("endpoint") | |
| g, err := garf.New(ctx, garfEndpoint) | |
| if err != nil { | |
| span.End() |
No description provided.