Combine error taming, rich assert, and console virtualization

[erights]

Fixes #440

[kriskowal]

This looks like it’s starting well.

We will have to come up with a sensible way to use console as a shim vs assert as a module. There’s a tension there. It might be good to separate the layers such that assert is a module that relies on the underlying console for reporting.

Also, console implementations do generally implement sprintf style log methods. We probably can and ought to respect those as valid ways to pronounce details.

[erights]

Hi @kriskowal this is ready for review. However, in getting here, this got rather large. If you'd like, we should discuss how to break it up into reviewable-sized chunks.

[kriskowal]

This is a partial review. I am still digesting.

[kriskowal]

This is a partial review. I am still digesting.

Capturing out-of-band feedback: I’m recommending that we use an API like

} catch (error) {
  assert.note(error, details`number too small ${num}`);
  throw error;
}

(Which I believe would be more palatable than logToConsole(...asLogRecord(...)))

Recommending that we use the catch-and-rethrow in SES module loading as a proving ground for this, and as an example of usage in this change.

I retract my suggestion that we use the term cause (in favor of using note), since I believe there’s a separate need for that term for in-band error annotation, e.g., new Error('Could not find spoon', networkErrorThatCausedFailureToFindSpoon), where code can catch and programmatically inspect the cause chain to make relevant recovery decisions (like whether to retry).

[kriskowal]

I’m intrigued by the idea that the assertion library would provide “progressively enhanced” behavior depending on whether the console in scope is tamed or untamed. On the one hand, emitting the annotations to the debug system is good. On the other hand, reporting them to the untamed console, regardless of whether they’re later recovered, is a nightmare developer experience.

I will look more closely at this change. I suspect that the untamed case would need a mechanism for correlating related log messages, perhaps using a sequential identifier.

[erights]

Ready for review!

[erights]

Adding @warner and leaving @michaelfig as optional reviewers. Only review from @kriskowal is critical.

[erights]

This is a partial review. I am still digesting.

Capturing out-of-band feedback: I’m recommending that we use an API like

} catch (error) {
  assert.note(error, details`number too small ${num}`);
  throw error;
}

(Which I believe would be more palatable than logToConsole(...asLogRecord(...)))

Done.

Recommending that we use the catch-and-rethrow in SES module loading as a proving ground for this, and as an example of usage in this change.

Still TODO. After this PR is merged but before integration?

I retract my suggestion that we use the term cause (in favor of using note), since I believe there’s a separate need for that term for in-band error annotation, e.g., new Error('Could not find spoon', networkErrorThatCausedFailureToFindSpoon), where code can catch and programmatically inspect the cause chain to make relevant recovery decisions (like whether to retry).

Done.

[kriskowal]

I would like to at least discuss the proposed console API extension. Apart from that, all remaining feedback is for your discretion.

[michaelfig]

I looked carefully at the types, and from that standpoint, LGTM!

[erights]

Ready for review again. For assert.js and console.js, I recommend reviewing the whole files rather than the diffs.

[erights]

For assert.js and console.js, I recommend reviewing the whole files rather than the diffs.

Nevermind. I squashed and both modules are new compared to master, leaving little choice but to compare entire files. My apologies; I should have squashed less aggressively.

[kriskowal]

Thank you for working in my feedback on the API. This seems better.

I am still concerned about the special relationship between assert and console. The console gets constructed with view into a particular assert module instance, but I expect each compartment will have its own assert module instance and errors will be forwarded only to the root console.

How do we ensure that details attached in a deeply nested compartment surface to the outermost console?

[erights]

Back to draft until it is again ready for review. Please spend no more time reviewing the current state.

[erights]

Ready for review again! Includes new design doc at https://github.com/Agoric/SES-shim/tree/console/packages/ses/src/error

[kriskowal]

Approved with nits. Thanks!

[kriskowal]

TODO

[erights]

Not sure what you want me to do here. In moving this package from the agoric-sdk monorepo to the SES-shim monorepo, I had to regress the testing to use tap or tape rather than AVA. I put this comment there to help when we reverse this regression, once SES-shim switches to use AVA.

I'll leave this unresolved, but I am not inclined to postpone merging till we resolve this.

[kriskowal]

TODO

[erights]

Likewise