-
Notifications
You must be signed in to change notification settings - Fork 111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[APP-6612] Add Filters for Downloading Logs #4673
Merged
JosephBorodach
merged 31 commits into
main
from
APP-6612-Add-Filters-For-Downloading-Logs
Jan 15, 2025
Merged
Changes from 2 commits
Commits
Show all changes
31 commits
Select commit
Hold shift + click to select a range
386db8c
Support downloading logs
JosephBorodach 232f143
Restructure RobotsLogsAction to extend functionality for outputing lo…
JosephBorodach 6a02466
Refactor saveLogsToDisk to handle large log files efficiently
JosephBorodach e14e9be
Restructure fetchAndSaveLogs to write logs to the file while retrievi…
JosephBorodach 761d6fe
Add nolint for not checking the error returned when closing the file
JosephBorodach 05dc2c2
add headers to log output for file downloads
JosephBorodach 921a908
Add constant variable for FormatJSON to appease linter. Add error che…
JosephBorodach 690b887
Make the json format consecutive json object instead of one large jso…
JosephBorodach e2786a5
Make error handling more comprehensive for requiring Output and Forma…
JosephBorodach 225b5cb
Make fetchAndSaveLogs, streamLogsForPart and printLogsToConsole metho…
JosephBorodach b0698af
Add filters including keyword, levels, and start and end times to the…
JosephBorodach b3e00d3
Merge remote-tracking branch 'upstream/main' into APP-6612-Add-Filter…
JosephBorodach bf0df5f
Replace dataFlagStart, dataFlagEnd, logsFlagStartTime and logsFlagEnd…
JosephBorodach 7abd091
Add logic to streamLogsForPart for the filters
JosephBorodach 0c6c6f9
Remove validation checks
JosephBorodach b292477
Remove constant variables for formatJSON and formatText
JosephBorodach 0d65512
Correct cli argument types to strings from timestamps
JosephBorodach 791cda4
Fix start issue
JosephBorodach 774614b
Add parseTimeString helper method and refactor streamLogsForPart and …
JosephBorodach 6a8138e
Remove logsFlagErrors flag from logs because it is redundant since th…
JosephBorodach 410bad0
Permit the combination of start + count + end in RobotsLogsAction
JosephBorodach f281e27
Make error for 'args.Start != && args.Count > 0 && args.End == ' in …
JosephBorodach f35b373
Merge branch 'main' into APP-6612-Add-Filters-For-Downloading-Logs
JosephBorodach 575f98e
Remove logsFlagErrors flag from logs because it is redundant since th…
JosephBorodach c164165
Fix bug to allow for more than 100 logs to be retrieved from streamLo…
JosephBorodach 2f273e2
Fix useless print with f usage found Errorf
JosephBorodach ef5db1a
Fix parseTimeString - timeLayout always receives same timelayout
JosephBorodach b8a5d77
Cleanup streamLogsForPart logic
JosephBorodach 21d27c6
Remove redundant comments from streamLogsForPart and add clarifying c…
JosephBorodach 8a30121
Add examples for using the start and end time options for the logs co…
JosephBorodach 0c77e33
Add todo comments to RobotsLogsAction
JosephBorodach File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -852,6 +852,11 @@ func (c *viamClient) streamLogsForPart(part *apppb.RobotPart, args robotsLogsArg | |
|
||
// Fetch logs in batches and write them to the output. | ||
for fetchedLogCount := 0; fetchedLogCount < maxLogsToFetch; { | ||
// We do not request the exact limit specified by the user in the `count` argument because the API enforces a maximum | ||
// limit of 100 logs per batch fetch. To keep the RDK independent of specific limits imposed by the app API, | ||
// we always request the next full batch of logs as allowed by the API (currently 100). This approach | ||
// ensures that if the API limit changes in the future, only the app API logic needs to be updated without requiring | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍🏼 much better |
||
// changes in the RDK. | ||
resp, err := c.client.GetRobotPartLogs(c.c.Context, &apppb.GetRobotPartLogsRequest{ | ||
Id: part.Id, | ||
Filter: keyword, | ||
|
@@ -882,7 +887,6 @@ func (c *viamClient) streamLogsForPart(part *apppb.RobotPart, args robotsLogsArg | |
resp.Logs = resp.Logs[:remainingLogsNeeded] | ||
} | ||
|
||
// Write the logs to the output | ||
for _, log := range resp.Logs { | ||
formattedLog, err := formatLog(log, part.Name, args.Format) | ||
if err != nil { | ||
|
@@ -894,7 +898,6 @@ func (c *viamClient) streamLogsForPart(part *apppb.RobotPart, args robotsLogsArg | |
} | ||
} | ||
|
||
// Increment the total number of logs fetched so far by the count of logs retrieved in this batch. | ||
fetchedLogCount += len(resp.Logs) | ||
|
||
// End of pagination if there is no next page token. | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jr22 - Is there a reason we don't have unit tests for this function's helpers? Should I add unit tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a reason that im aware of, feel free to add test coverage if you want!