Conversation
WalkthroughA new CLI command Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI (logsCmd)
participant API Client
participant Remote API
User->>CLI (logsCmd): Run `logs` command with flags
CLI (logsCmd)->>API Client: Build query from flags
API Client->>Remote API: Request logs with filters
Remote API-->>API Client: Return logs data
API Client->>CLI (logsCmd): Provide logs
CLI (logsCmd)->>User: Display formatted logs
alt Tail mode enabled
loop Every 5 seconds
CLI (logsCmd)->>API Client: Request new logs since last timestamp
API Client->>Remote API: Fetch logs
Remote API-->>API Client: Return new logs
API Client->>CLI (logsCmd): Provide new logs
CLI (logsCmd)->>User: Display new logs
end
end
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| dateFormat = "" | ||
| } | ||
|
|
||
| prevIds := []string{} |
There was a problem hiding this comment.
This array is to filter already shown logs from previous fetch. Issue is that passing in the last timestamp as startDate return results of that time (or later). I could not find a way around this
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
cmd/logs.go (3)
20-41: Remove redundant case conversion.The regex already uses the case-insensitive flag
(?i), making thestrings.ToLowercall redundant.Apply this diff to simplify the code:
- switch strings.ToLower(matches[2]) { + switch matches[2] { - case "m": + case "m", "M": - case "h": + case "h", "H": - case "d": + case "d", "D":Alternatively, remove the case-insensitive flag and keep the ToLower call for consistency.
124-139: Consider extracting the polling logic into a separate function.The anonymous function with the infinite loop makes the code harder to read and test. Consider extracting it into a named function.
+func pollLogs(ctx context.Context, fetcher func() bool, tail bool) { + for { + select { + case <-ctx.Done(): + return + default: + if success := fetcher(); !success { + return + } + if !tail { + return + } + time.Sleep(5 * time.Second) + } + } +} + // Then in printLogs function: - func() { - for { - select { - case <-ctx.Done(): - return - default: - if success := fetcher(); !success { - return - } - if !tail { - return - } - time.Sleep(5 * time.Second) - } - } - }() + pollLogs(ctx, fetcher, tail)
179-181: Consider adding validation for severity levels.The severity flag accepts any string value without validation. Consider validating against known severity levels to provide better user experience.
if severity, _ := cmd.Flags().GetString("severity"); severity != "" { + validSeverities := []string{"trace", "debug", "info", "warn", "error", "fatal"} + if !slices.Contains(validSeverities, strings.ToLower(severity)) { + logger.Fatal("invalid severity level: %s. Valid options: %s", severity, strings.Join(validSeverities, ", ")) + } query.Add("severity", severity) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/logs.go(1 hunks)
🔇 Additional comments (5)
cmd/logs.go (5)
1-18: LGTM! Clean package structure and appropriate imports.The package declaration and imports are well-organized and include all necessary dependencies for the CLI log functionality.
43-49: LGTM! Well-structured Log type.The struct design is clean with appropriate JSON tags and reasonable field types for log data.
51-54: LGTM! Standard API response structure.The LogsResponse struct follows a common pattern for API responses with success indication and data payload.
90-101: Review the deduplication logic for potential issues.The current deduplication logic clears
prevIdswhen none of the current response IDs are found in the previous set. This could cause issues in scenarios where the API returns non-overlapping result sets, potentially leading to missed or duplicate logs.Consider a more robust approach:
- // Check if any log in response.Data has an ID in prevIds - found := false - for _, log := range response.Data { - if slices.Contains(prevIds, log.ID) { - found = true - break - } - } - // If none of the IDs in the response are in prevIds, clear prevIds - if !found { - prevIds = prevIds[:0] - } + // Keep a reasonable limit on prevIds to prevent memory growth + if len(prevIds) > 1000 { + prevIds = prevIds[len(prevIds)-500:] + }
200-215: Comprehensive implementation covers all PR objectives.The implementation successfully delivers all features mentioned in the PR objectives (hideDate, hideTime, tail, since) plus additional filtering capabilities for agents, deployments, environments, etc.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/logs.go (1)
76-101: Consider optimizing the deduplication logic for better performance.The current deduplication approach using
slices.Containshas O(n) complexity per log entry, which could become inefficient with large log volumes. Consider using a map for O(1) lookups.- prevIds := []string{} + prevIds := make(map[string]bool)- // Check if any log in response.Data has an ID in prevIds - found := false - for _, log := range response.Data { - if slices.Contains(prevIds, log.ID) { - found = true - break - } - } - // If none of the IDs in the response are in prevIds, clear prevIds - if !found { - prevIds = prevIds[:0] - } + // Check if any log in response.Data has an ID in prevIds + found := false + for _, log := range response.Data { + if prevIds[log.ID] { + found = true + break + } + } + // If none of the IDs in the response are in prevIds, clear prevIds + if !found { + prevIds = make(map[string]bool) + }- for _, log := range response.Data { - if slices.Contains(prevIds, log.ID) { - continue - } - prevIds = append(prevIds, log.ID) + for _, log := range response.Data { + if prevIds[log.ID] { + continue + } + prevIds[log.ID] = true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/logs.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
cmd/logs.go (2)
Learnt from: pec1985
PR: agentuity/cli#342
File: cmd/logs.go:58-60
Timestamp: 2025-05-26T08:15:51.520Z
Learning: In the agentuity/cli codebase, util.GetURLs and util.EnsureLoggedIn functions don't return errors as their last return value. They handle errors internally and may call os.Exit(1) for fatal errors. The blank identifiers in the assignments are for other return values, not errors.
Learnt from: pec1985
PR: agentuity/cli#342
File: cmd/logs.go:58-60
Timestamp: 2025-05-26T08:15:51.520Z
Learning: In the agentuity/cli codebase, util.GetURLs returns (apiUrl, appUrl, transportUrl string) and util.EnsureLoggedIn returns (apikey, userId string). These functions handle errors internally and call os.Exit(1) for fatal conditions rather than returning errors. The blank identifiers in calls to these functions are for ignoring unused return values, not errors.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (actions)
🔇 Additional comments (6)
cmd/logs.go (6)
21-41: LGTM! Well-designed flexible duration parser.The implementation correctly handles both standard Go duration strings and the custom 'd' suffix for days. The regex pattern and parsing logic are sound, and the fallback to
time.ParseDurationensures compatibility with standard Go durations.
43-54: LGTM! Clean and appropriate data structures.The
LogandLogsResponsestructs are well-defined with proper JSON tags and appropriate field types. The structure aligns well with the API response format and supports all the required log attributes.
67-74: LGTM! Comprehensive date/time formatting logic.The conditional logic correctly handles all combinations of
hideDateandhideTimeflags, providing flexible display options for users. The use of Go's standard time format constants is appropriate.
124-139: LGTM! Excellent implementation of context-aware polling.The polling loop correctly handles context cancellation and implements the tail functionality with appropriate sleep intervals. The immediate return on fetch failure is a good design choice.
183-191: LGTM! Robust duration parsing with appropriate error handling.The integration of the custom duration parser with proper error handling and fallback to a default value ensures reliable functionality. The use of
logger.Fatalfor parsing errors is appropriate for a CLI tool.
200-215: LGTM! Comprehensive flag definitions with sensible defaults.The flag setup covers all the filtering and display options mentioned in the PR objectives. The default values (especially
env: "local"andsince: "1h") are reasonable for typical usage patterns.
| tail, _ := cmd.Flags().GetBool("tail") | ||
| hideDate, _ := cmd.Flags().GetBool("hideDate") | ||
| hideTime, _ := cmd.Flags().GetBool("hideTime") | ||
| printLogs(cmd.Context(), logger, cmd, query, tail, hideDate, hideTime) |
There was a problem hiding this comment.
use a signal context here instead of a background ctx so that SIGINT etc works better:
ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGINT, syscall.SIGTERM)
defer cancel()
|
small nit but otherwise looks great. 💯 |
3 participants
This is a first pass, mergable, but I'm sure there's a lot of room for improvement.
Neat arguments you can pass:
--hideDate,--hideTime,--tail,--sinceSummary by CodeRabbit