Non-global injected initial data

[kobelb]

The not-a-platform has the concept of "injected vars" which allows us to load various data in the initial http response embedded within the HTML. This allows us to immediately be able to render other HTML elements based on this data. Without this facility, and once we no longer allow setup/start lifecycle events to be "async" in the Kibana Platform per this RFC, we will have to perform a XHR to get this data and then render the dependent HTML elements. This can cause the page to "flicker" as we wait for the request to complete, as can be seen in #52386 and #40856.

It'd be nice to have non-global injected initial data so that we no longer have this page flicker. If I understand correctly, our main complaint with the not-a-platform's implementation of "injected vars" was with it being scoped globally, and crossing plugin boundaries.

[elasticmachine]

Pinging @elastic/kibana-platform (Team:Platform)

[pgayvallet]

When #52161 lands, the pages rendering will be done in NP. That will allow us way more control on the 'injected metadatas'.

We could provide an API like

coreSetup.rendering.setPluginDataProvider(async (request) => {
    return await someData(request);
})

That could be consumed, on a per-plugin basis, on the frontend with

// will only access data registered by my plugin
coreSetup.someService.getInjectedData<MyPluginInjectedData>(); 

PROS:

• avoid potential 'flickering'
• avoid 'useless' http requests after page load, resulting in potential speed improvement

CONS:

• same issue than with async setup/start: as the data from setPluginInjector will probably requires the provider API to be async, and as we need to wait for every plugin for their injected data before rendering, any plugin can cause the whole app to wait before load, or in worse case the page to never load. Also as plugins will probably need the data they registered to behave properly, a safety timeout is not an option.

I definitely see the usages and gains of the proposal, but my feeling is that this goes against what #45796 tries to achieve.

[kobelb]

I definitely see the usages and gains of the proposal, but my feeling is that this goes against what #45796 tries to achieve.

It's possible that I misunderstand the intent of #45796, but I see there being a difference between blocking Kibana from starting up vs. blocking a HTTP response.

[pgayvallet]

blocking Kibana from starting up

#45796 does not only impact server-side, it is also about making the client-side plugins lifecycle methods synchronous to avoid that a plugin could cause the whole app to freeze/hang/never start on the client-side. Awaiting some per-plugin data during the rendering process of the html page seems like a similar problematic.

[kobelb]

#45796 does not only impact server-side, it is also about making the client-side plugins lifecycle methods synchronous to avoid that a plugin could cause the whole app to freeze/hang/never start on the client-side. Awaiting some per-plugin data during the rendering process of the html page seems like a similar problematic.

Good point.

[joshdover]

Awaiting some per-plugin data during the rendering process of the html page seems like a similar problematic.

This is a good observation. If we added an async way for plugins to provide rendering data, we'd be trading off a slower first byte (and thus no visual feedback that Kibana is doing anything) for no UI flicker.

It seems like the UI flickering issue could be at least mitigated by thoughtful UI design using placeholders that fade in the real UI once the requisite data is loaded.

[kobelb]

This is a good observation. If we added an async way for plugins to provide rendering data, we'd be trading off a slower first byte (and thus no visual feedback that Kibana is doing anything) for no UI flicker.

It seems like the UI flickering issue could be at least mitigated by thoughtful UI design using placeholders that fade in the real UI once the requisite data is loaded.

What about capabilities? Almost nothing will be able to render until those are available.

[joshdover]

@kobelb Those are fetched by Core before plugin start methods are executed or any UI is mounted.

[kobelb]

@kobelb Those are fetched by Core before plugin start methods are executed or any UI is mounted.

🤦‍♂ I forgot the KP's "core" doesn't have to abide by all of the same rules as the plugins, the setup and start methods within the ApplicationService threw me off.

Perhaps the security and spaces plugins really are the exception here...

[rudolf]

Edit: I had a stale version of the page open, so didn't see the conversation already evolved.

@pgayvallet is right in that the intent is to limit the impact of async bugs in lifecycles but also context handlers which are called while Kibana is already started.

So if a plugin rendered it's own html page on it's own endpoint it would be fine, but anything else introduces a weakness where one plugin's "injected vars" bug prevents the entire Kibana from showing.

[joshdover]

After discussion, we've decided to punt on adding any blocking behavior here. If this becomes a significant usability problem in the future, we should reopen this issue and reconsider.

[joshdover]

Notes from duplicate issue I opened:

Summary of problems with the legacy injected vars:

