Migrate status page app to core #72017
Conversation
… status_page plugin
|
@spalger @mistic so, it seems that #72096 is not the only issue regarding removing the last legacy plugin having client-side code. Totally removing the legacy Looking at one of the failures, kibana/test/functional/apps/bundles/index.js Lines 66 to 72 in d1a6fa2 is performing an http call to Even stranger is that these failures are not present when running the FTR suite on local using the FTR test server, and are only present on the CI, so it must be present only on the production build. Would one of you mind taking a look? I guess the theme css files generation should now be moved to the KP optimizer instead? Maybe there more than that. |
pgayvallet
added
release_note:skip
|
Yeah, looked at the linked failures and one of them is a flaky test #71056 and the other just seems like a redundant test covered by the unit tests, and we don't have a good way to functionally test it so I think it's fine to remove the |
|
After a slack discussion with @spalger, the removal of the status_page plugin, as the last legacy plugin having UI code, is tightly coupled with the removal of the legacy optimizer, which is why we decided to keep 7bdc3c2 in the PR to keep things isolated. Once the legacy optimizer is definitely removed, we should just open a new PR to revert the changes of this specific commit. |
| mount(params: AppMountParameters) { | ||
| return renderStatusApp(params, { http, notifications }); |
There was a problem hiding this comment.
We choose to not use async import for the error app, so I think it makes sense to do the same for the status page.
| return null; | ||
| } | ||
| return ( | ||
| <EuiBasicTable<FormattedStatus> |
There was a problem hiding this comment.
TIL you can specify generics of react components in tsx.
| async (context, request, response) => { | ||
| if (anonymousStatusPage) { | ||
| return response.renderAnonymousCoreApp(); | ||
| } else { | ||
| return response.renderCoreApp(); | ||
| } |
There was a problem hiding this comment.
Note: the legacy status_page was returned with a status of 503 if the server status wasn't green, but
- we can't reproduce this behavior until the server-side API is moved to KP
- we don't have a way to return custom http status code using the
httpResourcesAPI, and even less error codes.
There was a problem hiding this comment.
we don't have a way to return custom http status code using the httpResources API, and even less error codes.
Should we create an issue to add this functionality and refactor the status page later?
| @@ -17,23 +17,36 @@ | |||
| * under the License. | |||
| */ | |||
|
|
|||
| import { Plugin, CoreSetup } from 'kibana/public'; | |||
| import type { OpsMetrics } from '../server/metrics'; | |||
There was a problem hiding this comment.
I'm importing from server into types here to avoid moving all the types from core/server/metrics to core/types. As it's an import type statement, I guess this is alright?
| return new kibana.Plugin({ | ||
| uiExports: { | ||
| app: { | ||
| title: 'Server Status', | ||
| title: 'Old Server Status', | ||
| main: 'plugins/status_page/status_page', | ||
| hidden: true, |
There was a problem hiding this comment.
As explained in the comments, the status_page plugin is the last legacy plugin with UI. removing it totally causes issues with the legacy optimizer no longer generating some files. The legacy plugin will have to be totally removed after we remove the legacy optimizer.
| try { | ||
| if (kbnServer.status.isGreen()) { | ||
| return await h.renderApp(app); | ||
| } else { | ||
| return await h.renderStatusPage(); | ||
| } | ||
| return await h.renderApp(app); |
There was a problem hiding this comment.
This block was mostly broken since we moved the ES service and status check to KP, as we are awaiting for ES to be ready before starting up the plugins. Only situation it would properly work is if the server status would change from green to red after initialization.
As this part (auto 503 handling) is another step of #41983, and as we can't easily render the status page from legacy h helper anyway, I think this change is alright for now.
There was a problem hiding this comment.
This block was mostly broken since we moved the ES service and status check to KP, as we are awaiting for ES to be ready before starting up the plugins.
we could serve status page with NotReadyServer while waiting for ES service to be ready
kibana/src/core/server/http/http_service.ts
Lines 187 to 201 in 9e8448f
That's optional suggestion and not a blocker for the current PR
There was a problem hiding this comment.
Yea, we do want to do that, however this is far from trivial from what I saw:
We are going to have a cyclic dependency issue doing so, as this notReadyServer is within the http service, and we gonna need to use the httpResources, or at least the rendering service to send the correct response. We could expose something like an internal registerNotReadyRoute or something to work-around this issue.
Second point, this will not work until we migrate the server-side status endpoints to core, as atm the endpoints needed by the page are in a legacy plugin, so the routes are not mounted until the end of core's start phase. Even when migrated, we would need to register these routes to the notReadyServer too, for them to be available during the setup phase.
Follow-up of previous point, the notReadyServer is currently a 'raw' HAPI server, so not directly compatible with our route registrations API. We would need to adapt the server to be compatible, or adapt the routes.
I added a point in #41983 and linked it to this discussion.
|
Pinging @elastic/kibana-platform (Team:Platform) |
pgayvallet
added
the
Feature:Legacy Removal
💚 Build Succeeded
Expand to view the summary
Build stats
|
| @@ -58,6 +62,7 @@ export class StatusService implements CoreService<InternalStatusServiceSetup> { | |||
| return { | |||
| core$, | |||
| overall$, | |||
| isStatusPageAnonymous: () => statusConfig.allowAnonymous, | |||
There was a problem hiding this comment.
That's a bummer that we cannot use exposeToBrowser for the platform code. Should I create an issue?
There was a problem hiding this comment.
Yea, I don't like what I had to do to mimic the same behavior...
| await retry.tryForTime(6000, async () => { | ||
| const text = await testSubjects.getVisibleText('statusBreakdown'); | ||
| expect(text.indexOf('plugin:kibana')).to.be.above(-1); | ||
| }); | ||
| }); | ||
|
|
||
| it('should show the build hash and number', async () => { |
There was a problem hiding this comment.
should we add tests for an anonymous user?
There was a problem hiding this comment.
We would need to add an x-pack suite for that, as the current suite is running on OSS. Should I do so?
There was a problem hiding this comment.
I would add it. Config is a public API.
There was a problem hiding this comment.
nm, there is already one:
kibana/x-pack/test/functional/apps/status_page/status_page.ts
Lines 20 to 24 in 0f9efa8
There was a problem hiding this comment.
I can add a similar test when status.allowAnonymous is false, but that would require to create a specific test suite to start the server with the correct option. Either that, or I can just add a unit test for src/core/server/core_app/core_app.ts to ensure the route is declared with correct options depending on the configuration value. wdyt?
There was a problem hiding this comment.
Either that, or I can just add a unit test for src/core/server/core_app/core_app.ts to ensure the route is declared with correct options depending on the configuration value. wdyt?
👍
| async (context, request, response) => { | ||
| if (anonymousStatusPage) { | ||
| return response.renderAnonymousCoreApp(); | ||
| } else { | ||
| return response.renderCoreApp(); | ||
| } |
There was a problem hiding this comment.
we don't have a way to return custom http status code using the httpResources API, and even less error codes.
Should we create an issue to add this functionality and refactor the status page later?
|
|
||
| try { | ||
| response = await http.get('/api/status', { | ||
| credentials: 'same-origin', |
There was a problem hiding this comment.
Is it required? It seems to be the default value. https://developer.mozilla.org/en-US/docs/Web/API/Request/credentials
| name: i18n.translate('core.statusPage.metricsTiles.columns.heapTotalHeader', { | ||
| defaultMessage: 'Heap total', | ||
| }), | ||
| value: get(data.metrics, 'process.memory.heap.size_limit'), |
There was a problem hiding this comment.
Can we remove get as we have proper typings?
| get(data.metrics, 'os.load.5m'), | ||
| get(data.metrics, 'os.load.15m'), | ||
| ], | ||
| type: 'float', |
There was a problem hiding this comment.
formatNumber doesn't have float branch
| path: '/status', | ||
| validate: false, | ||
| options: { | ||
| authRequired: !anonymousStatusPage, |
There was a problem hiding this comment.
Isn't the same as 'optional'?
There was a problem hiding this comment.
I don't think it is. Depending on the config value of status.allowAnonymous, the app is, or not, accessible to non-logged users.
| return { | ||
| isStatusPageAnonymous: server.config().get('status.allowAnonymous'), | ||
| }; | ||
| url: '/old-status', |
There was a problem hiding this comment.
Is it possible to land on it?
There was a problem hiding this comment.
Yea, by manually typing the path in the browser, it unfortunately is. Not sure how to not break the legacy optimizer without exposing the app, and the url is mandatory...
Maybe I should change the path to something a little scarier, like __deprecated__/status?
There was a problem hiding this comment.
I'm fine to keep it. We are about to remove it soon.
| }); | ||
|
|
||
| if (injectedMetadata.getAnonymousStatusPage()) { | ||
| http.anonymousPaths.register('/status'); |
There was a problem hiding this comment.
You cannot navigate to the other Kibana pages from other anonymous pages without reloading. It creates a bug when status.allowAnonymous: true and you logged-in user lands on the /status page and navigates to other pages. result: SecurityNavControl is not rendered.
As a workaround, we could make status page chromeless to enforce page reloading.
Is it possible to land on the /status page from a link in Kibana UI?
There was a problem hiding this comment.
You cannot navigate to the other Kibana pages from other anonymous pages without reloading. It creates a bug when status.allowAnonymous: true and you logged-in user lands on the /status page and navigates to other pages. result: SecurityNavControl is not rendered.
Hum. This indeed is an issue. We really need to have a way to know about anonymous vs logged-in apps from core, to be able to automatically performs that kind of reload when using navigateToApp between anon/logged or logged/anon apps. Do you know if we have an issue for that?
As a workaround, we could make status page chromeless to enforce page reloading.
chromeless apps do not really force page reloading, but I guess hiding the chrome navigation will remove any way for the user to navigate away from the status page using the UI. do you think it would be sufficient?
Is it possible to land on the /status page from a link in Kibana UI?
Not that I'm aware of. The app is hidden and there is no link to it anywhere in chrome.
There was a problem hiding this comment.
but I guess hiding the chrome navigation will remove any way for the user to navigate away from the status page using the UI.
that was my reasoning as well. I think it's okay since the user navigates to the /status page manually.
Hum. This indeed is an issue. We really need to have a way to know about anonymous vs logged-in apps from core, to be able to automatically performs that kind of reload when using navigateToApp between anon/logged or logged/anon apps. Do you know if we have an issue for that?
I'm not aware of any.
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / X-Pack Accessibility Tests.x-pack/test/accessibility/apps/uptime·ts.uptime settings pageStandard OutStack TraceBuild metrics@kbn/optimizer bundle module count
page load bundle size
History
To update your PR or re-run it, just comment with: |
* move http.anonymousPaths.register('/status'); logic into core, remove status_page plugin
* move status_page to core & migrate lib
* migrate the status_app components to TS/KP APIs
* update rendering snapshots
* use import type syntax
* moves `/status` server-side route to core
* fix route registration
* update generated file
* change statusPage i18n prefix
* (temporary) try to restore legacy plugin to check behavior
* add some FTR tests
* do not import whole lodash
* update snapshots
* review comments
* improve / clean component unit tests
* change url for legacy status app
* set status app as chromeless
* fix FTR test due to chromeless app
* fix typings
* add unit test for CoreApp /status registration
# Conflicts:
# x-pack/plugins/translations/translations/ja-JP.json
# x-pack/plugins/translations/translations/zh-CN.json
* master: (35 commits) Migrated karma tests to jest (elastic#72649) Migrate status page app to core (elastic#72017) Failing test: Jest Tests.src/plugins/vis_type_vega/public (elastic#71834) Fix Firefox TSVB flaky test with switch index patterns (elastic#72882) [ML] Fixing link to index management from file data visualizer (elastic#72863) test: 💍 add test for sub-expression variables (elastic#71644) fix bug (elastic#72809) [keystore] use get_keystore in server cli (elastic#72954) Show step number instead of incomplete step. (elastic#72866) Fix bug where user can't add an exception when "close alert" is checked (elastic#72919) [Monitoring] Fix issues displaying alerts (elastic#72891) [Ingest Manager] Add more Fleet concurrency tests elastic#71744 (elastic#72338) [Security Solution][Exceptions] - Update UI exceptions builder nested logic (elastic#72490) disable renovate masterIssue [ML] API integration tests for UPDATE data frame analytics endpoint (elastic#72710) [Uptime] Fix accessibility issue in Uptime app nav links (elastic#72926) [Maps] fix removing global filter from layer can cause app to start thrashing (elastic#72763) [Maps] fix blended layer aggregation error when using composite aggregation (elastic#72759) fix unexpected arguments to unload command Limits the upload size of lists to 9 meg size (elastic#72898) ...
* Migrate status page app to core (#72017) * move http.anonymousPaths.register('/status'); logic into core, remove status_page plugin * move status_page to core & migrate lib * migrate the status_app components to TS/KP APIs * update rendering snapshots * use import type syntax * moves `/status` server-side route to core * fix route registration * update generated file * change statusPage i18n prefix * (temporary) try to restore legacy plugin to check behavior * add some FTR tests * do not import whole lodash * update snapshots * review comments * improve / clean component unit tests * change url for legacy status app * set status app as chromeless * fix FTR test due to chromeless app * fix typings * add unit test for CoreApp /status registration # Conflicts: # x-pack/plugins/translations/translations/ja-JP.json # x-pack/plugins/translations/translations/zh-CN.json * fix i18n merge
Summary
Fix #67979
status_pageplugin to KPChecklist