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.

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

[inspector] show request method, path, and querystring #169970

Merged
merged 12 commits into from
Nov 3, 2023

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Oct 26, 2023

Closes #45931

PR updates data plugin search and bsearch endpoints to return method, path, and querystring request params from elasticsearch-js client requests. This provides inspector with the exact details used to fetch data from elasticsearch, ensuring inspector displays request exactly as used by elasticsearch-js client.

ESQL This PR makes it possible to open ESQL searches in console.
Screen Shot 2023-09-16 at 4 19 58 PM

background

If you are thinking to yourself, "haven't I reviewed this before?", you are right. This functionality has been through several iterations.

  1. Original PR [inspector] show request method, path, and querystring #166565 was reverted for exposing headers.
  2. Fix to only expose method, path, and querystring keys from request parameters was rejected because it applied changes to kibana_utils/server/report_server_error.ts, which is used extensively throughout Kibana.
  3. This iteration moves logic into the data plugin to be as narrow as possible.

@nreese nreese force-pushed the issue_45931_data_plugin_sanitized branch from 89fd4c8 to 3296dda Compare October 26, 2023 18:38
@nreese nreese force-pushed the issue_45931_data_plugin_sanitized branch from 60e9b2e to b3ac7d2 Compare October 26, 2023 19:30
@nreese
Copy link
Contributor Author

nreese commented Oct 26, 2023

@elasticmachine merge upstream

@nreese nreese marked this pull request as ready for review October 27, 2023 01:13
@nreese nreese requested review from a team as code owners October 27, 2023 01:13
@nreese nreese added release_note:enhancement Feature:Inspector Inspector infrastructure and implementations Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.12.0 labels Oct 27, 2023
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Tested locally with Safari 👍 .

Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Changes to inspector LGTM! Code only review.

Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

This is a huge improvement! Spent some time playing around with it and this will be super helpful in narrowing down issues related to search. Just had one comment, more of a discussion rather than feedback.

@@ -134,6 +139,9 @@ export const enhancedEsSearchStrategyProvider = (
const response = esResponse.body as estypes.SearchResponse<any>;
return {
rawResponse: shimHitsTotal(response, options),
...(esResponse.meta?.request?.params
? { requestParams: sanitizeRequestParams(esResponse.meta?.request?.params) }
Copy link
Member

Choose a reason for hiding this comment

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

This assumes we always want the request params being sent in the response - this will cause inflated responses most of the time even in cases where it's not used (e.g. the response from a GET request after the search has already been submitted). Can we make this so the behavior is opt-in, or at least remove these parameters from cases where the client should already have the request parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would this look? Would consumers of SearchSource pass in an option to turn-on request parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated server side logic to avoid returning requestParams for async polling requests 3f1bc36

@nreese
Copy link
Contributor Author

nreese commented Nov 1, 2023

@elasticmachine merge upstream

@nreese nreese requested a review from lukasolson November 1, 2023 18:24
Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

LGTM!

@nreese
Copy link
Contributor Author

nreese commented Nov 3, 2023

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #73 / detection engine api security and spaces enabled - rule execution logic Machine learning type rules "before all" hook for "should create 1 alert from ML rule when record meets anomaly_threshold"

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
inspector 57 58 +1

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
data 2537 2542 +5

Async chunks

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

id before after diff
inspector 26.2KB 26.4KB +203.0B

Page load bundle

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

id before after diff
data 411.2KB 411.4KB +198.0B
inspector 21.8KB 22.0KB +176.0B
total +374.0B
Unknown metric groups

API count

id before after diff
data 3186 3193 +7

History

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

@nreese nreese merged commit f284454 into elastic:main Nov 3, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Inspector Inspector infrastructure and implementations release_note:enhancement Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inspector should show request endpoint as well as body
7 participants