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

[Console] Handle multiple request exceptions #129443

Merged

Conversation

mibragimov
Copy link
Contributor

@mibragimov mibragimov commented Apr 5, 2022

Closes #78919

Summary

Fixes outputting multiple request results with exceptions in editor output.

Testing

To test this out, send the following requests at once

PUT test 
{
  "mappings": {
    "properties": {
      "trips" : {
        "type": "nested"
      }
    }
  }
}

POST test/_doc
{
  "value": "abc",
  "trips": [
    {
      "type": "home",
      "changes": 0
    },
    {
      "type": "home",
      "changes": 1
    }
  ]
}

POST test/_doc
{
  "value": "abc",
  "trips": [
    {
      "type": "home",
      "changes": 0
    },
    {
      "type": "home",
      "changes": 0
    }
  ]
}

POST test/_doc
{
  "trips" : 1
}

POST test/_refresh

Release Note

Console now supports properly handling multiple requests. For es errors such as 400, 405 exception results are displayed with successful request results in the order they called. For non-es errors such as 500, it outputs only the first exception result.

Screenshots

2022-04-14_13-39
2022-04-14_13-43

@mibragimov mibragimov added the WIP Work in progress label Apr 5, 2022
@mibragimov mibragimov self-assigned this Apr 5, 2022
@mibragimov mibragimov force-pushed the console/multiple-request-exception branch from 7b4f983 to 8be756e Compare April 14, 2022 08:12
@mibragimov mibragimov added release_note:enhancement Feature:Console Dev Tools Console Feature Feature:Dev Tools v8.3.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more and removed WIP Work in progress labels Apr 14, 2022
Copy link
Contributor

@hro-maker hro-maker left a comment

Choose a reason for hiding this comment

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

code looks good for me :)

@mibragimov mibragimov force-pushed the console/multiple-request-exception branch from 8be756e to 2e31b0b Compare April 25, 2022 06:19
@mibragimov
Copy link
Contributor Author

@elasticmachine merge upstream

@mibragimov mibragimov marked this pull request as ready for review April 25, 2022 11:26
@mibragimov mibragimov requested a review from a team as a code owner April 25, 2022 11:26
@elasticmachine
Copy link
Contributor

Pinging @elastic/platform-deployment-management (Team:Deployment Management)

@mibragimov mibragimov requested a review from sabarasaba April 25, 2022 11:26
Copy link
Member

@sabarasaba sabarasaba left a comment

Choose a reason for hiding this comment

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

Thanks for patching this up @mibragimov! code changes lgtm, tested locally.

I just have a few UX concerns about the changes:

  1. When multiple requests are sent the http status label only shows the state of the last one, even though there were a few that failed. Not sure how this could be addressed though.
  2. It's not very obvious from the output shown in the preview panel which request failed and which one didn't. Is it possible to highlight it somehow or show something next to line numbers? We could potentially do this with a separate issue though, wdyt?

Screenshot 2022-04-26 at 13 13 47

Copy link
Member

@sabarasaba sabarasaba left a comment

Choose a reason for hiding this comment

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

I've created a new issue for tracking those two UX concerns #130982

Muhammad Ibragimov added 2 commits April 27, 2022 13:47
- Extract a way of getting content-type to a separate function
@mibragimov mibragimov force-pushed the console/multiple-request-exception branch from c27d588 to 0e0f459 Compare April 27, 2022 09:04
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
console 399.4KB 399.4KB -7.0B

History

  • 💚 Build #40080 succeeded c27d588ea84ddb78cd2abd2a2f71148e8775fe9c
  • 💔 Build #40077 failed 2e31b0b60f9f8c66f9089776d3a66e5b80e84747
  • 💚 Build #38137 succeeded 8be756e742c9cf978b907d2aa5f8a5468c4c2a6a
  • 💚 Build #36102 succeeded 7b4f9835f068db9c1a925a5399e1c97f865a82b2

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

cc @mibragimov

@mibragimov mibragimov merged commit eaf47ae into elastic:main Apr 27, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 27, 2022
@mibragimov mibragimov deleted the console/multiple-request-exception branch April 27, 2022 10:47
kertal pushed a commit to kertal/kibana that referenced this pull request May 24, 2022
* Add handling multiple request exceptions

- Fix code styling for send_request.test.ts
- Extract a way of getting content-type to a separate function

Co-authored-by: Muhammad Ibragimov <muhammad.ibragimov@elastic.co>
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:Console Dev Tools Console Feature Feature:Dev Tools release_note:enhancement Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dev Tools Console: When sending multiple requests, you cannot figure out which one triggered an exception
7 participants