-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Infra UI] Add Features section to Infra settings #168712
[Infra UI] Add Features section to Infra settings #168712
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
a3473ef
to
166e7af
Compare
Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Management changes LGTM (packages/serverless/settings/observability_project/index.ts
). Please see comment for awareness. Did not test locally.
@@ -27,4 +27,5 @@ export const OBSERVABILITY_PROJECT_SETTINGS = [ | |||
settings.OBSERVABILITY_APM_TRACE_EXPLORER_TAB_ID, | |||
settings.OBSERVABILITY_ENABLE_AWS_LAMBDA_METRICS_ID, | |||
settings.OBSERVABILITY_APM_ENABLE_CRITICAL_PATH_ID, | |||
settings.OBSERVABILITY_ENABLE_INFRASTRUCTURE_HOSTS_VIEW_ID, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI - this setting will also appear on the Advanced Settings page in serverless. Just pointing out as it wasn't mentioned in the PR description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @alisonelizabeth I reached out to understand further but I was under the impression that there wasn't going to be an observability advanced settings area or at least one where Infra could have a setting. If there is and we can keep our setting there, we don't necessarily need to add it to our own UI. Since it won't work in our UI without enabling it here, I assume we then have to show it in the Project Advanced Settings and I misinterpreted the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mykolaharmash I think we need to leave this out for now and add it when we "flip the switch" to enable our plugin. Otherwise this setting will show up with no Infra UI if we haven't enabled the plugin for serverless yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @alisonelizabeth I reached out to understand further but I was under the impression that there wasn't going to be an observability advanced settings area or at least one where Infra could have a setting. If there is and we can keep our setting there, we don't necessarily need to add it to our own UI. Since it won't work in our UI without enabling it here, I assume we then have to show it in the Project Advanced Settings and I misinterpreted the docs.
You can go to /app/management/kibana/settings
to view Advanced Settings in serverless for each project type.
It is possible to have project-specific settings. The settings are defined here. You'll see that there is a set of common settings that are used across project types, as well as project-specific settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI - this setting will also appear on the Advanced Settings page in serverless. Just pointing out as it wasn't mentioned in the PR description.
Thank you @alisonelizabeth for pointing this out! Somehow I thought that the allowlist only enables the changes through the API and does not affect the UI 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mykolaharmash I think we need to leave this out for now and add it when we "flip the switch" to enable our plugin. Otherwise this setting will show up with no Infra UI if we haven't enabled the plugin for serverless yet.
Agree 💯
@mykolaharmash In #142477 at the time Hosts was unavailable or hidden in the navigation and turned on if the setting was enabled. We used an observable so it would react to the change without a refresh. I think the same should happen now where when I go to the advanced settings enable/disable it and then click on Hosts the change should be reflected. Would this be difficult to change? I also think our UI setting should update when the one in the Advanced Setting does. I understand it might not work vice versa where the Advanced Setting changes when the one in our UI settings changes and I think that's fine for now. |
Thank you for catching this @neptunian 🙌 I double-checked and it seems like toggling the flag in advanced settings does not have an immediate effect in the current version as well. Have not checked the code but my guess is that it worked before with nav item because the nav component was subscribed to the observable and once we switched to the new Hosts landing screen, that lives within our plugin, the subscription is not initialized while user is on the advanced settings screen, so it requires a page reload. With the Features section in our own settings toggling the flag works as expected and the landing screen updates without a refresh. In case we decide to keep the Feature section, what we could do is force page reload when making changes from Advanced Settings and rely on the observable when changing from Infra settings. Or alternatively, we could always reload the page, in case we want to keep the toggles in sync on the Infra and Advanced Settings screens (afaik there is no other way to reset the uiSettings cache other than page reload). Wdyt? |
Moving back to draft as requirements in #167062 have been updated. |
4012e50
to
f4a521b
Compare
Looks great! |
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mykolaharmash maybe to keep it consistent with what we have on Profiling and APM you should use the bottom button bar when something has changed.
Right, was thinking the same as we use the bottom bar on the Advanced Settings screen and APM settings. The current infra settings already had those custom form buttons and I did not want to bloat this PR with bottom bar refactoring. If you don't mind, I'll create a separate issue to migrate the whole form to use the bottom bar, wdyt? |
Created a ticket to unify the buttons #169210 cc @cauemarcondes |
Thanks for that... 🙏🏻 |
Closes #167062
Summary
uiSettings
into existing form in order to use the current "Discard" and "Apply" buttons logicuiSettings
withsettings.client
asuiSettings
seems to be [deprecated]([KibanaReact] Use settings service in useUiSetting hook #154710. Observables exposed byuiSettings
do not seem to work anymore.CleanShot.2023-10-17.at.16.48.24.mp4
How to test