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

[Event Annotations] individual annotation editing from library #158774

Closed
drewdaemon opened this issue May 31, 2023 · 22 comments · Fixed by #159692 or #163346
Closed

[Event Annotations] individual annotation editing from library #158774

drewdaemon opened this issue May 31, 2023 · 22 comments · Fixed by #159692 or #163346
Assignees
Labels
enhancement New value added to drive a business result Feature:Lens impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:needs-research This issue requires some research before it can be worked on or estimated Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@drewdaemon
Copy link
Contributor

drewdaemon commented May 31, 2023

Describe the feature:
#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 #101942). Edit: not sure on this

Plan

  • Get preview visualization to render—no annotations
  • Wire up time picker (unified search)
  • Extract chart picker from Lens and add to preview
  • Show annotations in preview (includes deriving initial time range)
  • Support light and dark mode—needs more research
  • Add time field picker
  • Add functional tests
  • Evaluate layout responsiveness—possibly spin off an enhancement issue

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.

@drewdaemon drewdaemon added loe:large Large Level of Effort Team:Visualizations Visualization editors, elastic-charts and infrastructure impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. labels May 31, 2023
@elasticmachine
Copy link
Contributor

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

@stratoula stratoula added impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Feature:Lens enhancement New value added to drive a business result and removed impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. labels Jun 1, 2023
@drewdaemon drewdaemon added loe:needs-research This issue requires some research before it can be worked on or estimated and removed loe:large Large Level of Effort labels Jun 2, 2023
@stratoula stratoula reopened this Jun 19, 2023
@drewdaemon
Copy link
Contributor Author

@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

  • What do we do if the data view doesn't have a default time field?
  • How should the initial time range be determined?
    • For manual
    • For query-based
  • What should be the behavior if the data view is missing?

@MichaelMarcialis
Copy link
Contributor

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

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:

  • use a sine/cosine trend in the fake data, precise sine is less common in data (even if can be related to week days and weekends) but feels safe to me
  • give the trend (probably an area chart) a shade of gray, and make it similar to a placeholder style
  • add an overlay at the bottom of the graph saying "randomly generated data"

@gvnmagni
Copy link

I totally agree, it should absolutely work fine!

@stratoula
Copy link
Contributor

stratoula commented Jul 11, 2023

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

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

stratoula commented Jul 12, 2023

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

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

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

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

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

  • Utilize fake data to generate a time series visualization in the preview panel (making it clear to users that the visualization is not based on real data).
  • Preview annotations are overlaid on the fake time series visualization using either:
    1. Static date.
    2. Custom query using real data (via selected data view).
  • If an annotation group is using a data view that no longer exists, we can:
    • Show the data view selector in an erroneous state.
    • Any annotation in the group using a custom query will not be rendered (as there will be no known data).
    • Any annotation in the group using a static date can continue to be rendered over the fake time series visualization.

Questions

  • 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.
  • 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?
  • 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?
  • 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 explained above)?
  • 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)?

@stratoula
Copy link
Contributor

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 #158774 (comment))?

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Also, ditto to everything @stratoula said.

@ninoslavmiskovic
Copy link
Contributor

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 :

  • Preview as a feature has one purpose and that is quickly show a preview to the user otherwise it is competing with the actual content (in our case a chart, annotation etc.), so the speed is very important otherwise the user might as well go to the editor and see how it looks like. I give a big ++ on using dummy data because we can better control the outcome of the preview and it will be much faster.
  • If we are adding a preview on annotation we should definitely add it to charts and everything worth a preview on the list.
  • It could be nice to have the preview as hover on the list (See google Docs example) :
    Skærmbillede 2023-07-17 kl  15 37 29
  • For annotations preview, I think you nailed it @stratoula that the purpose is to showcase how they are going to look otherwise the preview logic gets unnecessarily complex.

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

@stratoula
Copy link
Contributor

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

@drewdaemon
Copy link
Contributor Author

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

  • We are accepting that this is a full annotation editing experience instead of just a "Preview" in the sense that @ninoslavmiskovic described
    • Therefore, we are planning to use the Lens embeddable with real data (count over time)
    • It is not yet clear whether similar technical approach makes sense in the case of the saved palette work
  • If the data view doesn't have a default time field, we will provide users with the option to select one to be used in the flyout visualization
  • @MichaelMarcialis will create an enhancement issue (including wireframes) to let users select any of the visualizations that use a particular annotation group as the flyout visualization

@ninoslavmiskovic
Copy link
Contributor

ninoslavmiskovic commented Jul 17, 2023

@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
Copy link
Contributor Author

drewdaemon commented Jul 17, 2023

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

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
Copy link
Contributor Author

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?

stratoula added a commit that referenced this issue Sep 20, 2023
## Summary

Resolve #158774
Part of #159053

<img width="1920" alt="Screenshot 2023-09-13 at 2 00 25 PM"
src="https://github.com/elastic/kibana/assets/315764/69cfe07e-d442-462b-91c5-395d6040c383">

<img width="1920" alt="Screenshot 2023-09-13 at 2 00 09 PM"
src="https://github.com/elastic/kibana/assets/315764/260aedbe-31d0-415a-b387-10a9b13bf9a6">

<img width="1920" alt="Screenshot 2023-09-13 at 2 01 07 PM"
src="https://github.com/elastic/kibana/assets/315764/9672010b-d49b-4041-acf1-33d3baec1e9a">


### Known issues
- [ ] ~Responsive layout~ **Proposal:** don't optimize for mobile
- [x] Recovering embeddable from problematic data view state
- [x] margin around dimension buttons
- [x] Functional test coverage

### Checklist

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [x]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [ ] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [ ] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Stratoula Kalafateli <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Feature:Lens impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:needs-research This issue requires some research before it can be worked on or estimated Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
7 participants