Grok debugger migration

[Kerry350]

Summary

This PR performs a full new platform migration of the Grok Debugger dev tool.

There isn't really anything special to mention - there aren't any temporary measures or "hacks" as this is a pretty small app and all needed APIs / features were available.

This blog post contains some examples that can be used for testing.

[Kerry350]

Just noticed I forgot to add real validation for the registerGrokSimulateRoute route (there's currently a TODO with it). I'll add this (doesn't really need to delay review).

[Kerry350]

Validation added.

[jasonrhodes]

Code is looking great, I will run this later to make sure it works, and also let CJ and his team look it over as well.

@Kerry350 is there an easy way to test the different licensing scenarios to make sure they behave the same way as they used to? That part is confusing for me so I don't know if I'd be testing it correctly, maybe @cjcenizal can help there?

[cjcenizal]

@jasonrhodes I'll test out some licensing scenarios. Thanks for highlighting that. According to our subscription matrix it's available in OSS, so it should be available in all licenses:

[cjcenizal]

Oh shoot, why does our subscription matrix claim that it's OSS? That's not right -- it's in X-Pack! I'll try to track this down.

[cjcenizal]

OK, figured it out. I was looking at the wrong entry. It's in Basic:

[cjcenizal]

Tested locally, code LGTM! I tested on a Basic and Trial license.

[jloleysens]

@Kerry350 thanks a ton for taking on this work - it's looking great so far 🎉.

I left a suggestion and one change request. I also tested locally and am happy for this to be merged once @cjcenizal concerns are addressed and my comment regarding public plugin registration is addressed.

[jloleysens]

I am not convinced we need to wrap both license state and the http service in a class. IRouter has quite a well designed API so my suggestion would be to create the router on the plugin and pass that in as a route dependency. Then we need to just wrap the route handler. For example see x-pack/plugins/index_management/server/services/license.ts.

[Kerry350]

@cjcenizal @jloleysens Thanks both for the reviews 👍 I'll start addressing feedback now.

[Kerry350]

@jloleysens I'd like to get your thoughts on keeping all registration synchronous, specifically I can't think how to provide the disabled value to devTools.register that way.

In ed846a1 I've changed things so that if the route is navigated to directly, and the license isn't active, the app still mounts but now displays an error callout (modelled off of search profiler's callout).

But this doesn't handle disabling the tab. The following would work:

async setup(coreSetup, plugins) {
    registerFeature(plugins.home);

    const license = await plugins.licensing.license$.pipe(first()).toPromise();

    plugins.devTools.register({
      order: 6,
      title: i18n.translate('xpack.grokDebugger.displayName', {
        defaultMessage: 'Grok Debugger',
      }),
      id: PLUGIN.ID,
      enableRouting: false,
      disabled: !license.isActive,
      async mount(context, { element }) {
        const [coreStart] = await coreSetup.getStartServices();
        const license = await plugins.licensing.license$.pipe(first()).toPromise();
        const { renderApp } = await import('./render_app');
        return renderApp(license, element, coreStart);
      },
    });
  }

But that involves making things asynchronous again. Also (unless I'm missing something) the dev tools plugin doesn't have a way to update the status of the tabs / links after the fact, like with coreStart.chrome.navLinks.update for non-dev tools Kibana plugins (interestingly I had added this in start for Grok Debugger and it seemed to affect things at some point in my testing...must have been imagining things 😬).

Do you have opinions on how the tab should be properly disabled if keeping things synchronous?

[cjcenizal]

Just left a couple small suggestions.

[jloleysens]

@Kerry350 I hear what you are saying regarding not being able to disable the tab. This is a limitation in how Dev Tools are being registered. The way SearchProfiler deals with it, historically, is to show the component but in a disabled state (as you have mimicked here). Dev Tools apps will need to be updated to become properly responsive to license changes in the future.

@flash1293 Would you mind weighing in here on whether registering Dev Tools will be disable-able in future (sorry if you're the wrong person to ping).

[Kerry350]

@jloleysens @cjcenizal Thanks again for the reviews. This should be good to go now once the build is green.

This is a limitation in how Dev Tools are being registered. The way SearchProfiler deals with it, historically, is to show the component but in a disabled state (as you have mimicked here).

Cool, thanks for the clarification that there isn't anything that can be done here (yet) 👍

The only feedback I didn't action was the changing of the route registration handling. As this was stylistic vs something broken and there's only one route currently I've just left it for now.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: c1e8411

History

• 💔 Build #37167 failed 4825d85
• 💔 Build #37150 failed ed846a1
• 💚 Build #36759 succeeded c1e3af5
• 💚 Build #36563 succeeded 19a1dbe
• 💚 Build #36508 succeeded 85ec75e

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[cjcenizal]

🍾 Woohooo!! Thanks @Kerry350 !