nasa / openmct

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

URL-based Status Indicator Plugin #1286

Closed larkin closed 6 years ago

larkin commented 7 years ago

After talking with a few customers it would be great to have a simple plugin for registering URL-based status indicators, i.e. those that poll a URL and show "connected/disconnected" or similar based on the availability of the URL.

Basic functionality might be

var googleIndicator = new openmct.plugins.IndicatorPlugin({ 
    url: 'https://google.com', 
    icon: 'database',
    label: 'Google',
    interval: 15000
});
openmct.install(googleIndicator);

And then you'd get an indicator icon that would go red/green based on whether or not it is connected to the server.

evenstensberg commented 7 years ago

Do you want the notification to be native to the device or in the browser? @larkin

evenstensberg commented 7 years ago

I was thinking about having this in the lower left together with localStorage, is that okay?

akhenry commented 7 years ago

@ev1stensberg notification is not necessary right now. My preferred approach here is the indicator in the lower left that you suggested. There is provision in the API for defining new indicators in this fashion, see https://nasa.github.io/openmct/docs/guide/#indicators-category

The ElasticIndicator (part of the Elasticsearch plugin) pretty much does what this ticket describes already, but we need a more generalizable version of it with a configurable URL.

evenstensberg commented 7 years ago

@akhenry Okay, got it. You want this under /persistence I assume?

larkin commented 7 years ago

@ev1stensberg We're in a period where this is spanning our new API and old API. In this case, we desire a new-style plugin but the plugin will have to utilize old-style APIs. As a new-style plugin, you should define it in src/plugins/URLIndicatorPlugin.js and register it as an openmct plugin in src/plugins/plugins.js. It would then be utilized as described in the initial issue description, above.

Because it's a new-style plugin, it will not register a bundle. Instead, your plugin would construct and return a install function which would be called with an openmct object. As we have not defined the new API for registering indicators, you would use the transitional API to register the indicator: openmct.legacyExtension('indicator', indicatorDefinition);.

