search

google / wireit

Wireit upgrades your npm/pnpm/yarn scripts to make them smarter and more efficient.
Apache License 2.0
6.09k stars 107 forks source link

Display summary with timing data #525

Open aomarks opened 2 years ago

aomarks commented 2 years ago

On each iteration, show a summary of how many scripts ran/were fresh/were restored from cache -- and how long it took.

We could probably remove some noisy logging like "No command to execute" and "Already fresh" as well as part of this.

joshkraft commented 1 year ago

Hi @aomarks - working on a POC of this enhancement and had a quick question.

Do you think it would be appropriate to implement this within the default-logger.ts, or would it be better for that to stay relatively stateless? As an alternative, it seems like it might be possible to implement this in standard.ts.

aomarks commented 1 year ago

Hi @joshkraft. Cool! Definitely welcome a contribution here!

Probably many valid ways to do this, but my first thought would be a new class called e.g. SummaryLogger which takes another logger to pass events through to, but as it sees events goes by, also keeps a counter of the interesting events we want to summarize, and then when you call .printSummary() or something it displays that and maybe resets the counters? That way we could call it once when we're in normal mode, and on each iteration when we're in watch mode.

Also note I just added a new WatchLogger at https://github.com/google/wireit/blob/main/src/logging/watch-logger.ts -- I think we'd want to instantiate the SummaryLogger both here for watch mode, and also in cli.ts for standard mode. (I'm assuming we want summaries for watch mode, but that's actually a good question -- do we?)

It would also be great to discuss here exactly what counts we should actually display -- have you thought about what you'd find most useful?

joshkraft commented 1 year ago

Nice, I like the SummaryLogger approach. I'll give that a shot.

As far as logging in watch mode - I'd personally be fine with having the summary display in both watch mode and standard mode. Taking a look at WatchLogger, it seems like it might make sense to only display the summary on 'interesting' iterations?

For stats - so far I've got something like this:

🏁 [summary] Executed 9 script(s) in 1.21 seconds
    Ran:              9 (100%)
    Skipped (fresh):  0 (0%)
    Skipped (cached): 0 (0%)

I could see an argument for making this more verbose (error stats?), or more concise (does the user care if something was skipped because it was fresh or cached?).

Unfortunately, I haven't used wireit enough at this point to have an educated opinion on what would be most useful. Do you have any thoughts?

aomarks commented 1 year ago

Nice, I like the SummaryLogger approach. I'll give that a shot.

As far as logging in watch mode - I'd personally be fine with having the summary display in both watch mode and standard mode. Taking a look at WatchLogger, it seems like it might make sense to only display the summary on 'interesting' iterations?

Yep, I agree we should only display it for interesting iterations. I don't think much work will be needed in WatchLogger to make that happen -- we currently only send any events to the DefaultLogger for interesting iterations, so we'd just replace DefaultLogger with SummaryLogger (which itself would take the DefaultLogger) and it should just work how we want.

For stats - so far I've got something like this:

🏁 [summary] Executed 9 script(s) in 1.21 seconds
    Ran:              9 (100%)
    Skipped (fresh):  0 (0%)
    Skipped (cached): 0 (0%)

I could see an argument for making this more verbose (error stats?), or more concise (does the user care if something was skipped because it was fresh or cached?).

Unfortunately, I haven't used wireit enough at this point to have an educated opinion on what would be most useful. Do you have any thoughts?

That seems like a great start. And I like the checker flag emoji :)

Another case to consider are "no-command" scripts (scripts that just have dependencies and nothing else). I think we should ignore those completely, they don't seem interesting to summarize. (They are identified by having reason set to no-command on their success event).

joshkraft commented 1 year ago

Hi @aomarks, went ahead and created a PR for this functionality, feel free to take a look when you get a chance: https://github.com/google/wireit/pull/691