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

feat: APIs for reporting activity to Station #124

Merged
merged 14 commits into from
Mar 23, 2023
Merged

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Mar 22, 2023

Implement the following new APIs:

namespace Zinnia {
   // omitted: existing APIs like `peerId` and `walletAddress`

   /** Report activities to the Station. These messages are displayed in the main UI. */
   activity: {
     /** Report an informative status update, e.g. "Connecting to the network." */
     info(message: string);

     /** Report an error, e.g. "Cannot connect to the orchestrator." */
     error(message: string);
   }

   /** Report completion of a single job */
   jobCompleted();
 }

Rework the implementation of Console APIs to call our internal printer so that we can intercept these logs and potentially handle them in different ways.

Example output:

Screenshot 2023-03-23 at 10 08 32

See #75 and #121.

bajtos added 5 commits March 22, 2023 09:20
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
@bajtos bajtos requested a review from juliangruber March 22, 2023 12:38
@bajtos bajtos mentioned this pull request Mar 22, 2023
11 tasks
@juliangruber
Copy link
Member

Rework the implementation of Console APIs to treat all logs as debug logs and print them to stderr, regardless of their severity.

What do you think about using another fd (3) for communication to Station? This way we can keep stdout/stderr for the module, which is nicer for when Zinnia is used independently of Station

@bajtos
Copy link
Member Author

bajtos commented Mar 22, 2023

Rework the implementation of Console APIs to treat all logs as debug logs and print them to stderr, regardless of their severity.

What do you think about using another fd (3) for communication to Station? This way we can keep stdout/stderr for the module, which is nicer for when Zinnia is used independently of Station

I see what you mean. If we want to use Zinnia for building something else than a Station module, then yes, it makes sense to provide console.log for printing to stdout, console.error for printing to stderr, and simply ignore other Zinnia APIs.

On the other hand, while building Zinnia modules, I find it useful to be able to discard debug logs and see only the activity that will be reported to the station.

❯ zinnia run runtime/tests/js/station_apis_tests.js
debug-info
debug-error
[13:58:20.331  INFO] information
[13:58:20.331 ERROR] problem
[13:58:20.331 STATS] Jobs completed: 1
❯ zinnia run runtime/tests/js/station_apis_tests.js 2>/dev/null
[13:57:59.074  INFO] information
[13:57:59.074 ERROR] problem
[13:57:59.074 STATS] Jobs completed: 1

There are different ways how to meet both requirements. For the Station, I will be building a different reporter in zinniad, one that is aware of multiple modules and prints activities in ND-JSON format. In this pull request, we can focus on module development use cases only.

Using fd 3 is possible, although I don't know if that's supported on Windows.

If we decide we want to make zinnia usable outside of Station context, which I am not sure if we should, then I prefer to keep Console APIs as they are now (console.log goes to stdout, console.error goes to stderr) and add a CLI flag to suppress messages from Console APIs when debugging.

Building on top of my example above:

❯ zinnia run --silent runtime/tests/js/station_apis_tests.js
[13:57:59.074  INFO] information
[13:57:59.074 ERROR] problem
[13:57:59.074 STATS] Jobs completed: 1

What do you think, @juliangruber?

BTW it's common for applications to log to stderr, e.g. Node.js debug module and Go's built-in log module.

@juliangruber
Copy link
Member

On the other hand, while building Zinnia modules, I find it useful to be able to discard debug logs and see only the activity that will be reported to the station.

I guess you can still do that, just not as accessible:

$ zinnia run file >/dev/null 2>/dev/null 3>&1

But good point, the consumability of the station events does become a problem then.

Another option: By default log console.log to stdout, console.error to stderr and zinnia to stderr. This way you can always read everything. Then when Station launches Zinnia, it can override this behavior through something like this:

$ zinnia run file --station-fd 3

Whether this works on Windows is another question, I didn't find an obvious answer with a quick google search.

To me it's just very unexpected that console.error would land on stdout, because all other headless JS environments that I do behave differently. This isn't saying it's a wrong move for Zinnia, but it is a surprise.

I'd still suggest to keep this as is currently, and iterate if it becomes an issue.

@juliangruber
Copy link
Member

BTW it's common for applications to log to stderr, e.g. Node.js debug module and Go's built-in log module.

💯, it's just not common for console.error to log to stdout


/// Reporter that collects all recorded events, useful for testing.
pub struct RecordingReporter {
pub events: RefCell<Vec<String>>,
Copy link
Member Author

Choose a reason for hiding this comment

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

We want the recorder to be immutable so that we can pass it around and have multiple immutable references. However, our inner state is mutable. The solution is called "interior mutability", see https://doc.rust-lang.org/book/ch15-05-interior-mutability.html

bajtos added 2 commits March 22, 2023 14:16
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
@bajtos
Copy link
Member Author

bajtos commented Mar 22, 2023

it's just not common for console.error to log to stdout

Just to clarify, it's console.log that's logging to stderr now.

Then when Station launches Zinnia, it can override this behavior through something like this:

$ zinnia run file --station-fd 3

The Station will launch a different Zinnia binary (zinniad) that can handle logs differently. Let's defer the discussion of logging for Station until I start working on that please.

To me it's just very unexpected that console.error would land on stdout, because all other headless JS environments that I do behave differently. This isn't saying it's a wrong move for Zinnia, but it is a surprise.

This is a valid point. Let me think about this more.

bajtos added 3 commits March 23, 2023 08:49
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
@bajtos
Copy link
Member Author

bajtos commented Mar 23, 2023

I decided to make the following changes:

(1)
Let Console APIs behave consistently with other JS runtimes like Node.js and Deno:

  • Most Console methods print to stdout
  • console.warn and console.error print to stderr

This way, Zinnia CLI can be used as a replacement for Node.js or Deno with additional APIs for easier interaction with libp2p, IPFS and Filecoin.

(2)
Add colours to activity logs to make them stand out from regular console logs.

Screenshot 2023-03-23 at 10 08 32

This affects only people using Zinnia to build Station Modules. Code not invoking activity/job-completion reporting APIs will never trigger these coloured logs.

Note that these two changes affect only Zinnia CLI for local development. We can implement different logic for handling logs & activities in zinniad that will run inside the Station.

I consider this PR ready for final review and merging. @juliangruber PTAL 🙏🏻

bajtos added 2 commits March 23, 2023 10:17
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
Comment on lines +6 to +13
#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Deserialize_repr)]
#[repr(u8)]
pub enum LogLevel {
Debug,
Info,
Warn,
Error,
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Deserialize_repr allows us to parse numeric levels provided by Deno Console implementation and treat them as an enum in the rest of our codebase.

fn report(&self, scope: &str, msg: &str, color: Color) -> Result<()> {
if use_color() {
let mut spec = ColorSpec::new();
spec.set_fg(Some(color)).set_bold(true);
Copy link
Member Author

Choose a reason for hiding this comment

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

A discussion point:

Is the bold font too much? Maybe adding the colours is enough to make activities stand out?

Bold:

Screenshot 2023-03-23 at 10 08 32

Regular:

Screenshot 2023-03-23 at 10 25 53

Please note that the exact visual representation depends on the terminal and the configured colour scheme. Different users may get slightly different visuals.

Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
Copy link
Member

@juliangruber juliangruber left a comment

Choose a reason for hiding this comment

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

I skipped over some parts that went above my head (for now). Lgtm, great work Miro!

@bajtos bajtos merged commit bb0159d into main Mar 23, 2023
@bajtos bajtos deleted the feat-report-job-completion branch March 23, 2023 13:43
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.

2 participants