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

add cloud logs command #58

Merged
merged 2 commits into from
Oct 31, 2023
Merged

Conversation

rajatjindal
Copy link
Contributor

spin-cloud-logs 0.1.2 (b977558 2023-08-11)
Logs fetches app logs from Fermyon Cloud

USAGE:
    spin cloud logs [OPTIONS] --app-name <app-name>

OPTIONS:
        --app-name <app-name>
            App name

        --environment-name <environment-name>
            Find app to fetch logs for in the Fermyon instance saved under the specified name. If
            omitted, Spin looks for app in default unnamed instance [env:
            FERMYON_DEPLOYMENT_ENVIRONMENT=]

        --follow
            Follow logs output

    -h, --help
            Print help information

        --interval <interval>
            interval in secs to refresh logs from cloud [default: 5]

        --tail <tail>
            Number of lines to show from the end of the logs [default: 10]

    -V, --version
            Print version information

@rajatjindal rajatjindal force-pushed the add-tail-command branch 3 times, most recently from 8aa4826 to b1c8913 Compare August 11, 2023 08:27
@rajatjindal rajatjindal force-pushed the add-tail-command branch 2 times, most recently from 3aee368 to e39a0d7 Compare August 11, 2023 10:23
Copy link
Contributor

@rylev rylev left a comment

Choose a reason for hiding this comment

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

I'd like to figure out if there's a way to do this without repeatedly downloading all logs.

src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/mod.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@kate-goldenring kate-goldenring left a comment

Choose a reason for hiding this comment

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

Thanks for adding this Rajat! To @rylev's point, instead of getting all logs, can you update channel_logs to take in max aka count. The api_channels_id_logs_get already has support for requesting n many logs and it is the latest n returned by journal.

src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
@rajatjindal
Copy link
Contributor Author

Thanks for adding this Rajat! To @rylev's point, instead of getting all logs, can you update channel_logs to take in max aka count. The api_channels_id_logs_get already has support for requesting n many logs and it is the latest n returned by journal.

I want to do that too. but lets say we start the tail command and fetched 100 lines of logs. now the app added 5 new line of logs. when we ask for 100 lines of logs again, how do we know that the last 5 are the latest ones to display?

@rajatjindal rajatjindal force-pushed the add-tail-command branch 2 times, most recently from 49137e9 to e436683 Compare August 28, 2023 05:53
@rajatjindal
Copy link
Contributor Author

Thanks for adding this Rajat! To @rylev's point, instead of getting all logs, can you update channel_logs to take in max aka count. The api_channels_id_logs_get already has support for requesting n many logs and it is the latest n returned by journal.

I want to do that too. but lets say we start the tail command and fetched 100 lines of logs. now the app added 5 new line of logs. when we ask for 100 lines of logs again, how do we know that the last 5 are the latest ones to display?

I looked at the logging api code. if we can add ability to say "give me logs since ", then we might be able to circumvent the problem. I'll give that a try soon.

cc @michelleN - I know you were looking at the logging api as well.

@rajatjindal
Copy link
Contributor Author

this will fix #107

src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@kate-goldenring kate-goldenring left a comment

Choose a reason for hiding this comment

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

Thanks for updating the APIs to make this possible @rajatjindal

src/main.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
@rajatjindal
Copy link
Contributor Author

+1 using some color for the prefix could be nice here as well.

chatted with Ryan, and adding a color to prefix can be done as a follow up PR. thanks

@rajatjindal rajatjindal force-pushed the add-tail-command branch 2 times, most recently from 357c675 to 2f493b5 Compare October 26, 2023 01:32
@rajatjindal
Copy link
Contributor Author

@kate-goldenring @rylev @itowlson @karthik2804

thank you for your feedback on this PR. I think this is now ready to be merged. please let me know if there are comments that I might have missed to address.

thanks

Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

I don't think anything I have is blocking (though it would be highly desirable to get rid of that panic), but I don't feel I know enough to approve, sorry.

src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Outdated Show resolved Hide resolved
@rajatjindal
Copy link
Contributor Author

@itowlson, as always, thank you for your insightful feedback on this PR.

src/commands/logs.rs Outdated Show resolved Hide resolved
@itowlson
Copy link
Contributor

Looking good @rajatjindal, just one question and one thing that I mentioned in a comment but forgot to log in the review - sorry about that!

@bacongobbler
Copy link
Member

bacongobbler commented Oct 27, 2023

Would you mind changing this to use the /api/apps/:id/logs endpoints? The latest version of the cloud-openapi client contains these endpoints. The channel log endpoints are planned to be deprecated soon.

fermyon/cloud-openapi#10

@rajatjindal rajatjindal force-pushed the add-tail-command branch 3 times, most recently from 0787d17 to ef05578 Compare October 30, 2023 11:44
@rajatjindal
Copy link
Contributor Author

rajatjindal commented Oct 30, 2023

I have updated openapi client and updated the PR to use new endpoints.

Copy link
Member

@radu-matei radu-matei left a comment

Choose a reason for hiding this comment

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

Tested this end to end and everything looks great!

LGTM

Copy link
Contributor

@rylev rylev left a comment

Choose a reason for hiding this comment

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

Just some last second code nits. Feel free to merge this without addressing those!

src/commands/logs.rs Outdated Show resolved Hide resolved
src/commands/logs.rs Show resolved Hide resolved
src/commands/logs.rs Show resolved Hide resolved
Copy link
Collaborator

@kate-goldenring kate-goldenring left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @rajatjindal

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>
Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>
@rajatjindal rajatjindal merged commit 9742df5 into fermyon:main Oct 31, 2023
8 checks passed
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.

Add spin cloud logs to view app logs on the CLI
7 participants