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

[lens] show 'View details' UI action to open clusters inspector tab when request fails #172971

Merged
merged 25 commits into from
Dec 20, 2023

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Dec 8, 2023

Closes #171570

PR make the following changes

  1. Consolidates data EsError logic and Lens EsError logic. This resulted in being able to remove large parts of lens error_helper.tsx file.
  2. Consolidates lens WorkspacePanel error logic. Before PR configuration errors and data loading errors each rendered their own version of EuiEmptyPrompt with slightly different behavior. Now, both render WorkspaceErrors component and have the same behavior
  3. Updated lens ExpressionWrapper to return original error to embeddable output.

Test - EsError in embeddable

  1. install sample web logs
  2. create new dashboard
  3. Click "Create visualization"
  4. Drag "timestamp" field into workspace.
  5. Click "Save and return"
  6. Add filter
    {
      "error_query": {
        "indices": [
          {
            "error_type": "exception",
            "message": "local shard failure message 123",
            "name": "kibana_sample_data_logs"
          }
        ]
      }
    }
    
  7. Verify EsError and "View details" action are displayed
    Screenshot 2023-12-08 at 1 34 20 PM

Test - multiple configuration errors in lens editor

  1. install sample web logs
  2. create new lens visualization
  3. Drag "timestamp" field into workspace.
  4. Add filter
    {
      "error_query": {
        "indices": [
          {
            "error_type": "exception",
            "message": "local shard failure message 123",
            "name": "kibana_sample_data_logs"
          }
        ]
      }
    }
    
  5. Verify EsError and "View details" action are displayed
    Screenshot 2023-12-08 at 1 09 22 PM

Test - EsError in embeddable

  1. install sample web logs
  2. create new dashboard
  3. Click "Create visualization"
  4. Drag "timestamp" field into workspace.
  5. Change "Vertical axis" to "Cumulative sum". Select "Field" select and hit delete key
  6. Clone layer one or more times
  7. Verify pagination is displayed, allowing users to click through all configuration errors
    Screenshot 2023-12-08 at 12 59 18 PM

@nreese
Copy link
Contributor Author

nreese commented Dec 8, 2023

/ci

@nreese
Copy link
Contributor Author

nreese commented Dec 8, 2023

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Dec 8, 2023

/ci

@nreese
Copy link
Contributor Author

nreese commented Dec 8, 2023

/ci

@nreese
Copy link
Contributor Author

nreese commented Dec 10, 2023

/ci

@nreese
Copy link
Contributor Author

nreese commented Dec 10, 2023

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Dec 10, 2023

/ci

@nreese
Copy link
Contributor Author

nreese commented Dec 10, 2023

/ci

@nreese
Copy link
Contributor Author

nreese commented Dec 16, 2023

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

merge conflict between base and head

@nreese nreese requested review from a team as code owners December 17, 2023 15:18
@nreese
Copy link
Contributor Author

nreese commented Dec 17, 2023

@elasticmachine merge upstream

@botelastic botelastic bot added the Feature:Embedding Embedding content via iFrame label Dec 17, 2023
@nreese nreese added release_note:enhancement Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.13.0 and removed Feature:Embedding Embedding content via iFrame labels Dec 17, 2023
@elasticmachine
Copy link
Contributor

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

Copy link
Member

@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.

Presentation changes lgtm!

code review and tested error reporting

@botelastic botelastic bot added the Feature:Embedding Embedding content via iFrame label Dec 18, 2023
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.

Search changes LGTM

Copy link
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.

Works great, LGTM! Thanx Nathan!

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.

Did a code review and it looks 👌 (left only few minor comments).

On the UI side, wouldn't be better now to show the actual number of errors in the prompt title?

Screenshot 2023-12-20 at 10 22 30

Currently it is just a generic "An error":

Screenshot 2023-12-20 at 10 21 30

WDYT? cc @stratoula @MichaelMarcialis

@stratoula
Copy link
Contributor

@dej611 yes it is a nit but whatever Michael prefers here. The title in singular can improve and be more generic. I can see the number of errors from the paginated items.

@nreese
Copy link
Contributor Author

nreese commented Dec 20, 2023

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 1154 1155 +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
@kbn/search-errors 26 17 -9

Async chunks

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

id before after diff
discover 592.4KB 592.4KB -34.0B
lens 1.4MB 1.4MB -3.3KB
maps 2.9MB 2.9MB -41.0B
total -3.4KB

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
@kbn/search-errors 0 1 +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 410.6KB 410.6KB -23.0B
embeddable 78.2KB 78.1KB -44.0B
kbnUiSharedDeps-srcJs 2.3MB 2.3MB +747.0B
total +680.0B
Unknown metric groups

API count

id before after diff
@kbn/search-errors 27 18 -9

History

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

@nreese nreese requested a review from dej611 December 20, 2023 20:53
@nreese
Copy link
Contributor Author

nreese commented Dec 20, 2023

On the UI side, wouldn't be better now to show the actual number of errors in the prompt title?

I am going to merge this since I would like to build on these changes for Maps plugin. Copy updates can be made at a later time

@nreese nreese merged commit f2ad024 into elastic:main Dec 20, 2023
36 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Dec 20, 2023
@MichaelMarcialis
Copy link
Contributor

MichaelMarcialis commented Jan 2, 2024

On the UI side, wouldn't be better now to show the actual number of errors in the prompt title?

Currently it is just a generic "An error":

WDYT? cc @stratoula @MichaelMarcialis

@dej611, @stratoula, @nreese: Apologies for the late reply, as I was on PTO for the holidays. FWIW, I agree with Marco that including the error count in the title when multiple errors are in play would make for a nice improvement. I'm coming into this conversation a bit sideways, but here are some additional rapid-fire thoughts I had while looking at these screenshots. I'd be happy to discuss further or provide additional design support for these as needed.

  • If these are indeed errors that are preventing the visualization from being rendered (i.e. not warnings), we should use the error icon from EUI rather than the warning icon in these empty prompts.

  • The Dashboard app screenshot example above shows an empty prompt without a title. Is this intentional? Why do the other empty prompts have titles but this one doesn't?

  • Some of the error messages are housed in an EUI code block (monotype font, gray background, and copy button) while others are regular paragraph text. What is the reasoning for this? Should we be consistent with how we present these error messages (either all paragraphs or code blocks)?

  • Am I correct in assuming that the "View details" button would open the same inspect flyout that you and I have discussed in our previous work on remote cluster errors? If so, do we even need the error pagination shown in the last example screenshot above? Would it be better to take an approach that is consistent with our work on remote cluster errors and simply have a general error message and count, with the "View details" button allowing users to view all the error(s) details in the flyout? Here's one of the mockups of our work on multiple erroneous clusters empty prompt below:

    Multiple Clusters

@stratoula
Copy link
Contributor

stratoula commented Jan 3, 2024

I created a feature request here #174143. The new design seems much better @MichaelMarcialis I agree

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:Embedding Embedding content via iFrame release_note:enhancement Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[lens] 'View details' UI action to open clusters inspector tab when request fails
9 participants