Since new-style plugins are not well documented (we're working on this right now), here's some sample code to start with:

in src/plugins/URLIndicatorPlugin.js

define(function () {
    function URLIndicatorPlugin(options) {
        // Build an implementation with `options` similar to ElasticIndicator.js
        var MyImplementation = constructPlugin(options); 
        // Return an install function.
        return function install(openmct) {
            openmct.legacyExtension('indicator', {
                implementation: MyImplementation, 
                depends: []
            });
        };
    }

    return URLIndicatorPlugin;
});

in src/plugins/plugins.js


// require URLIndicatorPlugin, register.
plugins.URLIndicatorPlugin = URLIndicatorPlugin;

Let me know if you have any questions!

evenstensberg commented 7 years ago

Sure! Any forum/particular place you want me to ask those questions?

larkin commented 7 years ago

@ev1stensberg the best way is to either post here or open a pull request for discussion & mention me. For private discussions, email us at arc-dl-openmct@mail.nasa.gov.

evenstensberg commented 7 years ago

src/plugins/URLIndicatorPlugin.js turns into src/plugins/URLIndicatorPlugin/URLIndicatorPlugin.js .

evenstensberg commented 7 years ago

@larkin Can you check if I done the structure right?

Also, what's the next step to get the indicator rendered on the screen? Do I need it in some register first?

larkin commented 7 years ago

@ev1stensberg the structure is good. You'll need to pass through options from the plugin as registered in plugins.js to the URLIndicatorPlugin, I'd recommend that you implement that functionality in URLIndicatorPlugin.js and change plugins.js to plugins.URLIndicatorPlugin = URLIndicatorPlugin;.

As for "why isn't the indicator rendering," that's my fault. Change URLIndicatorPlugin.js from 'indicator' to 'indicators'. After that, you should see the indicator in your status bar, e.g. screen shot 2017-01-23 at 10 43 38 am

evenstensberg commented 7 years ago

Great. Not having options avaliable yet was intended. I wanted to get the icon rendered on paint before doing the actual implementation. Thanks for the fix 👍

evenstensberg commented 7 years ago

@larkin Do you have a set of two icons, for connected and disconected?

larkin commented 7 years ago

@ev1stensberg for consistency with existing indicators, we use color (assigned via css class) to indicate status and don't have different icons for different states. You can implement this in indicator.getGlyphClass(). Valid return values are:

As minimum requirements, the plugin should support specifying a single custom icon to use. It should also provide a default icon if one is not chosen: icon-database works for now.

evenstensberg commented 7 years ago

Great, will publish the PR before the weekend, possibly on saturday/sunday if time is limited!

evenstensberg commented 7 years ago

@larkin Do you want only those options provided in the initial ticket or some more/others as well?

larkin commented 7 years ago

@ev1stensberg the options above are just a sample, you are free to implement extra options as you work out the details, but when in doubt, KISS. It's easy to add options later and hard to remove them.

Keep in mind, it must be simple to use. url should be the only required option, the rest should have reasonable defaults / fallbacks.

evenstensberg commented 7 years ago

Is the URL external? If the route is outside the app ( i.e google.com ) CORS kicks in

larkin commented 7 years ago

@ev1stensberg assume the url could be outside the app and assume that CORS will be properly configured.

evenstensberg commented 7 years ago

@larkin What/where is the scope of the indicators?

larkin commented 7 years ago

@ev1stensberg Ah, well you could implement a custom template, in which you could use a custom controller, which would allow you to get a reference to scope, but that's a total mess and we don't want that. Not to mention you'd have to re-implement the stuff that is in the base indicator template. So let's look for a better solution-- giving indicators a $scope to work with.

Look at BottomBarController.js which handles instantiation of indicators. I would add a dependency on $scope for the BottomBarController, and then pass that to the Indicators as they are instantiated.

evenstensberg commented 7 years ago

Pushed a new commit to the branch, have a look at it. Seems like app.module needs to accept CORS requests, but also the express server. I've tried setting res.header accordingly. From my very limited Angular knowledge( with ironically google ( from the issue ticket url ) ) , I've found out that there's multiple CORS requests ( most probably ), that needs to be resolved. Added cors to dependencies, will allow us to target specific urls later ( like localhost:8080/urlRedirect ) that will later allow one cors request, of which is from the user.

Furthermore, this needs investigation, probably from a person known with Angular

larkin commented 7 years ago

@ev1stensberg For testing purposes, use http://localhost:8080/ instead of google as your test url. That way, you wouldn't need to do anything at all about CORS-- no changes to FrameworkLayer.js. For this testing indicator, it would just tell you "the development server is still alive."

The development server (express) is not used in any production deployments-- I apologize that this is not very clear. So you should not need to make any changes app.js either.

evenstensberg commented 7 years ago

Great, saves me a lot of time 👍

evenstensberg commented 7 years ago

@larkin Question: If I were to pass options along with [$http, $interval], how would I do that? My impression is that openmct injects the http and interval modules, but if I call the module ( with options from the ticket ) it gets overridden. Suggestions?

larkin commented 7 years ago

@ev1stensberg in this case, you shouldn't inject the options as dependencies. Those definitions are defined globally and have only a single value at runtime, where as there might be multiple URL indicators. To handle this, the install function you return should construct a indicator implementation with the proper configuration.

Let me know if that isn't clear and I can look at your latest branch and give a more specific answer.

evenstensberg commented 7 years ago

In URLIndicator I'm having function URLIndicator(options, $http, $interval) {...} and if I call the function, $http nor $interval get defined. What I meant with options; is arguments to the function. A bit unclear at my end

Edit: As the params doesn't get called in userland

evenstensberg commented 7 years ago

I did this, trying to get the argument injected. Dunno what the workaround for this is, getting an error about the injection when running. Template w/scope?

skjermbilde 2017-01-26 kl 11 15 37

larkin commented 7 years ago

@ev1stensberg You can do it by extending the URLIndicator class and injecting a reference to options, something like:

define([
    './URLIndicator'
], function (
    URLIndicator
) {

    return function URLIndicatorPlugin(opts) {

        function CustomURLIndicator() {
            this.options = opts;
            URLIndicator.apply(this, arguments);
        }

        CustomURLIndicator.prototype = Object.create(URLIndicator.prototype);

        return function install(openmct) {
            openmct.legacyExtension('indicators', {
                "implementation": CustomURLIndicator,
                "depends": ["$http", "$interval"]
            });
        }
    }

});
evenstensberg commented 7 years ago

Thanks, sorted out now, finally. What do you mean by "label" in the original ticket?

evenstensberg commented 7 years ago

https://github.com/nasa/openmct/pull/1417 Most of this is boilerplate, notify me if you want something changed or if I misunderstood this issue.

evenstensberg commented 6 years ago

Could close in favor for #1417

evenstensberg commented 6 years ago

Close-able?

larkin commented 6 years ago

Indeed, thanks @ev1stensberg !