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 log streaming in beta platform #1743

Merged
merged 4 commits into from
Apr 18, 2024
Merged

Conversation

Kazy
Copy link
Contributor

@Kazy Kazy commented Apr 17, 2024

I know I've pushed against having v1/v2, we'll refactor later, but that's the cleanest way I've found.

@Kazy Kazy requested review from jonaro00, oddgrd and iulianbarbu April 17, 2024 12:17
@Kazy Kazy changed the title Logger update Add log streaming in beta platform Apr 17, 2024
Copy link
Contributor

@oddgrd oddgrd left a comment

Choose a reason for hiding this comment

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

Cool! I left some non-blocking questions and notes.

cargo-shuttle/src/lib.rs Show resolved Hide resolved
cargo-shuttle/src/lib.rs Outdated Show resolved Hide resolved
common/src/log.rs Outdated Show resolved Hide resolved
@@ -832,6 +837,38 @@ impl Shuttle {
Ok(CommandOutcome::Ok)
}

async fn logs_beta(&self, args: LogsArgs) -> Result<CommandOutcome> {
Copy link
Member

Choose a reason for hiding this comment

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

will there be a non-streaming option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and probably sooner rather than later since I've just discovered the cost of a Kinesis Stream 🥲

if args.raw {
println!("{}", log.line);
} else {
println!("[{}] ({}) {}", log.timestamp, log.source, log.line);
Copy link
Member

Choose a reason for hiding this comment

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

Can use the Display impl?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we should use Display there. It's only useful in the context of printing to the console.

cargo-shuttle/src/lib.rs Show resolved Hide resolved
@Kazy Kazy merged commit 8767db4 into feat/shuttle-ecs-common Apr 18, 2024
11 of 32 checks passed
@Kazy Kazy deleted the logger-update branch April 18, 2024 15:50
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.

4 participants