Skip to content
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

[A0CLI-5] feat: logs tail #6

Merged
merged 3 commits into from
Jan 22, 2021
Merged

[A0CLI-5] feat: logs tail #6

merged 3 commits into from
Jan 22, 2021

Conversation

shushen
Copy link
Contributor

@shushen shushen commented Jan 22, 2021

Description

Add logs tail command

The implementation is naive: it starts with a search query sorted by date:1 and then takes the last logID and switch to checkpoint method in a loop.

The limit of 100 log entries returned in each API call and the sleep duration of 1 second potentially could risk falling behind if the log is being generated at a rate > 100/second, therefore, we only sleep when we are getting less than 90 log entries.

References

https://auth0team.atlassian.net/browse/A0CLI-5

Testing

  • This change adds test coverage for new/changed/fixed functionality

Checklist

  • I have added documentation for new/changed functionality in this PR or in auth0.com/docs
  • All active GitHub checks for tests, formatting, and security are passing
  • The correct base branch is being used, if not master

internal/cli/logs.go Outdated Show resolved Hide resolved
internal/cli/logs.go Outdated Show resolved Hide resolved
Copy link
Contributor

@cyx cyx left a comment

Choose a reason for hiding this comment

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

Have a bunch of nits, but otherwise :shipit: 💪 🙇

Copy link
Contributor

@paddycarey paddycarey 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! Left some comments and a suggestion for the formatter/renderer.

However the command is broken currently due to the list assignment issue mentioned, that should be fixed before merging.

As an aside, it shouldn't block this PR, we can change it later if wanted, but I quite like how the heroku CLI does logs i.e. not with a tail subcommand, but rather a couple flags on logs itself:

So it'd look like:

  • auth0 logs (return 100 recent logs)
  • auth0 logs -n $x (return $x recent logs)
  • auth0 logs --tail (tail logs)

What do you think?

internal/display/logs.go Outdated Show resolved Hide resolved
internal/cli/logs.go Outdated Show resolved Hide resolved
internal/cli/logs.go Outdated Show resolved Hide resolved
}

cli.renderer.LogList(list)
time.Sleep(1 * time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about this one, should we just remove the sleep and let the SDK handle retrying and rate limits? It'll make the initial request quickly but gracefully back off if we're rate limited for whatever reason. NAB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hitting rate limit itself triggers an event and generates an additional log entry, I'm inclined to avoid hitting rate limit if possible. Since this is not real-time logs, the event itself also only appears after a short interval, there is probably not very useful in trying to be real-time.

I've changed the implementation to only sleep when we are not getting a lot entries (<90) returned, otherwise we will keep polling. This should avoid falling behind as I noted earlier.

internal/cli/logs.go Outdated Show resolved Hide resolved
Co-authored-by: Patrick Carey <[email protected]>
@shushen
Copy link
Contributor Author

shushen commented Jan 22, 2021

As an aside, it shouldn't block this PR, we can change it later if wanted, but I quite like how the heroku CLI does logs i.e. not with a tail subcommand, but rather a couple flags on logs itself:

So it'd look like:

  • auth0 logs (return 100 recent logs)
  • auth0 logs -n $x (return $x recent logs)
  • auth0 logs --tail (tail logs)

I like that! This PR was meant to get things started and was obviously pretty rough. Sorry for stealing the thunder with my mediocre implementation :-) and thanks a lot for the review! Please take another look!

@shushen shushen requested review from paddycarey and cyx January 22, 2021 16:44
Copy link
Contributor

@paddycarey paddycarey left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@shushen shushen merged commit dbc1462 into main Jan 22, 2021
@shushen shushen deleted the logs branch January 22, 2021 18:20
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.

3 participants