elastic / kibana

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

[Event Annotations] individual annotation editing from library #158774

Closed drewdaemon closed 1 year ago

drewdaemon commented 1 year ago

Describe the feature: https://github.com/elastic/kibana/pull/157988 is adding a tab to the visualization library that allows searching and editing annotation groups.

The work for individual annotation editing is actually finished, but gated since the preview got descoped (it didn't make much sense for a user to edit an individual annotation without visual feedback).

We should add the preview from the original designs and reveal the individual annotation editing capabilities.

Screenshot 2023-07-20 at 2 45 58 PM

Technical overview

The preview should be a vertical bar chart with a count of records function. It should use the Lens embeddable to display the visualization.

~It will also be used for savable color palettes (described in https://github.com/elastic/kibana/issues/101942).~ Edit: not sure on this

Plan

Removing the circular dependency

We will split the current event_annotation plugin into two plugins, creating the event_annotation_application plugin which will import the Lens embeddable for the preview.

elasticmachine commented 1 year ago

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

drewdaemon commented 1 year ago

@MichaelMarcialis I understand that we'll be using a count of records over time visualization with whatever data view is attached to the particular annotation group.

A couple questions

MichaelMarcialis commented 1 year ago

Great questions, @drewdaemon . I'll attempt to provide some answers below:

What do we do if the data view doesn't have a default time field?

I seem to have overlooked this possibility when I suggested using a Discover-like document count histogram for the preview visualization. Would I be correct in assuming that it wouldn't be desirable to use the first available date field in those situations, as it would have the potential to yield unhelpful or unexpected preview visualizations?

Would it be better to take the approach that we are planning to use for the color mapping preview? There we are planning to simply display a preview visualization using dummy data for the purpose of allowing users to preview their colors. Could we take a similar approach here and just display a dummy time series visualization? Or would that be confusing for users (mixing of dummy and real data), as the annotations would be using real data from their data view? CCing @markov00 and @gvnmagni to get their thoughts.

How should the initial time range be determined?

  • For manual
  • For query-based

For the sake of performance and consistency with Lens, my initial thinking was that we could simply default to the "Last 15 minutes" date range (regardless if the annotation is a static date or custom query). In retrospect, the problem with this approach is that users may create one or more annotations that fall outside that default time window and become confused if they forget to change their preview date range.

I suppose one alternative would be to offer a new "Auto" date range option that attempts to best apply a time window that shows each annotation at least once. Thoughts? CCing @ninoslavmiskovic as well.

What should be the behavior if the data view is missing?

If the user has previously created an annotation group and the data view it used has since been removed, I imagine we should 1) show the data view selector in an erroneous state and also 2) show an empty prompt explaining the situation in the visualization preview area. Let me know if you want a quick wireframe of that, or if my description is enough to go by for now.

markov00 commented 1 year ago

Would it be better to take the approach that we are planning to use for the color mapping preview? There we are planning to simply display a preview visualization using dummy data for the purpose of allowing users to preview their colors. Could we take a similar approach here and just display a dummy time series visualization? Or would that be confusing for users (mixing of dummy and real data), as the annotations would be using real data from their data view? CCing @markov00 and @gvnmagni to get their thoughts.

I feel that a fake series is a good choice. To make it clear we should render it in a way that is clearly not real data but some sort of demo. For example a mix of these things could work:

gvnmagni commented 1 year ago

I totally agree, it should absolutely work fine!

stratoula commented 1 year ago

It is a bit difficult for me to follow how the preview will work with fake data. Let's say that I have a query annotation that queries all the destinations from Greece and the fake data don't have this field as the fake dataset is a sample dataset while our annotations are based on dataviews and depend on the users' real datasets. This means that this annotation layer is not going to be depicted in the preview chart.

markov00 commented 1 year ago

It is a bit difficult for me to follow how the preview will work with fake data. Let's say that I have a query annotation that queries all the destinations from Greece and the fake data don't have this field as the fake dataset is a sample dataset while our annotations are based on dataviews and depend on the users' real datasets. This means that this annotation layer is not going to be depicted in the preview chart.

@stratoula I'm not following you, let me try to understand better your example. What I'm getting back from a query to a "query-based annotation"? I suppose I'm getting some timestamps and some other associated values, correct? So now, without considering Lens and how it works, what is the problem of using this response in a simplified elastic-chart with a fake dataset and adding the annotations on top of it?

stratoula commented 1 year ago

Ok now I get your proposal. I thought that we are removing the dataview dependency which is important for having data. Which means that Drew's question on:

What should be the behavior if the data view is missing?

is valid.

What if the values we get are on a timespan that doesnt exist on our fake data? Or are we going to generate them automatically based on the timespan returned from the annotations query?

My point is as we need the dataview to fetch the results, which is the advantages of having fake data instead of a real data based on the dataview that the users have set for the annotations?

markov00 commented 1 year ago

What should be the behavior if the data view is missing?

If the data view is missing on a query-based annotation group, I believe we probably don't even need to display that group. The annotations are lost (because the dataview is no more available) so I don't see the need and the advantage to edit annotations or previewing them. Probably editing the dataview to match an existing one is the only thing to do here.

What if the values we get are on a timespan that doesnt exist on our fake data? Or are we going to generate them automatically based on the timespan returned from the annotations query?

We will build fake data based on the timestamp of the resulting annotations. We can do a multi-step passage where first we count how many annotations are within a bit "time span" and we then try to reduce it to show just a smaller set.