• The server blocks on rendering the bootstrap HTML & JS on executing this functions (which may be async). This delays the loading of the frontend code for data that may not be necessary to start using the UI.
• Some injected vars are shared across plugins. Any plugin can access any other plugin's injected vars creating implicit dependencies between plugins which can easily cause breaks if a plugin is disabled.

In the Kibana Platform, we introduced the exposeToBrowser config option to expose config values to the frontend. This covers most cases that injected vars was being used for, however there are some use cases that cannot be solved by this:

• The short URL plugin uses an injected var to provide the frontend with the resolved URL fetched from a Saved Object. Without injected vars, the service requires loading this data via an async request to a separate endpoint after the frontend has loaded and started. [Dasbhoard][Short URL] Parse embeded in the short URL #54403 (comment)
• Security/spaces need to be able to detect if secure cookies is enabled based on the fact whether or not TLS is enabled, and spaces is enabled.

Drawbacks with requiring these plugins to get this data via a fetch to a dedicated endpoint:

• The request doesn't get started until after all JS bundles have loaded and core has executed the plugin.
• If enough plugins need this, we may have many requests that happen as soon as plugins are setup/started. These requests may get queued due to open connection limitations imposed by browsers.

Possible Solution

We could add an API to the RenderingService that allows plugins to register functions that provide data to the frontend, similar to the legacy platform, with some distinct differences:

• Plugins should only be able to read data that they provided themselves, not from other plugins. This should eliminate the issue where implicit dependencies between plugins can be created.
• These functions can be executed separate from the main render endpoint to avoid delaying the browser from starting to load and parse JS code. Instead, we can make a request to an endpoint in the bootstrap.js script so that this data can be loaded in parallel with JS and CSS. This endpoint would be provided by the RenderingService and would include return the data returned by all registered data providers.

class MyPluginServer {
  setup(core) {
    // Register a function to be executed by the RenderingService
    core.rendering.registerRenderData(async () => ({
      helloWorld: someAsyncFunc()
    }));
  }
}

class MyPluginPublic {
  constructor(initContext) {
    // Only data provided by this plugin's server-side code would be available
    this.helloWorld = initContext.renderData.helloWorld;
  }
}

Discussion

• Is this a premature optimization? Should we continue without the current pattern?

[pgayvallet]

These functions can be executed separate from the main render endpoint to avoid delaying the browser from starting to load and parse JS code. Instead, we can make a request to an endpoint in the bootstrap.js script so that this data can be loaded in parallel with JS and CSS. This endpoint would be provided by the RenderingService and would include return the data returned by all registered data providers.

Correct me if I'm wrong:

1. this doesn't really fix the initial reason we decided to close this initially: plugins will still be able to make the client-side app hang/never-init with an unresolved data promise.

2. plugins will still need these data to start, so we won't be able to actually finish client-side initialization until this additional call is finished. domready will trigger sooner, but we will stick with the default html content until the second call is finished, as the app startup relies on it.

