nasa / openmct

A web based mission control framework.
https://nasa.github.io/openmct/
Other
12k stars 1.24k forks source link

Application still loads with incorrectly configured time API / conductor plugin #1624

Open larkin opened 7 years ago

larkin commented 7 years ago

If you don't set a time system or clock before the application starts, and the URL doesn't contain information, and you don't use the Conductor plugin (which will set a default), the application loads but certain components that depend upon a timesystem will fail (e.g. imagery view). Some other views handle an undefined timesystem gracefully (table), but not all.

The question here, is should all views have a fallback when the Time API is not configured, or should the application fail to load and present a very clear error for developers? (loading but being buggy is not an option). e.g. is Time API a required configuration?

I'm thinking all views should have a fallback; I'm not sure how far they should fallback. If no time system, should they use first domain value? If no domain values, then what?

A follow-up item: should a plugin that uses the "start" event for initialization be able to prevent application loading if it encounters an error? Currently, throwing an error in a handler for the "start" event only prevents other handlers from being called, it does not prevent the application from loading.

@VWoeltjen input welcome.

Reported while helping debug problems with imagery.

VWoeltjen commented 7 years ago

The question here, is should all views have a fallback when the Time API is not configured, or should the application fail to load and present a very clear error for developers? (loading but being buggy is not an option). e.g. is Time API a required configuration?

The Time System provides fairly cosmetic information. Failing to load in the absence of that seems unforgiving, particularly if telemetry isn't what you're working on.

I think there are two levels of defaults here:

I'm thinking all views should have a fallback; I'm not sure how far they should fallback. If no time system, should they use first domain value? If no domain values, then what?

Not sure quite what is meant here. What would views need to use domain values for in this circumstance?

A follow-up item: should a plugin that uses the "start" event for initialization be able to prevent application loading if it encounters an error? Currently, throwing an error in a handler for the "start" event only prevents other handlers from being called, it does not prevent the application from loading.

That makes sense, particularly given the behavior of EventEmitter.

gdunge commented 7 years ago

On Jun 20, 2017, at 11:51 AM, Pete Richards notifications@github.com wrote:

A follow-up item: should a plugin that uses the "start" event for initialization be able to prevent application loading if it encounters an error?

If I may butt in here:

Preventing Open MCT from opening on certain errors sounds like a good idea to me. I had a problem today where Open MCT was acting strange, and I eventually figured out that CouchDB wasn’t running. The only indication (apart from the occasional “missing item” error) in Open MCT was that the little database icon in the bottom left corner was red.

It seems to me that the lack of the specified persistence store should be a fatal error for Open MCT. Quietly starting up a non-functional app is not what I want. In this specific case, Open MCT won’t contain the info in the specified store and the user will be astonished (violating the principle of least astonishment). I’d like a big error message that gives the user something to act upon.

— Doug Weathers, Design Engineer Immortal Data Incorporated “Data Systems for Deep Space and Time” Midland International Air and Space Port dougw@mac.com

larkin commented 7 years ago

@gdunge thanks for the inputs, very valuable here.

@charlesh88 Do you have a design for how Open MCT should display a "critical error" that would completely prevent the application from being used? i.e. similar to the load screen, where the entire interface is blocked and we can show arbitrary error text.

@VWoeltjen w.r.t. missing time config, I think it's possible to handle gracefully. but if it's invalid time config, then I think we throw a critical error (as a developer should be configuring it properly).

larkin commented 7 years ago

I think any use of the time API should require all time API settings to be valid, but no use of time API should not do validation.

The tutorial is in an interesting state where it sets a clock and clock offsets but never sets a time system. It works, but logs errors, and it probably shouldn't work. Using the time API without a time system is a big question mark.

It's worth including a bit about how the time API works in the tutorial, and properly setting a timeSystem. Have opened https://github.com/nasa/openmct-tutorial/issues/16

akhenry commented 6 years ago

Ideally, yeah, components that don't aren't implicitly temporal in nature should be able to carry on without the Time API being configured. Components that are temporal, but can fallback to a useful mode that doesn't depend on time data should do so.

Then there's the issue of degrees of support. Are there cases where a partially initialized Time API is useful? I'm not sure. Is a time system without default bounds useful? Probably not. Are bounds without a Time System useful? I would argue no.

I guess the question is how we communicate to users of the Time API that it's in an invalid state. The simplest thing to do for now is return undefined TimeSystem, bounds, etc. if they have not been defaulted and let client code handle those cases gracefully.