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

nreese · 2023-09-28T16:56:35Z

Reinstates changes from #166565. Original changes reverted by #167485. This PR only returns method, path, and querystring keys from request parameters.

When reviewing, 933a5ac are the changes that resolve the issue and the only different changes from the revert of the revert.

I updated the integration tests to check entire requestParams object instead of individual keys. This gives us a guarentee that 'headers' is not getting returned from the API since it will fail these tests.

…g" (elastic#167485)" This reverts commit 239e503.

…-ref HEAD~1..HEAD --fix'

kibana-ci · 2023-09-28T18:10:07Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 70ddf18

Failed CI Steps

FTR Configs #71

Test Failures

[job] [logs] FTR Configs #71 / event_log Event Log public API "after all" hook in "Event Log public API"
[job] [logs] FTR Configs #71 / event_log Event Log public API Legacy Ids "before all" hook for "should support search event by ids and legacyIds"

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`inspector`	56	57	+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
`kibanaUtils`	417	421	+4

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

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`kibanaUtils`	9	10	+1

Page load bundle

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

id	before	after	diff
`data`	408.0KB	408.2KB	+205.0B
`inspector`	21.8KB	22.2KB	+414.0B
total			+619.0B

Unknown metric groups

API count

id	before	after	diff
`data`	3311	3313	+2
`kibanaUtils`	610	614	+4
total			+6

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

elasticmachine · 2023-09-28T18:56:45Z

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

ThomThomson

Changes in 933a5ac LGTM! Nice updates to the integration tests especially to ensure the response is sanitized.

jughosta

LGTM 👍

Is it expected that Inspector in Graph and Vega apps does not show request params yet?

rudolf

By changing reportServerError we're changing the behaviour of a lot of server APIs owned by other teams. We should rather limit this behaviour change to the data plugin only.

rudolf · 2023-10-02T08:34:53Z

src/plugins/data/common/search/types.ts

+  /**
+   * HTTP request parameters from elasticsearch transport client t
+   */
+  requestParams?: ConnectionRequestParams;


Can we construct a dedicated type that only includes the information we intend to send back?

rudolf · 2023-10-02T09:21:51Z

src/plugins/kibana_utils/server/report_server_error.ts