My point is as we need the dataview to fetch the results, which is the advantage of having fake data instead of a real data based on the dataview that the users have set for the annotations?

This is true for query-based annotations, not for static ones. Using real data could take more time to query then quickly see just the annotations + a client side generated array of numbers.

stratoula commented 1 year ago

This is true for query-based annotations, not for static ones. Using real data could take more time to query then quickly see just the annotations + a client side generated array of numbers.

I absolutely agree for the static ones, my concerns are mostly for the query based ones

stratoula commented 1 year ago

If the data view is missing on a query-based annotation group, I believe we probably don't even need to display that group. The annotations are lost (because the dataview is no more available) so I don't see the need and the advantage to edit annotations or previewing them. Probably editing the dataview to match an existing one is the only thing to do here.

+++ on this. We could possibly have an error/warning state and allow the users to change the dataview

MichaelMarcialis commented 1 year ago

Before we discuss this topic in our next visualization editor weekly sync, I'd like to attempt to summarize the above. Please let me know if this makes sense and if we all agree with the following (for the annotation creation, editing, and preview experience in the visualize library)? I can plan to provide design support as needed early next week.

Summary

Questions

stratoula commented 1 year ago

I think before we agree if we want fake data or real data we need to agree on which is the purpose of the preview. So for example if we only want to show to the users how the annotations are going to look does it make sense to also have fake annotations? If not, as I said above, I am not sure how query annotations based on real data and chart with fake data can easily work together. Also if we decide to go with fake data we need to use EC and not the Lens embeddable. (as Lens atm doesnt accept external data as an input). It is not necessary to use Lens but it will offer a lot of simplification during development.

About Michael's questions my personal opinion is:

Is it possible to repurpose aspects of this preview interface for the upcoming color mapping feature? It also offers a very similar visualization preview experience when creating/editing from the visualize library.

Yes we should, in my mind preview is a reusable component which should be easily reused everywhere we want to add a preview

Do we want to allow users to preview different chart types (bar, line, area), similar to how we are planning to do for the color mapping preview experience?

I think that this is a product question but there are no technical difficulties so I think we should

Do we want to allow users to preview across both light and dark mode, similar to how we are planning to do for the color mapping preview experience?

Same as above

Are we comfortable using the "Last 15 minutes" time range as the default for the preview? Or should we plan to explore a new "auto" time range option for these previews (as https://github.com/elastic/kibana/issues/158774#issuecomment-1629641006)?

This depends on the decision we are going to take about the fake vs real data.

Are we able to detect whether an annotation group is using a data view that no longer exists from the table view in the visualize library? If so, should we also show the annotation group as having errors from the table view as well (prior to users opening it in the flyout or Lens)?

Hmmm I am not sure about it, I think we can. If we can +1 from me, it will be a nice UX

drewdaemon commented 1 year ago

Are we able to detect whether an annotation group is using a data view that no longer exists from the table view in the visualize library? If so, should we also show the annotation group as having errors from the table view as well (prior to users opening it in the flyout or Lens)?

Hmmm I am not sure about it, I think we can. If we can +1 from me, it will be a nice UX

Yes, we can. We're already fetching the data view object to show the data view column on that view. We'll know if it's missing.

drewdaemon commented 1 year ago

Also, ditto to everything @stratoula said.

ninoslavmiskovic commented 1 year ago

My 2 cents on this.

First, it feels like a "preview" issue in general, so maybe we should upgrade the title :)

Second, great input from everybody.

Third, my comments :

To summarize: "Previews should be fast and simple and leave the user with an indication of "what is on the other side""

stratoula commented 1 year ago

Thanx @ninoslavmiskovic for sending us your thoughts. We have it on our weekly sync today so we can discuss it synchronously all together.

drewdaemon commented 1 year ago

To sum up from the meeting (feel free to correct/edit)

ninoslavmiskovic commented 1 year ago

@drewdaemon You captured it well 👌🏻 Small comment worth noting down: The reason why we moved away from "just a preview" is that the underlining work for making it a full editing experience is well on the way 😃 and it makes sense to have a full editing experience when we are enabling the full configuration options.

Also, I still think it makes sense to work on a "Preview" when the user hovers over Saved Object items on the list view.

++ We should consider the balance between "full editing experience" vs "preview for saved color palettes work and other editing experiences on the list view..

drewdaemon commented 1 year ago

Also, I still think it makes sense to work on a "Preview" when the user hovers over Saved Object items on the list view.

Yeah, I like this idea. Here's an issue.

MichaelMarcialis commented 1 year ago

Based on our most recent discussions in the weekly sync regarding this issue, I've put together some quick updated wireframes for the annotation preview interface. I've shared these with @drewdaemon and he's currently investigating whether these suggestions will work or if additional design support is needed.

drewdaemon commented 1 year ago

Just realized that we never resolved this

How should the initial time range be determined?

  • For manual
  • For query-based

I suppose one alternative would be to offer a new "Auto" date range option that attempts to best apply a time window that shows each annotation at least once. Thoughts? CCing @ninoslavmiskovic as well.

I think this would be straightforward with manual annotations. For query-based annotations, the only way I think this could work would be with a request to Elasticsearch to fetch the timestamps for the annotations themselves.

My suggestion is to move forward with the last 15 minutes as the default time range and consider this as an optional follow-up. Any objections?