If we decide to go with this (which I'm not against) however, I'm not sure the synchronous lifecycles changes really makes sense anymore as the initial issue it tries to address will be present with this custom data feature.

[kobelb]

These functions can be executed separate from the main render endpoint to avoid delaying the browser from starting to load and parse JS code. Instead, we can make a request to an endpoint in the bootstrap.js script so that this data can be loaded in parallel with JS and CSS. This endpoint would be provided by the RenderingService and would include return the data returned by all registered data providers.

The data that the Security and Spaces plugin would like to have provided in the injected initial data is already retrieved in the auth, or post-auth lifecycles and could easily be cached and so that we don't end up executing this request twice.

this doesn't really fix the initial reason we decided to close this initially: plugins will still be able to make the client-side app hang/never-init with an unresolved data promise.

Is this a common problem in the LP? I've struggled to form an opinion on how much this concern should be driving the design.

[mshustov]

this doesn't really fix the initial reason we decided to close this initially: plugins will still be able to make the client-side app hang/never-init with an unresolved data promise.

Do we really need to support async hooks? can Security, Spaces & UrlShortener can go with sync getter?

kibana/x-pack/plugins/features/server/routes/index.ts

Line 22 in 02344aa

{ path: '/api/features', options: { tags: ['access:features'] }, validate: false },

kibana/x-pack/legacy/plugins/security/index.js

Line 150 in 02344aa

const { loginAssistanceMessage } = securityPlugin.__legacyCompat.config;

kibana/src/legacy/server/url_shortening/routes/goto.js

Line 33 in 02344aa

redirectUrl: url,

[flash1293]

About the short url use case - we need to fetch a saved object to fill in the injected data. But we only need to do this when rendering the app using a custom route anyway (using the newly introduced rendering api). So for short url it would be enough to be able to inject the data only when calling the rendering api directly (not as a hook for all routes/apps)

Basically as a separate parameter in this function call:
https://github.com/elastic/kibana/pull/55021/files#diff-31cac4a8cc01f2e664340116f9a89065R58

[pgayvallet]

Is this a common problem in the LP? I've struggled to form an opinion on how much this concern should be driving the design.

I don't have a strong opinion on this. It's one of core responsibility to be resilient to plugin code. However I agree that the question about to what extent this should be adamant should be asked.

The preloaded data is a very common practice with a lot of upsides regarding performances (reducing the number of requests is almost always better than the first-byte timing). This just goes against the lifecycle unblocked RFC...

Long story short, I'm neither yea nor nay

[flash1293]

I like having a core design that allows to scale the system without making it brittle and unreliable.

My suggestion to cover the existing use cases without hurting the lifecycles:

Sync hooks that can be run on every request:

class MyPluginServer {
  setup(core) {
    // Register a function to be executed by the RenderingService
    core.rendering.registerRenderData(() => ({
      helloWorld: someFunc()
    }));
  }
}

Plus passing an object to the explicit render call:

const injectedData: await myAsyncFunc();
return response.ok({
        headers: {
          'content-security-policy': http.csp.header,
        },
        body: await context.core.rendering.render({ injectedData }),
      });

The first case can't block the request because it's synchronous, the second one will only slow down the route controlled by the plugin itself, in which case it's okay to block it.

[pgayvallet]

The first case can't block the request because it's synchronous

Sync hooks does not have the same issue and there is no reasons we shouldn't implement it if this is a need. Only question I would have regarding them is: Is there an actual realistic usage of them? It seems to me that the async nature was required to fetch/load them on the server-side?

the second one will only slow down the route controlled by the plugin itself

True. However I'm not sure to see the potential gain of this: When we'll be fully migrated, Kibana will be a true SPA, and there will no longer be page-reload between apps. These data would only be loaded for your custom route if that's the first page the user accesses or when he would be performing a page refresh.

[flash1293]

Sync hooks does not have the same issue and there is no reasons we shouldn't implement it if this is a need. Only question I would have regarding them is: Is there an actual realistic usage of them? It seems to me that the async nature was required to fetch/load them on the server-side?

@kobelb I think you have to answer that question :) Would a sync hook cover your use case?

True. However I'm not sure to see the potential gain of this: When we'll be fully migrated, Kibana will be a true SPA, and there will no longer be page-reload between apps. These data would only be loaded for your custom route if that's the first page the user accesses or when he would be performing a page refresh.

Exactly, that's what's necessary for the short url service. It's basically a workaround to resolve really long URLs in browsers that do not support them. The URL params are never part of an actual URL, but are passed as part of the initial HTML payload and then put into the session storage of the browser. Of course this can also be done with a fetch call, but that would slow down the rendering of the actual app the user wants to navigate to because it adds a client/server roundtrip:

1. Load short url redirect page
2. Do fetch call to retrieve super long URL (this is the additional step removed when it is part of the initial HTML response)
3. Put the params in in the local storage
4. Load the actual app the short url points to

It's not a big deal, I just raised it because it's a regression from current state. I'm also fine with avoiding this additional complexity in the core if it's just for this very case.

I can think about potential future use cases that would profit from being able to this though, e.g. inlining data into an embedded dashboard that's shown in an iframe on another web page. In general all cases where we can deduct from the URL of the initial request that the page will definitely request a defined set of additional data and directly putting it into the initial request itself. Of course the tradeoff here is time to first byte vs time to load, but it would be nice to let the plugin have control over this.

[pgayvallet]

Exactly, that's what's necessary for the short url service [...]

Thanks for the detailed clarification

In general all cases where we can deduct from the URL of the initial request

What bothers me here is that it may not be crystal clear for developers that these additional data shall only be used for that specific use case. I'm afraid some tries to use the feature thinking that it's just a data-preload that will work in any case, even if accessing their application after another initial page load. If we want to provide that, we will need to be very explicit on the API naming and documentation

[flash1293]

I'm torn whether the introduced complexity (and potential confusion and misuse of consumers) is worth it. Fine with both ways here. Maybe someone can think of a better way to expose the functionality to make the intended purpose easier to understand.

[kobelb]

@kobelb I think you have to answer that question :) Would a sync hook cover your use case?

It would for the moment, but it's coincidental and in the future it might not.