Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kibana NP migration: homeApi -> registerTutorials #50224

Closed
TinaHeiligers opened this issue Nov 11, 2019 · 17 comments · Fixed by #54910
Closed

Kibana NP migration: homeApi -> registerTutorials #50224

TinaHeiligers opened this issue Nov 11, 2019 · 17 comments · Fixed by #54910
Assignees
Labels
Feature:NP Migration Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@TinaHeiligers
Copy link
Contributor

TinaHeiligers commented Nov 11, 2019

Move tutorials api in src/legacy/core_plugins/kibana/server/routes/api/home to the new platform.
The most important part is the registry where plugins can register new tutorials(src/legacy/ui/tutorials_mixin.js). This is where the main effort lies.

The following methods need to be returned from the new TutorialsPlugin setup method:
getTutorials, registerTutorial and addScopedTutorialContextFactory.
At the moment, the methods accept the main kbn server and request objects. Once the server and request are not explicitly called, we'll be able to migrate the decoration methods to a new Tutorials plugin.

@TinaHeiligers TinaHeiligers self-assigned this Nov 11, 2019
@TinaHeiligers
Copy link
Contributor Author

@joshdover Please feel free to update/edit and extrapolate on the blockers within the spaces api x-pack/plugins/spaces/server/lib/spaces_tutorial_context_factory.ts and the refactoring that needs to happen in apm.

@joshdover
Copy link
Contributor

After digging into this a bit with Tina, we found a couple blockers to moving this:

  • Spaces relies on access to the raw Hapi object to get the space ID in x-pack/plugins/spaces/server/lib/spaces_tutorial_context_factory.ts. This will need to be refactored before tutorials can be moved. @legrego when will the getSpaceId be available in the New Platform? I'd expect it to be available on the request context?
  • The APM tutorial (src/legacy/core_plugins/kibana/server/tutorials/apm/index.js) is relying on the legacy global config object to get access to the apm_oss.indexPattern and xpack.apm.ui.enabled keys. This will need to be refactored to get these configs directly from the apm_oss and apm plugins' setup contract. @ogupte is this something you could help with?
  • Similarly, the EMS tutorial (src/legacy/core_plugins/kibana/server/tutorials/ems/index.js) is relying on the global config object to access server.basePath and map.emsLandingPageUrl. The basePath can be accessed via core.http.basePath.get but the map config will need to be provided by the Maps plugin. @aaronjcaldwell could you help here?

One thing that could make this simpler is if the tutorials were registered by the individual plugins rather than all registered by the Kibana plugin. That would make the access to different pieces of data much simpler. I'm not sure if there's any reason for them to all live in Kibana other than historical ones. @TinaHeiligers thoughts on moving each tutorial to their respective plugins?

@TinaHeiligers
Copy link
Contributor Author

@joshdover Moving each tutorial to its respective plugin makes sense. That way, changes can be managed individually and won't be coupled to the kibana plugin. @flash1293 your thoughts?

@lukeelmers
Copy link
Member

+1 to the idea of moving tutorials to their respective plugins. This feels in line with some of our other design decisions we've made during NP migration.

Also FWIW -- earlier in the migration process we had made the assumption that tutorials would live inside of a home plugin, which would also handle the UI for everything that's in the home page. This is why we didn't have a tutorials plugin listed in the original meta issue. If we think it warrants its own domain that's totally fine -- I don't know enough about the roadmap for this feature to have an informed opinion -- but just wanted to point this out.

@joshdover
Copy link
Contributor

joshdover commented Nov 12, 2019 via email

@flash1293
Copy link
Contributor

Thanks for digging into this @TinaHeiligers

Having a home plugin that provides the feature catalogue and tutorials sounds good to me - we discussed this in our sync but were not completely sure how granular those should become.
Then it looks like sample data could also go into this plugin, what do you think @joshdover ?

Personally I don't have a problem with a NP and LP version of a plugin during the transition phase - especially in the kibana app LP we need to leave all apps behind till the very last moment, so we would have that problem in a lot of places.

@joshdover
Copy link
Contributor

SGTM 👍

@kindsun
Copy link
Contributor

kindsun commented Nov 12, 2019

+1 Moving tutorials to their respective plugins sounds reasonable to me. Happy to help!

@joshdover
Copy link
Contributor

