Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Discover][ES|QL] No legacy table for ES|QL mode #180370

Merged
merged 3 commits into from
Apr 10, 2024
Merged

[Discover][ES|QL] No legacy table for ES|QL mode #180370

merged 3 commits into from
Apr 10, 2024

Conversation

jughosta
Copy link
Contributor

@jughosta jughosta commented Apr 9, 2024

Summary

Even if doc_table:legacy is enabled this PR makes sure that we render the new grid for ES|QL mode.

Checklist

@jughosta jughosta self-assigned this Apr 9, 2024
@jughosta jughosta added Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. Feature:ES|QL ES|QL related features in Kibana labels Apr 9, 2024
@jughosta
Copy link
Contributor Author

jughosta commented Apr 9, 2024

/ci

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
canvas 1214 1215 +1
cloudSecurityPosture 416 417 +1
discover 778 779 +1
eventAnnotationListing 502 503 +1
lens 1383 1384 +1
logsExplorer 709 710 +1
savedSearch 47 48 +1
securitySolution 5298 5299 +1
slo 672 673 +1
unifiedDocViewer 119 120 +1
total +10

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/discover-utils 37 41 +4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
discover 593.0KB 593.4KB +410.0B
unifiedDocViewer 60.6KB 60.6KB +62.0B
total +472.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
unifiedDocViewer 9.7KB 10.0KB +223.0B
Unknown metric groups

API count

id before after diff
@kbn/discover-utils 59 63 +4

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @jughosta

@jughosta jughosta marked this pull request as ready for review April 9, 2024 15:01
@jughosta jughosta requested a review from a team as a code owner April 9, 2024 15:01
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

davismcphee
davismcphee approved these changes Apr 10, 2024
View reviewed changes
Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes look good and it works well! Thanks for addressing it so quickly, LGTM 👍

src/plugins/discover/public/application/main/components/layout/discover_documents.tsx
@@ -140,15 +140,19 @@ function DiscoverDocumentsComponent({

const expandedDoc = useInternalStateSelector((state) => state.expandedDoc);

const isTextBasedQuery = useMemo(() => getRawRecordType(query) === RecordRawType.PLAIN, [query]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const isTextBasedQuery = useMemo(() => getRawRecordType(query) === RecordRawType.PLAIN, [query]);
const isTextBasedQueryMode = useMemo(() => isTextBasedQuery(query), [query]);

Nit: I know this was just moved up a couple of lines, but it might be a good opportunity to switch to the isTextBasedQuery util from src/plugins/discover/public/application/main/utils/is_text_based_query.ts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tbh I think we should delete isTextBasedQuery discover wrapper and use isOfAggregateQueryType directly instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davismcphee

  • forgot to add the mention in the comment above

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would work for me too, my main concern is really just around consistency in the utils we use. Personally I find isOfAggregateQueryType harder to grok than isTextBasedQuery since aggregate query is an unclear term to me, but that's more of a nit than anything as long as we're consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something for a later cleanup. I would like minimize extra refactoring shortly before FF as it's unrelated to the current ticket.

src/plugins/discover/public/application/main/components/top_nav/on_save_search.tsx
savedSearch.rowsPerPage = uiSettings.get(DOC_TABLE_LEGACY)
savedSearch.rowsPerPage = isLegacyTableEnabled({
uiSettings,
isTextBasedQueryMode: isOfAggregateQueryType(savedSearch.searchSource.getField('query')),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
isTextBasedQueryMode: isOfAggregateQueryType(savedSearch.searchSource.getField('query')),
isTextBasedQueryMode: isTextBasedQuery(savedSearch.searchSource.getField('query')),

Nit: similar suggestion here

@jughosta jughosta merged commit 099c073 into elastic:main Apr 10, 2024
16 checks passed
@jughosta jughosta deleted the 180286-no-legacy-for-esql branch April 10, 2024 14:51
kibanamachine added a commit that referenced this pull request May 15, 2024
@kibanamachine @jughosta
[8.14] [Discover] Fix JSON view height in DocViewer (#183468) (#183502)
4864ec3 
# Backport

This will backport the following commits from `main` to `8.14`:
- [[Discover] Fix JSON view height in DocViewer
(#183468)](#183468)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Julia
Rechkunova","email":"julia.rechkunova@elastic.co"},"sourceCommit":{"committedDate":"2024-05-15T11:06:24Z","message":"[Discover]
Fix JSON view height in DocViewer (#183468)\n\n- A fix
after\r\nhttps://github.com//pull/180370/files#diff-ed350967c6564d64f0123a2d55700b1ddc7f5d6fcb3786384fb7ae25334fe9eaL672\r\n\r\n##
Summary\r\n\r\nThis PR fixes the height of JSON block in DocViewer.
~Also it reduces\r\nthe available height as the flyout now has a footer
with Close
button.~","sha":"994b8c2beab211e15facff3eec3676d8e96bcbea","branchLabelMapping":{"^v8.15.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Discover","release_note:skip","Team:DataDiscovery","backport:prev-minor","Feature:UnifiedDocViewer","v8.15.0"],"title":"[Discover]
Fix JSON view height in
DocViewer","number":183468,"url":"https://github.com/elastic/kibana/pull/183468","mergeCommit":{"message":"[Discover]
Fix JSON view height in DocViewer (#183468)\n\n- A fix
after\r\nhttps://github.com//pull/180370/files#diff-ed350967c6564d64f0123a2d55700b1ddc7f5d6fcb3786384fb7ae25334fe9eaL672\r\n\r\n##
Summary\r\n\r\nThis PR fixes the height of JSON block in DocViewer.
~Also it reduces\r\nthe available height as the flyout now has a footer
with Close
button.~","sha":"994b8c2beab211e15facff3eec3676d8e96bcbea"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.15.0","branchLabelMappingKey":"^v8.15.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/183468","number":183468,"mergeCommit":{"message":"[Discover]
Fix JSON view height in DocViewer (#183468)\n\n- A fix
after\r\nhttps://github.com//pull/180370/files#diff-ed350967c6564d64f0123a2d55700b1ddc7f5d6fcb3786384fb7ae25334fe9eaL672\r\n\r\n##
Summary\r\n\r\nThis PR fixes the height of JSON block in DocViewer.
~Also it reduces\r\nthe available height as the flyout now has a footer
with Close
button.~","sha":"994b8c2beab211e15facff3eec3676d8e96bcbea"}}]}]
BACKPORT-->

Co-authored-by: Julia Rechkunova <julia.rechkunova@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davismcphee davismcphee davismcphee approved these changes

Assignees

@jughosta jughosta

Labels
backport:skip This commit does not require backporting Feature:Discover Discover Application Feature:ES|QL ES|QL related features in Kibana release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.14.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Discover][ES|QL] Legacy table should not be rendered for ES|QL mode
5 participants
@jughosta @kibana-ci @elasticmachine @davismcphee @kibanamachine