NASA-AMMOS / aerie-ui

The client application for Aerie.
https://nasa-ammos.github.io/aerie-docs/
MIT License
29 stars 5 forks source link

Add expansion errors to error console #302

Open cohansen opened 1 year ago

cohansen commented 1 year ago

When execute a command expansion in the UI it should propagate the errors into the console rather than just showing Error: internal error.

To replicate this easily, create an activity with 0 duration and run an expansion. Right now you'll get an error with the following message: invalid ISO 8601 string: null, but I'm going to improve that.

dyst5422 commented 1 year ago

Our original intent with command expansion errors was that:

  1. Errors that come from expansions (introduced by the user code of expansions) should be emitted in the expanded sequences themselves with the command stem of $$ERROR$$ so as to allow for partially successful expansion (and the appropriate tracing of that)
  2. Errors related to expansion that are not specifically due to the expansion but a bug in our code should propagate normally as bug errors

I think the two types of errors should have different behaviors on the UI. The specifics of those behaviors I can't exactly specify.

I think having additional behavior for case 1 outside of it embedding it in the sequence in the UI is a good idea, but unsure of what exactly that should be - I think NOT in the error console because it's really not an "error" of the system, but a behavioral result of the users.

I think case 2 most likely belongs in the error console.

dyst5422 commented 1 year ago

The specific error you listed is interesting because it's a little bit of 1 and a little bit of 2.

1 because it's a direct result of the user's expansion implementation (generating the null value) 2 because it occurs outside of the user code execution environment (trying to interpret the null as an iso string)

Ultimately though, this seems like an error in our implementation of catching the error and should be forced more squarely into 1 so it's clear that it's an issue the users need to address, not the Aerie team.

That's not the point of this issue though, so should probably be tracked elsewhere

cohansen commented 1 year ago

This issue was specifically talking about your second case (in your first post), right now if we can't run the expansion due to an error so we just get a generic error message that isn't propagated from the sequencing server itself.

Interesting read about error handling from the expansions itself though, do we have this tracked somewhere as a decision or just does it just exist in code?

dyst5422 commented 1 year ago

I don't know if we formally recorded this in documentation anywhere. We definitely should though because it's a core philosophy of command expansion strongly driven by previous solutions' command expansion behavior and mission feedback.

Generally, this philosophy of enabling partial success is a key concept for software that is intended not as a total replacement for users, but as an assist to users. It was a hard learned lesson for me over the last 6 years of mission software work.