From a implementation standpoint, this may be easier if instead of migrating all of the existing tutorial code at once, we break this up into separate changes:

  • Build the new implementation inside the home plugin (combined with the feature_catalogue).
  • Migrate all existing tutorials to their respective plugins
  • Delete the old tutorial code once all tutorials have been registered with the new plugin

@TinaHeiligers
Copy link
Contributor Author

@joshdover +1 to breaking the work up into separate changes.
When should the sample data be moved to the home plugin? In this step?

Build the new implementation inside the home plugin (combined with the feature_catalogue).

@joshdover
Copy link
Contributor

I think it can be done separately or at the same time, not sure it matters too much since they are not directly related.

@TinaHeiligers
Copy link
Contributor Author

TinaHeiligers commented Nov 12, 2019

At the moment, some tutorial registration methods need the server and context as arguments while others don't use the server object at all.

We can use the new plugin to register tutorial providers that don't use the server but have to rely on the legacy code for registering those that do.

Until the tutorial registrations are moved to their respective application plugins, registration will continue to happen within the registerTutorials(server) method in the legacy code.

Two routes

As an intermediate step, we have decided to implement two routes, one for the legacy registration:

'/api/kibana/home/tutorials'

and

'/api/kibana/home/NP_tutorials' for the new platform registration.

Tutorial registration

Note: The examples that follow will change somewhat when the tutorials are implemented as services within the home plugin. i.e. references to "tutorials" will change to "home"

For the new plugin, tutorial registrations that don't need access to the server will follow:

e.g. server.newPlatform.setup.plugins.tutorials.registerTutorial(logstashLogsSpecProvider);

whereas those that do still need access to the server will be registered directly off of the server:

e.g. server.registerTutorial(nginxLogsSpecProvider);

There are two remaining tutorial registrations using the legacy registration methods:

  1. the apmSpecProvider
    and
  2. emsBoundariesSpecProvider

How we handle the client side

One the client side, we merge the response from both in the loadTutorials function as follows:

  try {
    const responseLegacyPlatform = await fetch(baseUrl, {
      method: 'get',
      credentials: 'include',
      headers: headers,
    });
    const responseNewPlatform = await fetch(baseUrlNP, {
      method: 'get',
      credentials: 'include',
      headers: headers,
    });
    if (responseLegacyPlatform.status >= 300) {
      throw new Error(i18n.translate('kbn.home.loadTutorials.requestFailedErrorMessage', {
        defaultMessage: 'Request failed with status code: {status}', values: { status: responseLegacyPlatform.status } }
      ));
    }
    if (responseNewPlatform.status >= 300) {
      throw new Error(i18n.translate('kbn.home.loadTutorials.requestFailedErrorMessage', {
        defaultMessage: 'Request failed with status code: {status}', values: { status: responseNewPlatform.status } }
      ));
    }

    tutorialsLegacyPlatform = await responseLegacyPlatform.json();
    tutorialsNewPlatform = await responseNewPlatform.json();
    tutorials = tutorialsLegacyPlatform.concat(tutorialsNewPlatform);
    tutorialsLoaded = true;
  } catch(err) {
    getServices().toastNotifications.addDanger({
      title: i18n.translate('kbn.home.loadTutorials.unableToLoadErrorMessage', {
        defaultMessage: 'Unable to load tutorials' }
      ),
      text: err.message,
    });
  }
}```

Note: the nomenclature isn't cast in stone and can change.

> +1 Moving tutorials to their respective plugins sounds reasonable to me. Happy to help!

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@dgieselaar
Copy link
Member

dgieselaar commented Nov 20, 2019

APM will expose the indices from its contract in the NP plugin when #49620 is implemented, but that is currently blocked by #49455, so might take a few days.

@TinaHeiligers
Copy link
Contributor Author

APM will expose the indices from its contract in the NP plugin when #49620 is implemented, but that is currently blocked by #49455, so might take a few days.

@dgieselaar A few days is fine, thanks for following up.

@dgieselaar
Copy link
Member

@TinaHeiligers the indices are now exposed in our NP plugin (actually for a while now, I'm a bit late). Do you need anything from the APM side beyond this?

@flash1293 flash1293 assigned flash1293 and unassigned TinaHeiligers Jan 13, 2020
@flash1293
Copy link
Contributor

@dgieselaar I picked up this task. I already discussed offline with @ogupte to move the APM tutorial completely into the apm folder. I'm going to prepare a PR for this shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:NP Migration Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants