Improve error messages emitted from IF and plugins

[jmcook1186]

Sub of #655

What

Define and implement standards for error reporting that enable plugin developers to report clear, actionable information to users and make use of our IF logging protocols.

Why

As a user I want to be able to diagnose problems with IF from the error messages. As a developer I want to work with the IF logger and emit error messages in a way that integrates well with IF.

Context

We want to emit error messages using a finite set of predefined error classes so that we can provide specific documentation and advice to users in our documentation about the behaviours that lead to errors of each class and common remedies.

In our logging upgrade (#600 ) we are likely to implement a simple rerouting of console.x methods from plugins to the IF logger so that they can be reformatted and responsive to IF CLI flags. We can also use this same flow to improve how IF handles errors emitted from plugins.

First, we encourage plugin builders to use console.error() for their error messages and console.warn for warnings. IF then routes these to the Winston logger and prepends the name of the plugin that raised the error. The logger can also do some validation, for example we want error messages to come in the form:

CLASS: message

where the class is one of the predefined classes we publish in an if-core package. That package should include our errors.ts and strings.ts files so that they can be imported by plugin developers and we can refine and add to the files by creating new releases developers can install, rather than trying to convince plugin developers to copy files manually.

All a plugin builder need do is import the error classes they need from if-core and use console.error. Our IF logger handles everything else.

So, the actual actions to satisfy this criterion are to:

• add these error handling recommendations to the "building IF plugins" page on if.greensoftware.foundation
• to create a repo and release if-core containing common types and assets needed by plugin builders
• add if-core as a dependency to the template repository

Enhancing the error class list

Here's the list of error classes we currently use across our IF features and plugins

IF errors CliInputError ManifestValidationError ModuleInitializationError InvalidAggregationParamError InvalidGroupingError PluginCredentialError PluginInitalizationError WriteFileError AuthorizationError (plugin) APIRequestError (plugin) InputValidationError (plugin) UnsupportedValueError (plugin) ConfigValidationError (plugin) ReadFileError (plugin) WriteFileError (plugin) MakeDirectoryError (plugin)

Today we also maintain a set of messages in if/src/config/strings.ts. These are messages that can be emitted attached to one of the error classes. However, there are examples across the if repository and elsewhere where some messages are defines in strings.ts and others are written inline where the error is emitted, for example here are two errors emitted in time-sync.ts:

in this case the error is imported from strings

throw new InputValidationError(INVALID_OBSERVATION_OVERLAP);

in this case the error is defined inline

  throw new InputValidationError(
    `Unexpected date datatype: ${typeof date}: ${date}`
  );

It would be better to have all error messages from IF features and builtins defined in strings.ts and avoid defining them inline wherever possible, as this approach makes them more reusable across IF and plugins and enables us to document them centrally.

We should also consider deprecating the errorBuilder function everywhere across IF, as this is just a very simple function that we can do without but it makes it harder for plugins to conform to our error handling norms so we should remove it.

So the actions to satisfy this criterion are:

• review and refine error classes
• move all error messages from IF features and builtins to strings.ts
• add to error documentation on if.greensoftware.foundation so that all classes are documented and clearly defined, including behaviours that cause them and possible remedies.

SoW

• [x] Update IF error handler to do validation on error messages to ensure a valid error class is invoked

• [x] Check the list of error classes and identify any missing classes or classes we can remove.

• [x] Confirm error classes are precise, unambiguous and understandable. Errors should be user-centric, not IF-centric, for example

InputValidationError could be broken into more specific classes that describe what has actually failed, for example ParameterIsInvalid, RequiredParameterNotFound etc

We can have some exceptions that we can try to automatically recover from, e.g. RetryException (the plugin hit some rate limit and wants us to retry in X seconds).

• [x] Migrate all custom errors in IF to strings.ts and eliminate any unnecessary replication

• [x] Deprecate error builder function everywhere across IF

• [x] Add documentation to if.greensoftware.foundation and plugin template repo explaining that plugin builders should raise errors using console.error({CLASS}:{message}) and that the classes are to be found in errors.ts int he template repo (identical to the same file in the IF repo)

• [x] Define the behaviours that cause errors of each class to arise, common examples, and possible remedies. This can all be added to if.greensoftware.foundation/reference/errors.

• [x] Then we need to determine a strategy for how this list of error classes should be distributed to plugin builders and how plugin builders can integrate the IF error handling protocol (is documenting on IF site and adding our error builder to the template repository enough?)

• [x] If plugins return an error that we don’t recognize or can’t parse, we should fail loudly in the console and exit.

Acceptance criteria

• [x] error messages all use the ErrorClass: error message syntax and use a known, class from errors.ts. GIVEN the suggested changes have been merged WHEN I run ie --manifest with errors THEN the error is always attached to a known error class

• [x] the set of error types is documented on if.greensoftware.foundation GIVEN the documentation exists WHEN I navigate to if.greensoftware.foundation/reference/errors THEN I see the list of error types with a brief explanation of the kinds of behaviours that cause them to be emitted, with real examples from our codebase

• [ ] the README for each builtin includes a comprehensive list of the error messages the builtin can emit, with reasons and remedies linking to the appropriate docs. GIVEN the suggested changes have been merged WHEN I visit if.greensoftware.foundation/reference/errors THEN I find a comprehensive list of the error types F can emit and understand when and why they are used.

• [x] unit tests are updated where necessary GIVEN the suggested changes have been merged WHEN I run npx jest --coverage THEN I see a coverage report showing 100% coverage and 100% passing

• [x] we have a repository, if/core and an associated npm package containing common assets needed by plugin builders, especially errors.ts and strings.ts. GIVEN this package exists WHEN I run npm i @grnsft/if-core THEN I receive the necessary files to use IF's error classes.

• [x] if-core is a dependency of if-plugin-template GIVEN the package exists and the dependency is added to the package.json in if-plugin-template WHEN I download if-plugin-template and run npm i THEN I see if-core in my local node-modules

• [x] if-core is a dependency of if, used as a source of errors (all errors should be imported from if-core) GIVEN the package exists and the dependency is added to the package.json in if WHEN I download if and run npm i THEN I see if-core in my local node-modules

• [x] if-core is a dependency of if, used as a source of plugin types (all types should be imported from if-core) GIVEN the package exists and the dependency is added to the package.json in if WHEN I download if and run npm i THEN I see if-core in my local node-modules

[narekhovhannisyan]

Currently, I have attached a branch with a possible solution to this problem.

1. Separation of errors by logical chunks such as CLI, Manifest, Plugin, etc.
2. Log, warn, error and info have appropriate colors.
3. Formatting will look like this:

(severity)[scope]
<message here>

4. Still haven't covered ordinary console class logs, will work on it.
5. Exhaust can also be included in scopes.

[jawache]

Some initial thoughts on this.

• Can 3rd party plugin authors log using our framework (I don't think so right @narekhovhannisyan?). If not how can we make it so they can? 95% of all the errors are to be with plugins and the best error messages we are going to get are from the plugins themselves.
• How will can users configure the logs they want to receive? E.g. what are the proposed CLI params to support logging?
• I don't think things like the severity or other things should be on a seperate line, that way if you grep a large log, you will only get the line and you will have to open the log and navigate to it and then look at the line before to find the severity. Makes more sense to be on the same line.
• [!important] is unecessary, that's just how you write markdown in GitHub so it shows up like so

  [!important] Hello

• We need to discuss the issue of logging much more broadly than just how the message should be displayed, I'm not sure if we have a ticket for that but we should discuss how users will interact with the CLI, common errors and walk through step by step how you can figure out from an error what you need to do to solve it. Things like helping them to identify the line in the manifest file and plugin is part of that story, but what other information might be useful to help diagnose issues.

[jmcook1186]

@narekhovhannisyan @jawache let's spike on this because I anticipate this blocking some other priority tasks such as https://github.com/Green-Software-Foundation/if/issues/337 and https://github.com/Green-Software-Foundation/if/issues/615. We need to determine:

• what exception standards we want to implement
• what we can put in place to help plugin developers conform to those standards
• how IF handles exceptions from plugins that don't conform to our standards
• can we list all possible exceptions that can be thrown by an IF run?

Some of this might be covered by @narekhovhannisyan 's work detailed in comments above, but we need to understand where we are and get aligned on what specific outcomes we want to see.

[jawache]

Thanks @jmcook1186, the top level issue for all of this is this in my opinion: https://github.com/Green-Software-Foundation/if/issues/600 that's what I put in the Notion doc.

The intention of that issue was a broader discussion, based partly on on my personal challenges over the last few months trying to build manifest files and how our logging is lacking on many levels (not just the way we express errors in the console).

I'll put my thoughts on the top level ticket but I'd rather we have a broader conversation first, and then from that narrow into specific tasks like how we want exceptions to be logged.

[zanete]

Blocked as waiting for #600

[zanete]

needs a sign off after the call on tuesday next week

[zanete]

@narekhovhannisyan please review the AC to confirm it's clear / ready for dev + add a size

[zanete]

@narekhovhannisyan please see the above comment to get this issue moving to the next stage

[narekhovhannisyan]

@jmcook1186

Add documentation to if.greensoftware.foundation and plugin template repo explaining that plugin builders should raise errors using console.error({CLASS}:{message}) and that the classes are to be found in errors.ts int he template repo (identical to the same file in the IF repo). 

Here it says that users should rise error via console.error. However, users should throw an error to raise instead of logging. So it means our logger should handle formatting instead of the end user which will simplify user experience.

[narekhovhannisyan]

moving strings.ts will create difficulties for us and for the users since we have to create PRs every time to add error message or fix already existing one. At the moment only common types and error classes can be moved. We will see on the go what else we can move.

[narekhovhannisyan]

Sizing as L

[jmcook1186]

ok so if I understand correctly, users throw new Error() invoking one of our classes that they import from if-core. strings.ts can stay in IF and not migrate to if-core.

[narekhovhannisyan]

@jmcook1186 we can move global config validation as a utility to if-core. It's redundant and plugin developers should write the same structure again and again.

  const validateGlobalConfig = () => {
    if (!globalConfig) {
      throw new GlobalConfigError(MISSING_GLOBAL_CONFIG);
    }

    const globalConfigSchema = z.object({
      coefficient: z.number(),
      'input-parameter': z.string().min(1),
      'output-parameter': z.string().min(1),
    });

    return validate<z.infer<typeof globalConfigSchema>>(
      globalConfigSchema,
      globalConfig
    );
  };

We can convert it into function and make it utility.

[narekhovhannisyan]

@jmcook1186 If plugins return an error that we don’t recognize or can’t parse, we should fail loudly in the console and exit.

What kind of logging do we want for this? currently we have logging which forwards user to our issue template.

[jmcook1186]

This should just refer to users using an error class that we don't already support - in which case we need to include an error class in if-core that covers unknown error classes!

e.g. error UnsupportedErrorClass: plugin threw error class: <errorClass> that is not recognised by Impact Framework

[jmcook1186]

Maybe we need a generic error class to act as a catch-all that we apply whenever a plugin just uses raw console.log(), e.g. it gets parsed as Unspecified Error: The user has not provided an IF error class. The message was: <error message>

But if a user does provide an error class but it's not one of our supported ones, it throws an UnsupportedErrorClass exception.

wyt?

[zanete]

PR of First part will be ready this afternoon There will be at leat 4 PRs in total This feels like going over L estimate

[narekhovhannisyan]

@jmcook1186 I have another suggestion which is simplier in this case. At the moment we already have a handler for errors, and it already checks if error classes that are included in our list. If the error class is unknown, then logs it with the reference to issue. We can tune that behavior to error UnsupportedErrorClass: plugin threw error class: <errorClass> that is not recognized by Impact Framework. WDYT?

[jmcook1186]

@narekhovhannisyan ok that seems good. What happens if the plugin builder provides no error class and just uses raw console.x?

[narekhovhannisyan]

@jmcook1186 we should ignore it in our error handling layer. It will be caught by logging handler

[jmcook1186]

@narekhovhannisyan ok, so it would only be printed to the console if --debug is raised?

[narekhovhannisyan]

let me check

[narekhovhannisyan]

with debug flag it logs like this INFO: 2024-06-11T09:35:37.530Z: sum: test log without debug flag test log

[jmcook1186]

ok that seems fine. I guess what we're saying to plugin builders is that if you want to give your users the best UX, tight integration to IF and access to docs and debugging support then you have to use our error handlers, and we'll make that as easy as possible, but if you don't we won't break your ability to emit messages to the console. That makes sense to me.

@narekhovhannisyan

[narekhovhannisyan]

@jmcook1186 agree. So we are limiting the use of error classes that are not supported from our side. Yeah?

[zanete]

@narekhovhannisyan to prepare a break down of the topics and how it could be divided between all who can help

[narekhovhannisyan]

@jmcook1186 can you please help me with this one?

the README for each builtin includes a comprehensive list of the error messages the builtin can emit, with reasons and remedies linking to the appropriate docs.

[zanete]

Related issue #602 for @jmcook1186