Skip to content

[search source] open incomplete response warning in inspector#167205

Merged
nreese merged 30 commits intoelastic:mainfrom
nreese:open_in_inspector
Oct 2, 2023
Merged

[search source] open incomplete response warning in inspector#167205
nreese merged 30 commits intoelastic:mainfrom
nreese:open_in_inspector

Conversation

@nreese
Copy link
Copy Markdown
Contributor

@nreese nreese commented Sep 25, 2023 •
edited
Loading

Closes #167098

PR updates "View details" button in incomplete response callouts to open inspector to request id and cluster tab. PR them removes shards model as its no longer used.

Clicking "View details"
Screenshot 2023-09-28 at 9 19 49 AM

Opens
Screenshot 2023-09-28 at 9 21 35 AM

@nreese nreese mentioned this pull request Sep 26, 2023
Merged
@nreese
Copy link
Copy Markdown
Contributor Author

nreese commented Sep 27, 2023

@elasticmachine merge upstream

@nreese
Copy link
Copy Markdown
Contributor Author

nreese commented Sep 28, 2023

@elasticmachine merge upstream

@nreese nreese marked this pull request as ready for review September 29, 2023 14:17
@nreese nreese requested review from a team as code owners September 29, 2023 14:17
@nreese nreese added release_note:enhancement Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// v8.11.0 labels Sep 29, 2023
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Sep 29, 2023

Pinging @elastic/kibana-presentation (Team:Presentation)

@nickpeihl nickpeihl self-requested a review September 29, 2023 17:28
nickpeihl
nickpeihl approved these changes Sep 29, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@nickpeihl nickpeihl left a comment

Choose a reason for hiding this comment

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

inspector plugin changes lgtm. Showing the incomplete results in the inspector looks so much better than the modal!

code review and tested using these instructions

jughosta
jughosta approved these changes Oct 2, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@jughosta jughosta left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Data Discovery changes LGTM 👍
Agree that the clusters flyout looks much better than the modal!

stratoula
stratoula approved these changes Oct 2, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Lens changes LGTM, code review only

@drewdaemon
Copy link
Copy Markdown
Contributor

drewdaemon commented Oct 2, 2023 •
edited
Loading

