[APP-6612] Add Filters for Downloading Logs #4673

JosephBorodach · 2025-01-03T17:57:10Z

This PR implements additional filters to the machines logs CLI command, specifically: keyword, start time, end time and log level. Below are the details and test cases for the command:

See ticket here

Invalid: Without any arguments

go run cli/viam/main.go machines logs

With Keyword Filter

go run cli/viam/main.go machines logs -machine="1" --organization="viam-dev" --location="office temp" --keyword="stack"

With Error Level

go run cli/viam/main.go machines logs -machine="1" --organization="viam-dev" --location="office temp" --levels="error"

Start Time

Start from after the last log

go run cli/viam/main.go machines logs --machine="1" --organization="viam-dev" --location="office temp" --start="2025-01-10T21:30:00Z"

The most recent few logs

go run cli/viam/main.go machines logs -machine="1" --organization="viam-dev" --location="office temp" --start="2025-01-13T07:20:31.000Z"

Logs from a slightly earlier start time

go run cli/viam/main.go machines logs -machine="1" --organization="viam-dev" --location="office temp" --start="2025-01-12T17:37:26.985Z"

Invalid: Cannot use start and count without end.

go run cli/viam/main.go machines logs -machine="1" --organization="viam-dev" --location="office temp" --start="2025-01-12T17:37:26.985Z" --count=10

End Time

Notice how in the second screenshot, we skip the logs passed 15:16.

Start & End Time

go run cli/viam/main.go machines logs -machine="1" --organization="viam-dev" --location="office temp" --start="2025-01-12T17:37:26.985Z" --end="2025-01-13T04:24:08.928Z"

Start + Count + End Time

go run cli/viam/main.go machines logs -machine="1" --organization="viam-dev" --location="office temp" --start="2025-01-13T08:19:18.865Z" --end="2025-01-13T21:03:37.372Z" --count=5

Edge Case

Do we not want to output something prettier if there are no logs to display?

Without providing `count`, defaults to 100 logs

go run cli/viam/main.go machines logs -machine="1" --organization="viam-dev" --location="office temp" --output="logs.txt" --format="text"

`count` of 10,000 works

go run cli/viam/main.go machines logs -machine="1" --organization="viam-dev" --location="office temp" --output="logs.txt" --format="text" --count=10000

`count` of 10,001 does not work

go run cli/viam/main.go machines logs -machine="1" --organization="viam-dev" --location="office temp" --output="logs.txt" --format="text" --count=10001

@jr22 - I personally do not love that when surpassing the maxLogs we still display the following:

…gs to text or json file

…ng the logs, instead of retrieving all of the logs and then writing them all to the file

…ck when writing to file

…n object

…t arguments to be paired - either both are provided, or an error will be displated

…ds called on the *viamClient

… logs cli command

purplenicole730

LGTM! There were some changes in flag names that were recently merged in, but I don't suspect it should affect your PR too much

purplenicole730 · 2025-01-07T15:02:31Z

cli/app.go

+						&cli.StringFlag{
+							Name:  logsFlagOutputFile,
+							Usage: "path to output file",
+						},
+						&cli.StringFlag{
+							Name:  logsFlagFormat,
+							Usage: "file format (text or json)",
+						},


maybe mention needs format flag, needs output flag for the users in the usage text? Not sure if necessary, but a suggestion, since I feel like I would read that as being able to format it when printing to console as well

You can decide the format when printing to the console! The command will default to text if the format flag is not included. Are you suggesting something else?

Oh, you can also format it for the console! Ignore me then :)

stuqdog

A couple small things but generally lgtm

stuqdog · 2025-01-07T16:02:11Z

cli/app.go

+	logsFlagFormat     = "format"
+	logsFlagOutputFile = "output"
+	logsFlagKeyword    = "keyword"
+	logsFlagLevels     = "levels"
+	logsFlagStartTime  = "start"
+	logsFlagEndTime    = "end"
+	logsFlagErrors     = "errors"
+	logsFlagTail       = "tail"
+	logsFlagCount      = "count"


