Skip to content

feat(logs): implement several filters & support retrieval of persisted logs #165

Merged
magurotuna merged 20 commits intodenoland:mainfrom
magurotuna:query_logs
Jul 27, 2023
Merged

feat(logs): implement several filters & support retrieval of persisted logs #165
magurotuna merged 20 commits intodenoland:mainfrom
magurotuna:query_logs

Conversation

@magurotuna
Copy link
Copy Markdown
Member

@magurotuna magurotuna commented Jul 17, 2023

This commit does two enhancements to the log subcommand.

  • add several options such as --grep, --levels, and --regions that allows for filtering out logs that we're not interested in
  • add options called --since and --until, which enable the retrieval of persisted logs (as opposed to live logs)

So now we can run the following commands, for instance:

# Live logs
deployctl logs --project=helloworld --prod --levels=error,warning --regions=region1,region2

# Query past logs (in Linux)
deployctl logs --project=helloworld --since=$(date -Iseconds --date='3 hours ago') \
--until=$(date -Iseconds --date='42 minutes ago') --grep=foo

# Query past logs (in macOS)
deployctl logs --project=helloworld --since=$(date -Iseconds -v-3H) \
--until=$(date -Iseconds -v-42M) --grep=foo

Demo:

image

Copy link
Copy Markdown
Contributor

@satyarohith satyarohith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I specify incorrect region code, the screen is stuck and doesn't respond.

➜  deployctl git:(query_logs) deno run -A deployctl.ts logs --project=dotland --region=gcp-asia-east1
✔ Project: dotland
2023-07-20T08:54:46.830606578Z   gcp-asia-east1 Listening on http://localhost:80/
2023-07-20T08:54:46.830834871Z   gcp-asia-east1 isolate start time: 1.10 s
^C
➜  deployctl git:(query_logs) deno run -A deployctl.ts logs --project=dotland --region=incorrect-region
✔ Project: dotland

We should throw "invalid region code" error (better if we do it in the backend)

Copy link
Copy Markdown
Contributor

@satyarohith satyarohith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--grep is case sensitive, do we want to keep that? I would prefer it case insensitive leaning towards more matches.

➜  deployctl git:(query_logs) deno run -A deployctl.ts logs --project=dotland --region=gcp-asia-east1 --grep=listening --timerange=4h,now
✔ Project: dotland
✖ No logs found matching the provided condition
➜  deployctl git:(query_logs) deno run -A deployctl.ts logs --project=dotland --region=gcp-asia-east1 --grep=localhost --timerange=4h,now
✔ Project: dotland
✔ Found 1 logs
2023-07-20T08:54:46.830Z   gcp-asia-east1 Listening on http://localhost:80/
➜  deployctl git:(query_logs) deno run -A deployctl.ts logs --project=dotland --region=gcp-asia-east1 --grep=Listening --timerange=4h,now
✔ Project: dotland
✔ Found 1 logs
2023-07-20T08:54:46.830Z   gcp-asia-east1 Listening on http://localhost:80/

Copy link
Copy Markdown
Contributor

@satyarohith satyarohith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of --timerange, what are your thoughts on using --since and --until flags for time range? They are more common with popular tools like git and journalctl.

@magurotuna
Copy link
Copy Markdown
Member Author

When I specify incorrect region code, the screen is stuck and doesn't respond.

That's because you were on live mode - currently there's no way for frontend (in this case, deployctl) to know what are the available region codes at the time of invoked, so deployctl just tries to subscribe all the live logs and filter them out based on the --region value that the user inputs, even if the region code is invalid.

I think we could improve this by making a change to backend. I'll work on the backend work soon

-grep is case sensitive, do we want to keep that?

I agree that case insensitive would be greater in most cases. We could support that by fixing the backend.

instead of --timerange, what are your thoughts on using --since and --until flags for time range? They are more common with popular tools like git and journalctl.

Sounds like a good idea - Not only that's probably better in terms of UX, but also we can simplify the code to parse the command line args. I'll do that approach.

Copy link
Copy Markdown
Contributor

@satyarohith satyarohith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the ability to pass multiple values to --level and --region flags would be very useful. The below example demonstrates a query to fetch warn and error level logs from asia-east1 and us-east4.

