-
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
[Logs UI] Support inline Log Views in the UI #152933
[Logs UI] Support inline Log Views in the UI #152933
Conversation
…ne-log-views-client
Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI) |
@elasticmachine merge upstream |
x-pack/plugins/infra/public/observability_logs/log_view_state/src/defaults.ts
Show resolved
Hide resolved
x-pack/plugins/infra/public/pages/logs/settings/inline_log_view_callout.tsx
Outdated
Show resolved
Hide resolved
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.
It's a great job Kerry, congrats! 👏
I found the visual implementation to work well and the playground you added is awesome for testing purposes!
One thing I notices and I'm not sure if it is the expected behaviour, is once you apply directly on the URL the logView=(id:playground-log-view,type:log-view-inline)
parameter, it fails and notify of a validation error because the attributes are missing, here a quick demo:
inline-log-view.mov
The ML warning splash page links to settings to revert to a persisted Log View. It could also be done in place on the page. I went back and forth over whether we should keep the reverting in one place?
I personally believe that for users the shortest path would be the best UX.
If there is nothing more they can do except switch back to a persisted log view to enable the ML page, I'd rather give them the call to action directly on the page, with no need to leave it to reach the settings and click another button.
I also left a couple of comments but are minor things and not-blocking 👍
Thank you for the review 🙏
This is the expected behaviour for now. If we start out strict we can potentially relax things (validation wise) in the future if we need to. It's hard to tell exactly what our needs might be as we're not directly exposing any of this in the UI yet, but staying strict (and requiring a complete type) simplifies things for now.
Sounds good to me, I'll change this 👍 |
…ne-log-views-client
…erry350/kibana into 142840-support-inline-log-views-client
@tonyghiani Pushed all of the changes discussed 👍 |
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.
LGTM, thanks for the quick changes 👏
…ne-log-views-client
…erry350/kibana into 142840-support-inline-log-views-client
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsasync chunk count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @Kerry350 |
## Summary This closes elastic#142840. It is the UI portion of support for inline Log Views. ## Visible changes to the UI ### ML warning ![Screenshot 2023-03-10 at 14 55 34](https://user-images.githubusercontent.com/471693/224348959-8db70d91-dc8b-4f4e-926b-ec05e7481b78.png) ### Alert dropdown warning ![Screenshot 2023-03-10 at 14 55 43](https://user-images.githubusercontent.com/471693/224349026-cdf17767-225a-4ecd-af8a-b90e2a21816f.png) ### Settings page warning ![Screenshot 2023-03-10 at 14 56 02](https://user-images.githubusercontent.com/471693/224349066-bcb63ba8-fee8-4b7a-b41b-7d89e09f002a.png) ## Reviewer hints / notes - The ACs on the issue are quite extensive and should provide a good number of things to test. - Make use of the "playground" page (see below) to make this easier - The `AlertDropdown` has been made lazy as the page load bundle increased by 100kb otherwise. - Our `link-to` functionality is scoped to persisted Log Views only at the moment as historically they've only accepted a path segment, not full query parameters. We can look to extend this in the future once we have concrete linking needs. ## Questions - I have allowed the Log View client to handle both the inline and persisted Log Views. I wonder if the function names become confusing? (e.g. are the RESTful prefixes misleading now?). - The ML warning splash page links to settings to revert to a persisted Log View. It could also be done in place on the page. I went back and forth over whether we should keep the reverting in one place? ## Testing There is now a "state machine playground" available at the following URL: `/app/logs/state-machine-playground`, it is enabled in developer mode only. It's not fancy or pretty it just serves to make testing things easier. There are various buttons, e.g. `Switch to inline Log View`, to facilitate easier testing whilst a Log View switcher is not in the UI itself. You can utilise these buttons, and then head to other pages to ensure things are working correctly, e.g. warning callouts and disabled buttons etc. If you'd like to play with the options used, e.g. for `update`, amend the code within `state_machine_playground.tsx`. It's also useful just to sit on this page, try various things, and verify what happens in the developer tools (does the context update correctly etc). ## Known issues - When saving on the settings page we actually revert to a "Loading data sources" state. This is also the case on `main`. The reason for this is the check within settings looks like so: ```ts if ((isLoading || isUninitialized) && !resolvedLogView) { return <SourceLoadingPage />; } ``` but the `resolvedLogView` state matching looks like so: ```ts const resolvedLogView = useSelector(logViewStateService, (state) => state.matches('checkingStatus') || state.matches('resolvedPersistedLogView') || state.matches('resolvedInlineLogView') ? state.context.resolvedLogView : undefined ); ``` so even though we have resolved a Log View previously the state matching overrides this. I'd prefer to follow this up in a separate issue as I'd like to think through the ramifications a bit more. It's not a bug, but it is jarring UX. --------- Co-authored-by: Kibana Machine <[email protected]>
Summary
This closes #142840. It is the UI portion of support for inline Log Views.
Visible changes to the UI
ML warning
Alert dropdown warning
Settings page warning
Reviewer hints / notes
The ACs on the issue are quite extensive and should provide a good number of things to test.
The
AlertDropdown
has been made lazy as the page load bundle increased by 100kb otherwise.Our
link-to
functionality is scoped to persisted Log Views only at the moment as historically they've only accepted a path segment, not full query parameters. We can look to extend this in the future once we have concrete linking needs.Questions
I have allowed the Log View client to handle both the inline and persisted Log Views. I wonder if the function names become confusing? (e.g. are the RESTful prefixes misleading now?).
The ML warning splash page links to settings to revert to a persisted Log View. It could also be done in place on the page. I went back and forth over whether we should keep the reverting in one place?
Testing
There is now a "state machine playground" available at the following URL:
/app/logs/state-machine-playground
, it is enabled in developer mode only. It's not fancy or pretty it just serves to make testing things easier. There are various buttons, e.g.Switch to inline Log View
, to facilitate easier testing whilst a Log View switcher is not in the UI itself. You can utilise these buttons, and then head to other pages to ensure things are working correctly, e.g. warning callouts and disabled buttons etc. If you'd like to play with the options used, e.g. forupdate
, amend the code withinstate_machine_playground.tsx
. It's also useful just to sit on this page, try various things, and verify what happens in the developer tools (does the context update correctly etc).Known issues
main
. The reason for this is the check within settings looks like so:but the
resolvedLogView
state matching looks like so:so even though we have resolved a Log View previously the state matching overrides this. I'd prefer to follow this up in a separate issue as I'd like to think through the ramifications a bit more. It's not a bug, but it is jarring UX.