it looks like some of these were added in a separate PR already. Would you mind pulling down main so we can get a more up-to-date look at what the diff is precisely?

stuqdog · 2025-01-07T16:04:28Z

cli/app.go

+	logsFlagStartTime  = "start"
+	logsFlagEndTime    = "end"


(minor) I'm gonna ask you to do a bit of bookkeeping for us here if you don't mind! We already have a "start" flag and an "end" flag (dataFlagStart and dataFlagEnd). Would you be willing to rename those to generalFlagStart and generalFlagEnd and then update call sites so that there's only the single instance of a --start or --end flag?

…s-For-Downloading-Logs

…Time to generalFlagStart and generalFlagEnd

RoxyFarhad · 2025-01-15T14:33:41Z

cli/client.go

 		if remainingLogsNeeded < len(resp.Logs) {
 			resp.Logs = resp.Logs[:remainingLogsNeeded]
 		}

+		// Write the logs to the output


nit: remove this comment and the one below

RoxyFarhad · 2025-01-15T14:34:16Z

cli/app.go

+						},
+						&cli.StringFlag{
+							Name:  generalFlagStart,
+							Usage: "ISO-8601 timestamp in RFC3339 format indicating the start of the interval filter",


nit: may be nice to give an example of what that is for both this and the one below

RoxyFarhad · 2025-01-15T14:35:36Z

cli/client.go

@@ -758,6 +761,16 @@ func RobotsLogsAction(c *cli.Context, args robotsLogsArgs) error {
 		return err
 	}

+	// Check if both start time and count are provided
+	if args.Start != "" && args.Count > 0 && args.End == "" {
+		return errors.New("unsupported functionality: specifying both a start time and a count without an end time is not supported. " +


do we need this? i feel like this is explaining weirdness in our code to our users which we shouldn't do.

May make more sense to make this a comment rather than something user facing and just say this is unsupported to the user

(I know this is out of scope, but this behaviour is also counter-intuitive to me). Other logs api's support this: https://docs.aws.amazon.com/ja_jp/AmazonCloudWatchLogs/latest/APIReference/API_StartQuery.html

so why don't we?

I agree that this functionality is something we really need to support but it will require changing the app's api to support sorting the order of the logs (currently we default to descending order for time)

even though I would love to implement the sorting functionality now, it's out of the scope of the pr and I am not sure we want to prioritize more resources to this now. I think we wanted to leave this comment so that it screams out to us that it needs to be addressed

can you make a ticket to add this in as something in the backloig? and link this code?

I agree that theres nothing to change here as part of the ticket, but wondered if there was something we could do without having to change the API

Good call! I have a ticket cut, I will add it to the code

…omment regarding why a limit is not passed to the APP GetRobotPartLogs call

…mmand

RoxyFarhad · 2025-01-15T15:41:21Z

cli/client.go

+		// 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


👍🏼 much better

JosephBorodach added 11 commits December 31, 2024 15:41

Support downloading logs

386db8c

Restructure RobotsLogsAction to extend functionality for outputing lo…

232f143

…gs to text or json file

Refactor saveLogsToDisk to handle large log files efficiently

6a02466

Restructure fetchAndSaveLogs to write logs to the file while retrievi…

e14e9be

…ng the logs, instead of retrieving all of the logs and then writing them all to the file

Add nolint for not checking the error returned when closing the file

761d6fe

add headers to log output for file downloads

05dc2c2

Add constant variable for FormatJSON to appease linter. Add error che…

921a908

…ck when writing to file

Make the json format consecutive json object instead of one large jso…

690b887

…n object

Make error handling more comprehensive for requiring Output and Forma…

e2786a5

…t arguments to be paired - either both are provided, or an error will be displated

Make fetchAndSaveLogs, streamLogsForPart and printLogsToConsole metho…

225b5cb

…ds called on the *viamClient

Add filters including keyword, levels, and start and end times to the…

b0698af

… logs cli command

JosephBorodach requested a review from a team as a code owner January 3, 2025 17:57

JosephBorodach requested review from stuqdog and purplenicole730 and removed request for a team January 3, 2025 17:57

Base automatically changed from APP-6612-Support-Downloading-Logs to main January 7, 2025 14:17

purplenicole730 approved these changes Jan 7, 2025

View reviewed changes

stuqdog reviewed Jan 7, 2025

View reviewed changes

Merge remote-tracking branch 'upstream/main' into APP-6612-Add-Filter…

b3e00d3

…s-For-Downloading-Logs

viambot added the safe to test This pull request is marked safe to test from a trusted zone label Jan 8, 2025

Replace dataFlagStart, dataFlagEnd, logsFlagStartTime and logsFlagEnd…

bf0df5f

…Time to generalFlagStart and generalFlagEnd

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 8, 2025

Add logic to streamLogsForPart for the filters

7abd091

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 8, 2025

Remove validation checks

0c6c6f9

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 8, 2025

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 14, 2025

Cleanup streamLogsForPart logic

b8a5d77

JosephBorodach force-pushed the APP-6612-Add-Filters-For-Downloading-Logs branch from 0b85f4e to b8a5d77 Compare January 14, 2025 16:42

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 14, 2025

JosephBorodach requested review from RoxyFarhad and removed request for jr22 January 15, 2025 14:27

RoxyFarhad reviewed Jan 15, 2025

View reviewed changes

Remove redundant comments from streamLogsForPart and add clarifying c…

21d27c6

…omment regarding why a limit is not passed to the APP GetRobotPartLogs call

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 15, 2025

JosephBorodach force-pushed the APP-6612-Add-Filters-For-Downloading-Logs branch from b892bb9 to 21d27c6 Compare January 15, 2025 15:26

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 15, 2025

JosephBorodach requested review from RoxyFarhad and removed request for RoxyFarhad January 15, 2025 15:32

Add examples for using the start and end time options for the logs co…

8a30121

…mmand

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 15, 2025

JosephBorodach requested a review from RoxyFarhad January 15, 2025 15:36

RoxyFarhad reviewed Jan 15, 2025

View reviewed changes

RoxyFarhad approved these changes Jan 15, 2025

View reviewed changes

Add todo comments to RobotsLogsAction

0c77e33

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 15, 2025

JosephBorodach merged commit 2b2ad24 into main Jan 15, 2025
16 checks passed

JosephBorodach deleted the APP-6612-Add-Filters-For-Downloading-Logs branch January 15, 2025 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APP-6612] Add Filters for Downloading Logs #4673

[APP-6612] Add Filters for Downloading Logs #4673

JosephBorodach commented Jan 3, 2025 •

edited

Loading

purplenicole730 left a comment

purplenicole730 Jan 7, 2025

JosephBorodach Jan 8, 2025

purplenicole730 Jan 9, 2025

stuqdog left a comment

stuqdog Jan 7, 2025

stuqdog Jan 7, 2025

RoxyFarhad Jan 15, 2025

RoxyFarhad Jan 15, 2025

RoxyFarhad Jan 15, 2025

RoxyFarhad Jan 15, 2025

JosephBorodach Jan 15, 2025 •

edited

Loading

RoxyFarhad Jan 15, 2025

JosephBorodach Jan 15, 2025

RoxyFarhad Jan 15, 2025

[APP-6612] Add Filters for Downloading Logs #4673

[APP-6612] Add Filters for Downloading Logs #4673

Conversation

JosephBorodach commented Jan 3, 2025 • edited Loading

Invalid: Without any arguments

With Keyword Filter

With Error Level

Start Time

Start from after the last log

The most recent few logs

Logs from a slightly earlier start time

Invalid: Cannot use start and count without end.

End Time

Start & End Time

Start + Count + End Time

Edge Case

Without providing count, defaults to 100 logs

count of 10,000 works

count of 10,001 does not work

purplenicole730 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuqdog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JosephBorodach Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JosephBorodach commented Jan 3, 2025 •

edited

Loading

Without providing `count`, defaults to 100 logs

`count` of 10,000 works

`count` of 10,001 does not work

JosephBorodach Jan 15, 2025 •

edited

Loading