➜  deployctl git:(query_logs) deno run -A deployctl.ts logs --project=dotland --level=error,warn --region=gcp-asia-east1,gcp-us-east4 --timerange=4h,now
✔ Project: dotland
✖ No logs found matching the provided condition

Renaming --level to --levels and --region to --regions might be a better idea?

Copy link
Copy Markdown
Contributor

@satyarohith satyarohith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice start Yusuke! It's useful to query logs using deployctl in addition to the UI.

@magurotuna
Copy link
Copy Markdown
Member Author

@satyarohith addressed some of your suggestions:

  • supports multiple regions and log levels (--regions, --levels)
  • checks if the input region(s) are correct. If not, an error with available region codes is displayed
    image
  • removed --timerange in favor of --since and --until. Supported format is RFC3339 only at least for now. Users can leverage date command to get a datetime string they want like the examples in the help message shown by deployctl logs --help.

Case-insensitive grep is not (yet) addressed because that's what we need to implement on backend. I'll be working on the backend fix soon, though I think this PR can land without waiting for the backend change.

@magurotuna magurotuna requested a review from satyarohith July 25, 2023 01:01
@magurotuna
Copy link
Copy Markdown
Member Author

It would be great if we could support grep by more than one word, for instance --grep=foo --grep=bar. I'll try to see if that's possible easily

@magurotuna
Copy link
Copy Markdown
Member Author

OK, added multi-word grep support

Copy link
Copy Markdown
Contributor

@satyarohith satyarohith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should support relative/human date strings for --since/--until.

In git you can do the following: git log --since="2 hours ago" and it works. This is helpful for the logs command because most users will do relative queries to check for issues.

And [1] is complicated compared to [2]. [1] is also not cross-platform compatible. Users have to find a different command if they're on Windows.

[1]

deployctl logs --project=helloworld --since=$(date -Iseconds --date='2 hours ago') --until=$(date -Iseconds --date='30 minutes ago') --grep=foo

[2]

deployctl logs --project=helloworld --since="2 hours ago" --until="30 minutes ago" --grep=foo

Copy link
Copy Markdown
Contributor

@satyarohith satyarohith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the expected behavior of multiple --grep flags? Does it mean the log can contain either values specified in the flags or all the values?

The current output suggests that all values should be present in a log.

Copy link
Copy Markdown
Contributor

@satyarohith satyarohith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to be me in general apart from my concern for the relative time strings support in --since/until.

@magurotuna
Copy link
Copy Markdown
Member Author

We should support relative/human date strings for --since/--until.
In git you can do the following: git log --since="2 hours ago" and it works. This is helpful for the logs command because most users will do relative queries to check for issues.
And [1] is complicated compared to [2]. [1] is also not cross-platform compatible. Users have to find a different command if they're on Windows.

In general I agree, but I didn't implement that in this PR because that would complicate the PR a lot. Also we need to determine what kind of format should be supported. (By the way I wasn't able to find the exact format that git accepts as an input for --since and --until - do you know about it by any chance?)
So I think for now we can ship these options with datetime format limited to RFC3339, and we'll be able to enhance it later. Delegating a particular function to a dedicated utility (in this case, date command) is also confirming to the Unix philosophy.

@satyarohith
Copy link
Copy Markdown
Contributor

By the way I wasn't able to find the exact format that git accepts as an input for --since and --until - do you know about it by any chance?

I couldn't find anything in the docs but from source, I could find some clues https://github.com/git/git/blob/a80be152923a46f04a06bade7bcc72870e46ca09/date.h#L10

So I think for now we can ship these options with datetime format limited to RFC3339, and we'll be able to enhance it later.

Sure, let's open an issue then.

Delegating a particular function to a dedicated utility (in this case, date command) is also confirming to the Unix philosophy.

Unix disregards Windows. We should follow it when it makes sense at the same time we should aim for a uniform experience across platforms.

@magurotuna
Copy link
Copy Markdown
Member Author

@satyarohith Thanks for the thorough review. Addressed your comments and opened an issue for supporting more date formats #168

@magurotuna magurotuna requested a review from satyarohith July 26, 2023 15:42
Copy link
Copy Markdown
Contributor

@satyarohith satyarohith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@magurotuna magurotuna merged commit e9f695b into denoland:main Jul 27, 2023
@magurotuna magurotuna deleted the query_logs branch July 27, 2023 05:41
@magurotuna magurotuna mentioned this pull request Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants