elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.71k stars 8.12k forks source link

[Lens][Embeddable] It renders two error sections unexpectedly from Lens Embeddable #156811

Open angorayc opened 1 year ago

angorayc commented 1 year ago

Original issue: https://github.com/elastic/kibana/issues/156271#issuecomment-1535996267

Seems that it renders two error sections when using Lens Embeddable.

I intentionally indexed some documents with invalid field type so I can see the error view from Lens Embeddable.

DELETE auditbeat-custom-index-1

PUT auditbeat-custom-index-1

PUT auditbeat-custom-index-1/_mapping
{
  "properties": {
    "@timestamp": {
      "type": "date"
    },
    "event.category": {
      "type": "keyword",
      "ignore_above": 1024
    }
  }
}

POST auditbeat-custom-index-1/_doc
{
  "@timestamp": "2023-05-05T09:41:49.668Z",
  "host": {
    "name": "foo"
  },
  "event": {
    "category": "an_invalid_category"
  },
  "some.field": "this",
  "source": {
    "port": 90210,
    "ip": "10.1.2.3"
  }
}

POST auditbeat-custom-index-1/_doc
{
  "@timestamp": "2023-05-05T09:42:22.123Z",
  "host": {
    "name": "bar"
  },
  "event": {
    "category": "an_invalid_category"
  },
  "some.field": "space",
  "source": {
    "port": 867,
    "ip": "10.9.8.7"
  }
}

POST auditbeat-custom-index-1/_doc
{
  "@timestamp": "2023-05-05T09:43:35.456Z",
  "host": {
    "name": "baz"
  },
  "event": {
    "category": "theory"
  },
  "some.field": "for",
  "source": {
    "port": 5,
    "ip": "10.4.6.6"
  }
}

POST auditbeat-custom-index-1/_doc
{
  "@timestamp": "2023-05-05T09:44:36.700Z",
  "host": {
    "name": "@baz"
  },
  "event": {
    "category": "malware"
  },
  "some.field": "rent",
  "source": {
    "port": 309,
    "ip": "10.1.1.1"
  }
}

Observation: The error message is incomplete. It does not display the full error description. (But I'd expect to see the full error description.)

unique_ips_layer_1_error

https://user-images.githubusercontent.com/6295984/236424101-d9677ba9-170d-4a52-9af5-ce73bb9fa847.mov

After inspecting the elements, it seems that Lens Embeddable renders two error sections here. The first one with incomplete message, and the second one with complete message.

Screenshot 2023-05-05 at 10 34 45

https://user-images.githubusercontent.com/6295984/236424043-e7157042-7c13-47b5-b77f-89b23b641cf1.mov

But it displays correctly in Lens

Screenshot 2023-05-05 at 10 45 32

Originally posted by @angorayc in https://github.com/elastic/kibana/issues/156271#issuecomment-1535996267

elasticmachine commented 1 year ago

Pinging @elastic/security-solution (Team: SecuritySolution)

elasticmachine commented 1 year ago

Pinging @elastic/kibana-visualizations @elastic/kibana-visualizations-external (Team:Visualizations)

stratoula commented 1 year ago

cc @drewdaemon I think we need to display all the errors? Can you take a look? Thanx

drewdaemon commented 1 year ago

Why this is happening

I looked into this and realized that we're dealing with two lasagna layers of architecture, each with its own error handling system...

During my efforts to improve the error handling and display in the Lens layer with the new UserMessages architecture, I failed to realize that the EmbeddablePanel imposes its own error display. When our code updates the embeddable output to report an error state, it slaps its own UI on top of ours.

This is why, as @angorayc noted, two DOM trees are present. The hidden one displaying the correct message is our own VisualizationErrorPanel, while the visible one with the incorrect message is the embeddable panel's stuff.

I think that the reason I didn't notice that this was happening before is that the embeddable panel's error UI looks similar to ours and it doesn't interfere with the (absolutely-positioned) non-blocking message list introduced in https://github.com/elastic/kibana/pull/147818.

The final question is: if we are updating the embeddable output with the same error message as the one we are rendering in the Lens layer, why is the message displayed by the embeddable panel different/incomplete?

The answer is that in the Lens layer we support displaying any React node, whereas the embeddable panel only supports strings. So, in this case, we're forced to use the short message on that UserMessage. Unfortunately, it looks like that shortMessage is for some reason incomplete. Sort of a hidden bug in the form-based datasource code in Lens.

Options

  1. Don't report errors in our embeddable output (not sure what the other ramifications of this would be)
  2. Introduce a way to tell the embeddable panel not to impose its own error display UI
  3. Accept the limitations of the embeddable panel's API and remove Lens's custom VisualizationErrorPanel. Fix the issue with that UserMessage's shortMessage so that a full error is displayed. Lose some capabilities and flexibility introduced with UserMessages.
  4. Extend the embeddable panel's API to match the current capabilities of Lens's UserMessage system and remove Lens's custom VisualizationErrorPanel.
  5. Remove the embeddable panel's error display capabilities entirely, putting the responsibility of rendering errors on the embeddables. Could export a set of shared components to promote consistency.

Option 2 is easiest and not as hacky as 1, but I also see arguments for trying to make embeddable error handling consistent via a unified API (this was attempted earlier in https://github.com/elastic/kibana/pull/143367). Thoughts @stratoula @dej611 @ThomThomson ?

drewdaemon commented 1 year ago

Another thing to think about is that right now the Lens embeddable only "counts" errors in the embeddable output if they are blocking. If they aren't blocking, we don't report them so that the EmbeddablePanel doesn't impose its error UI. So, we're already inconsistent.

We show non-blocking errors and other messages like this:

Screen Shot 2023-01-27 at 9 26 11 AM

I guess if we chose to invest in option 4 (or 5), we could also move this ^^ to the embeddable level and other embeddables could make use of it for free (e.g. Vega).

stratoula commented 1 year ago

@drewdaemon thanx for the investigation! The second appears to be indeed the fastest solution but I really love the 4 and 5. I tend more to the 5th one but both of them seem as the correct way to go. I would love @ThomThomson 's input on that!

dej611 commented 1 year ago

I would vote for 2 and 5: basically moving the responsability of the error visualization on the renderer size, with a general fallback for those who do not register any error handling.

ThomThomson commented 1 year ago

I do very much like Option 5 here. In my opinion the Embeddable Error UI is for embeddable specific and completely unrecoverable errors... (could not locate that embeddable factory, an error was thrown in the embeddable creation process etc).

I believe it should be possible to expose and render errors without putting them into the embeddable output stream - that way it would only be a concern of the individual Embeddable's implementation.

drewdaemon commented 1 year ago

Okay. To resolve this bug I will pursue strategy 1, removing these errors from the Lens embeddable's output stream. I have created https://github.com/elastic/kibana/issues/157894 to track progress toward solution 5.

markov00 commented 3 months ago

blocked by the new embeddable system refactoring https://github.com/elastic/kibana/issues/167429