We are now rendering a Lens visualization itself within a flyout. Any idea how these changes will interact? (I can check, but won't be until later today).

Screenshot 2023-10-02 at 7 21 54 AM

@nreese
Copy link
Copy Markdown
Contributor Author

nreese commented Oct 2, 2023

We are now rendering a Lens visualization itself within a flyout. Any idea how these changes will interact? (I can check, but won't be until later today).

Inspector flyout would open on top of lens visualization flyout, just like the existing modal would

@nreese
Copy link
Copy Markdown
Contributor Author

nreese commented Oct 2, 2023

@elasticmachine merge upstream

@kibana-ci
Copy link
Copy Markdown

kibana-ci commented Oct 2, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #59 / Reporting Functional Tests with Deprecated Security configuration enabled Access to Management > Reporting does allow user with reporting_user role
  • [job] [logs] FTR Configs #44 / serverless search UI empty pages should show search specific empty page in dashboards

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
data 476 469 -7
inspector 56 57 +1
total -6

Async chunks

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

id before after diff
data 54.3KB 48.3KB -6.0KB
discover 566.0KB 565.9KB -98.0B
inspector 26.3KB 26.2KB -52.0B
lens 1.4MB 1.4MB +18.0B
total -6.1KB

Page load bundle

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

id before after diff
data 407.9KB 407.9KB +13.0B
Unknown metric groups

API count

id before after diff
data 3311 3312 +1

ESLint disabled line counts

id before after diff
inspector 1 2 +1

References to deprecated APIs

id before after diff
data 73 71 -2

Total ESLint disabled count

id before after diff
inspector 3 4 +1

History

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

MichaelMarcialis
MichaelMarcialis approved these changes Oct 2, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

Code review only. LGTM!

I only have one quick tangent question, @nreese. Am I correct in assuming that some of the larger changes being suggested to the design for the inspect visualization flyout will be handled in a future issue/PR (such as general flyout IA changes, query and tab selection placement, cluster row contents, etc.)?

@nreese
Copy link
Copy Markdown
Contributor Author

nreese commented Oct 2, 2023

Am I correct in assuming that some of the larger changes being suggested to the design for the inspect visualization flyout will be handled in a future issue/PR (such as general flyout IA changes, query and tab selection placement, cluster row contents, etc.)?

Correct. Inspector changes are separate from this effort

@nreese nreese merged commit 5bbdec9 into elastic:main Oct 2, 2023
@kibanamachine kibanamachine added the backport:skip This PR does not require backporting label Oct 2, 2023
@markov00 markov00 mentioned this pull request Sep 10, 2024
Merged
markov00 added a commit that referenced this pull request Oct 14, 2024
@markov00
[Lens] Correctly use UserMessage longMessage as function (#192492)
e35507a 
## Summary

After #167205 was merged, the
`UserMessage.longMessage` was typed as `longMessage: string |
React.ReactNode | ((closePopover: () => void) => React.ReactNode);`

With the upcoming React 18 upgrade, an error will become visible because
`((closePopover: () => void) => React.ReactNode);` can't be used as a
ReactNode but it correctly needs to be called.

In this PR I've made the `closePopover` function being optional (to
simplify the refactoring) and I've added the typecheck where needed.
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 14, 2024
@markov00
[Lens] Correctly use UserMessage longMessage as function (elastic#192492
ca2fa7b 
)

## Summary

After elastic#167205 was merged, the
`UserMessage.longMessage` was typed as `longMessage: string |
React.ReactNode | ((closePopover: () => void) => React.ReactNode);`

With the upcoming React 18 upgrade, an error will become visible because
`((closePopover: () => void) => React.ReactNode);` can't be used as a
ReactNode but it correctly needs to be called.

In this PR I've made the `closePopover` function being optional (to
simplify the refactoring) and I've added the typecheck where needed.

(cherry picked from commit e35507a)
kibanamachine added a commit that referenced this pull request Oct 14, 2024
@kibanamachine @markov00
[8.x] [Lens] Correctly use UserMessage longMessage as function (#192492…
7e8d54c 
…) (#196194)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Lens] Correctly use UserMessage longMessage as function
(#192492)](#192492)

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

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

<!--BACKPORT [{"author":{"name":"Marco
Vettorello","email":"marco.vettorello@elastic.co"},"sourceCommit":{"committedDate":"2024-10-14T17:34:36Z","message":"[Lens]
Correctly use UserMessage longMessage as function (#192492)\n\n##
Summary\r\n\r\nAfter #167205 was
merged, the\r\n`UserMessage.longMessage` was typed as `longMessage:
string |\r\nReact.ReactNode | ((closePopover: () => void) =>
React.ReactNode);`\r\n\r\nWith the upcoming React 18 upgrade, an error
will become visible because\r\n`((closePopover: () => void) =>
React.ReactNode);` can't be used as a\r\nReactNode but it correctly
needs to be called.\r\n\r\nIn this PR I've made the `closePopover`
function being optional (to\r\nsimplify the refactoring) and I've added
the typecheck where
needed.","sha":"e35507a27d9c8df3fe5947c7227d6072d007dfa5","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Feature:Lens","v9.0.0","backport:prev-minor","v8.16.0"],"title":"[Lens]
Correctly use UserMessage longMessage as function
","number":192492,"url":"https://github.com/elastic/kibana/pull/192492","mergeCommit":{"message":"[Lens]
Correctly use UserMessage longMessage as function (#192492)\n\n##
Summary\r\n\r\nAfter #167205 was
merged, the\r\n`UserMessage.longMessage` was typed as `longMessage:
string |\r\nReact.ReactNode | ((closePopover: () => void) =>
React.ReactNode);`\r\n\r\nWith the upcoming React 18 upgrade, an error
will become visible because\r\n`((closePopover: () => void) =>
React.ReactNode);` can't be used as a\r\nReactNode but it correctly
needs to be called.\r\n\r\nIn this PR I've made the `closePopover`
function being optional (to\r\nsimplify the refactoring) and I've added
the typecheck where
needed.","sha":"e35507a27d9c8df3fe5947c7227d6072d007dfa5"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/192492","number":192492,"mergeCommit":{"message":"[Lens]
Correctly use UserMessage longMessage as function (#192492)\n\n##
Summary\r\n\r\nAfter #167205 was
merged, the\r\n`UserMessage.longMessage` was typed as `longMessage:
string |\r\nReact.ReactNode | ((closePopover: () => void) =>
React.ReactNode);`\r\n\r\nWith the upcoming React 18 upgrade, an error
will become visible because\r\n`((closePopover: () => void) =>
React.ReactNode);` can't be used as a\r\nReactNode but it correctly
needs to be called.\r\n\r\nIn this PR I've made the `closePopover`
function being optional (to\r\nsimplify the refactoring) and I've added
the typecheck where
needed.","sha":"e35507a27d9c8df3fe5947c7227d6072d007dfa5"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Marco Vettorello <marco.vettorello@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jughosta jughosta jughosta approved these changes

@nickpeihl nickpeihl nickpeihl approved these changes

@MichaelMarcialis MichaelMarcialis MichaelMarcialis approved these changes

@stratoula stratoula stratoula approved these changes

Assignees

No one assigned

Labels

backport:skip This PR does not require backporting release_note:enhancement Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// v8.11.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

open inspector clusters tab when viewing incomplete data details

9 participants

@nreese @elasticmachine @drewdaemon @kibana-ci @jughosta @nickpeihl @MichaelMarcialis @stratoula @kibanamachine