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

chore(ci): CI grouping and refactoring #1024

Merged
merged 6 commits into from Dec 4, 2023

Conversation

dcvz
Copy link
Contributor

@dcvz dcvz commented Nov 30, 2023

Checklist

  • Confirmed that run-checks all script has been executed.
  • Made sure the book is up to date with changes in this PR.

Changes

Improvements to CI Output:

  • Groups logs making them collapsible and easier to read / debug.
  • Updates the time duration output
  • Replaces our CI_RUN for standard CI env
Examples Output Screenshot 2023-12-01 at 00 56 24
STD Output Screenshot 2023-12-01 at 01 01 15

@dcvz dcvz changed the title chore(ci): Test more backends chore(ci): CI grouping and refactoring Nov 30, 2023
@dcvz dcvz force-pushed the chore/ci-more-backends branch 2 times, most recently from 6c58676 to 7922559 Compare November 30, 2023 23:14
@dcvz dcvz marked this pull request as ready for review December 1, 2023 00:08
Copy link
Collaborator

@antimora antimora left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@Luni-4 Luni-4 left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

I propose to change println! with debug! or info! macros, which can be enabled through filters.

Here an example of how to do that in main

   // Enable filter to log the information contained in the lib.
    let filter_layer = EnvFilter::try_from_default_env()
        .or_else(|_| {
            // We can add this through clap
            if opts.verbose {
                EnvFilter::try_new("debug")
            } else {
                EnvFilter::try_new("info")
            }
        })
        .unwrap();

    // Run tracer.
    tracing_subscriber::fmt()
        .without_time()
        .with_env_filter(filter_layer)
        .with_writer(std::io::stderr)
        .init();

and in Cargo.toml

tracing = "^0.1"
tracing-subscriber = { version = "^0.3", features = ["env-filter"] }

@@ -2,6 +2,7 @@ use clap::{Parser, Subcommand};

mod publish;
mod runchecks;
pub(crate) mod utils;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we write mod utils and import things as use crate::crate_name::function?

}

/// Start log group
pub(crate) fn start_log_group(title: String) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of String we can use Cow in this way we can write:
start_log_group("Something") and start_log_group(format!("something")). I guess we just need to write a generic that implements Into<Cow>

Comment on lines 11 to 12
pub name: String,
pub path: String,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can use pub(crate) or remove the visibility attribute if they are used in the same file

@Luni-4
Copy link
Collaborator

Luni-4 commented Dec 1, 2023

@dcvz

This new logging seems a bit complicated, I was proposing to replace println! with info! macro from tracing. Adding this new macros and series of matches complicates the code in my opinion

@dcvz
Copy link
Contributor Author

dcvz commented Dec 1, 2023

@Luni-4 if we want a logger, then I'd say let's do it right. This make sure it outputs correctly also to the Github Action logs, meaning we'll be able to see highlighted warnings and errors easier. It's also just about the same amount of boilerplate for the changes you were asking for logger, tracer and using Cow.

IMO - I think this is fine.. but I guess its up to you all to decide.

@Luni-4
Copy link
Collaborator

Luni-4 commented Dec 1, 2023

I'm not against this new logger, just saying that is more complete, perhaps this is the right term, of what I have thought, but I'm fine with it

@dcvz
Copy link
Contributor Author

dcvz commented Dec 1, 2023

@louisfd @nathanielsimard this one is ready to go!

Comment on lines +50 to +54
if std::env::var("CI").is_ok() {
log!(log::Level::Info, "::group::{}", title)
} else {
log!(log::Level::Info, "{}", title)
}
Copy link
Member

Choose a reason for hiding this comment

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

Why use a different rendering depending of the CI? can ::group::{} works everywhere? Same question for all other occurences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, in Github Actions, ::group:: creates a grouped log that can be minimized / maximized. In our normal shells it doesn't really do anything except actually print out ::group::Title -- I thought this might be neater. But if we don't mind printing out ::group:: in our normal developer shells, then we don't need different rendering.

@nathanielsimard nathanielsimard merged commit 82f5722 into tracel-ai:main Dec 4, 2023
9 checks passed
@dcvz dcvz deleted the chore/ci-more-backends branch December 4, 2023 16:39
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.

None yet

4 participants