@@ -43,6 +56,7 @@ export function reportServerError(res: KibanaResponseFactory, err: KbnServerErro
    body: {
      message: err.message,
      attributes: err.errBody?.error,
+      ...(err.requestParams ? { requestParams: err.requestParams } : {}),


KbnServerError and reportServerError are widely used outside of data plugin for catching any kind of error in route handlers. It's risky to return the 'requestParams' for all the places this gets used and this would also not be a pattern that we'd recommend gets widely adopted.

We should rather move this functionality and sanitizeRequestParams into the data plugin.

I am not sure I understand, are you suggesting not using KbnServerError and reportServerError in data plugin?

yes, we could create a new reportElasticsearchErrorAndRequestParams (naming tbd) in the data plugin and leave the reportServerError util as-is so we don't change the APIs of any other plugins/teams.

ok, I will do that. That aligns well with another effort to return rawResponse from data plugin search endpoints, #167099. I will work #167099 first to migrate data search plugin away from KbnServerError and reportServerError so that rawResponse is included in error response. Then I can circle back to this PR and build off of those data plugin only utility methods

rudolf · 2023-10-02T09:23:50Z

src/plugins/kibana_utils/server/sanitize_request_params.ts

+  'method' | 'path' | 'querystring'
+>;
+
+export function sanitizeRequestParams(requestParams: ConnectionRequestParams) {


Suggested change

export function sanitizeRequestParams(requestParams: ConnectionRequestParams) {

export function sanitizeRequestParams(requestParams: ConnectionRequestParams): SanitizedConnectionRequestParams {

Closes #167099 #### Problem `/bsearch` and `/search` APIs only return `error` key from elasticsearch error response. This is problematic because Inspector needs `rawResponse` to populate "Clusters and shards" While working on this issue, I discovered another problem with how error responses are added to inspector requestResponder. The `Error` instance is added as `json` key. This is a little awkward since the response tab just stringifies the contents of `json`, thus stringifing the Error object instead of just the error body returned from API. This PR address this problem by setting `json` to either `attributes` or `{ message }`. #### Solution PR updates `/bsearch` and `/search` APIs to return `{ attributes: { error: ErrorCause, rawResponse }}` for failed responses. Solution avoided changing KbnServerError and reportServerError since these methods are used extensivly throughout Kibana (see #167544 (comment) for more details). Instead, KbnSearchError and reportSearchError are created to report search error messages. #### Test 1) install web logs sample data set 2) open discover 3) add filter ``` { "error_query": { "indices": [ { "error_type": "exception", "message": "local shard failure message 123", "name": "kibana_sample_data_logs", "shard_ids": [ 0 ] } ] } } ``` 4) Open inspector. Verify "Clusters and shards" tab is visible and populated. Verify "Response" tab shows "error" and "rawResponse" keys. <img width="500" alt="Screenshot 2023-10-09 at 9 29 16 AM" src="https://github.com/elastic/kibana/assets/373691/461b0eb0-0502-4d48-a487-68025ef24d35"> <img width="500" alt="Screenshot 2023-10-09 at 9 29 06 AM" src="https://github.com/elastic/kibana/assets/373691/9aff41eb-f771-48e3-a66d-1447689c2c6a"> --------- Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Gloria Hornero <[email protected]>

nreese · 2023-10-26T18:27:35Z

Replacing with #169970.

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. <img width="500" alt="Screen Shot 2023-09-16 at 4 19 58 PM" src="https://github.com/elastic/kibana/assets/373691/56019fb5-ca88-46cf-a42f-86f5f51edfcc"> ### 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 #166565 was reverted for exposing `headers`. 2) [Fix to only expose method, path, and querystring keys from request parameters](#167544) 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. --------- Co-authored-by: kibanamachine <[email protected]>

nreese added 2 commits September 28, 2023 09:37

Revert "Revert "[inspector] show request method, path, and querystrin…

f9dad68

…g" (elastic#167485)" This reverts commit 239e503.

sanitize request parameters

933a5ac

nreese requested a review from rudolf September 28, 2023 16:58

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

70ddf18

…-ref HEAD~1..HEAD --fix'

nreese marked this pull request as ready for review September 28, 2023 18:56

nreese requested review from a team as code owners September 28, 2023 18:56

nreese added release_note:enhancement Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.11.0 labels Sep 28, 2023

nreese added the Feature:Inspector Inspector infrastructure and implementations label Sep 28, 2023

ThomThomson approved these changes Sep 28, 2023

View reviewed changes

jughosta approved these changes Sep 29, 2023

View reviewed changes

stratoula approved these changes Sep 29, 2023

View reviewed changes

Dosant approved these changes Sep 29, 2023

View reviewed changes

rudolf requested changes Oct 2, 2023

View reviewed changes

nreese added v8.12.0 and removed v8.11.0 labels Oct 9, 2023

nreese mentioned this pull request Oct 9, 2023

[search source] return rawResponse on search failure #168389

Merged

nreese closed this Oct 26, 2023

nreese mentioned this pull request Oct 27, 2023

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nreese commented Sep 28, 2023 •

edited

Loading

kibana-ci commented Sep 28, 2023

API count

elasticmachine commented Sep 28, 2023

ThomThomson left a comment

jughosta left a comment

rudolf left a comment

rudolf Oct 2, 2023

rudolf Oct 2, 2023

nreese Oct 2, 2023

rudolf Oct 2, 2023 •

edited

Loading

nreese Oct 2, 2023

rudolf Oct 2, 2023

nreese commented Oct 26, 2023

	export function sanitizeRequestParams(requestParams: ConnectionRequestParams) {
	export function sanitizeRequestParams(requestParams: ConnectionRequestParams): SanitizedConnectionRequestParams {

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

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

Conversation

nreese commented Sep 28, 2023 • edited Loading

kibana-ci commented Sep 28, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

elasticmachine commented Sep 28, 2023

ThomThomson left a comment

Choose a reason for hiding this comment

jughosta left a comment

Choose a reason for hiding this comment

rudolf left a comment

Choose a reason for hiding this comment

rudolf Oct 2, 2023

Choose a reason for hiding this comment

rudolf Oct 2, 2023

Choose a reason for hiding this comment

nreese Oct 2, 2023

Choose a reason for hiding this comment

rudolf Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

nreese Oct 2, 2023

Choose a reason for hiding this comment

rudolf Oct 2, 2023

Choose a reason for hiding this comment

nreese commented Oct 26, 2023

nreese commented Sep 28, 2023 •

edited

Loading

rudolf Oct 2, 2023 •

edited

Loading