-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Discover / Logs] Add new "Saved Search component" #199787
[Discover / Logs] Add new "Saved Search component" #199787
Conversation
45ae4a7
to
c1b0411
Compare
2cd148a
to
e6fd5f5
Compare
Pinging @elastic/obs-ux-logs-team (Team:obs-ux-logs) |
Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services) |
🤖 GitHub commentsExpand to view the GitHub comments
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.
APM changes LGTM
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.
Overall looks good, nice job! I left some minor comments, but it works and I can see there are some pending issues to address in the PR description! I'll give it a second pass once more changes will be added.
packages/kbn-saved-search-component/src/components/saved_search.tsx
Outdated
Show resolved
Hide resolved
packages/kbn-saved-search-component/src/components/saved_search.tsx
Outdated
Show resolved
Hide resolved
packages/kbn-saved-search-component/src/components/saved_search.tsx
Outdated
Show resolved
Hide resolved
packages/kbn-saved-search-component/src/components/saved_search.tsx
Outdated
Show resolved
Hide resolved
src/plugins/discover/public/embeddable/initialize_search_embeddable_api.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability_solution/apm/public/components/app/service_logs/index.tsx
Outdated
Show resolved
Hide resolved
ab8f032
to
5b05acf
Compare
@tonyghiani Thanks for the review, I've responded to your feedback 👍 Regarding followups in this PR, there's nothing more to come. The mixing of columns will be handled by the Discover folks, and the top-right panel button was something I just wanted to point out. So this should be considered "done" 👍 |
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.
Thanks for the changes Kerry, LGTM 👏
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.
Data Discovery changes LGTM overall! I left a few comments, but the only blocking one is related to a new prop on SavedSearchAttributes
.
src/plugins/discover/public/embeddable/components/saved_search_grid.tsx
Outdated
Show resolved
Hide resolved
src/plugins/discover/public/embeddable/initialize_search_embeddable_api.tsx
Show resolved
Hide resolved
…/kibana into obs-saved-search-embeddable-poc
@davismcphee Thanks for the review 🙏 I've responded to all the feedback, this is ready for another look. |
@@ -37,7 +37,8 @@ | |||
"lens", | |||
"maps", | |||
"uiActions", | |||
"logsDataAccess" | |||
"logsDataAccess", | |||
"savedSearch", |
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.
savedSearch is platform shared so does not impact solution independence ✅
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.
Thanks for the updates, and with that this LGTM on the Data Discovery side 👍 Great work, @Kerry350! I'm excited we've gotten to a point where we can rely on Discover directly for these things 🙂
src/plugins/discover/public/embeddable/initialize_search_embeddable_api.tsx
Show resolved
Hide resolved
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
History
cc @Kerry350 |
Manually removing |
Starting backport for target branches: 8.x |
💔 All backports failed
Manual backportTo create the backport manually run:
Questions ?Please refer to the Backport tool documentation |
Summary
Implements https://github.com/elastic/logs-dev/issues/111#issuecomment-2446470635.
This adds a new "Saved Search component". The component is a wrapper around the current Saved Search Embeddable, but uses
ReactEmbeddableRenderer
directly to render the embeddable outside of Dashboard contexts. It monitors changes to things likeindex
,filters
etc and communicates these changes through the embeddable API.For this PoC two locations were changed to use this component 1) Logs Overview flyout 2) APM Logs tab (when the Logs Overview isn't enabled via advanced settings).
The component itself is technically beyond a PoC, and resides in it's own package.
I'd like to get eyes from the Discover folks etc on the approach, and if we're happy I can fix the remaining known issues (apart from the mixing of columns point as I believe this exists on the roadmap anyway) and we can merge this for the initial two replacement points.Thanks Davis 👌.nonPersistedDisplayOptions
is added to facilitate some configurable options via runtime state, but without the complexity of altering the actual saved search saved object.On the whole I've tried to keep this as clean as possible whilst working within the embeddable framework, outside of a dashboard context.
Known issues
"Flyout on flyout" in the logs overview flyout (e.g. triggering the table's flyout in this context).Fixed withenableFlyout
option.Filter buttons should be disabled via pills (e.g. in Summary column).Fixed withenableFilters
option.Summary (
_source
) column cannot be used alongside other columns, e.g. log level, so column customisation isn't currently enabled. You'll just get timestamp and summary. This requires changes in the Unified Data Table. Won't be fixed in this PRWe are left with this panel button that technically doesn't do anything outside of a dashboard. I don't think there's an easy way to disable this. Won't be fixed in this PR
Followups
The Logs Overview details state machine can be cleaned up (it doesn't need to fetch documents etc anymore).The state machine no longer fetches it's own documents. Some scaffolding is left in place as it'll be needed for showing category details anyway.Example