Full Annotation Framework #3518

fabianmenges · 2017-09-23T17:34:32Z

This PR contains a full Annotation Framework

There are 5 different types of annotations:

Superset Annotations. The annotations introduced in Time Series Annotation Layers #3521
Event Annotations. Events are a list of dates with descriptions. They will be displayed as vertical lines. Their description will show up in the tooltips if you hover over them. They are fetched from a "table" slice, that contains the data. You can pick the time and description columns.
Interval Annotations. Just like Events but having a start and end date, they are rendered as a range.
Time Series Annotations. Timeseries are are added as an additional line on the line chart. The name and the display properties can be configured. The data is fetched from any timeseries slice.
Formulas Annotations. Formulas are added as an additional line to the chart. You will be able to enter a mathematical expression, that will be evaluated on the client side using "mathjs". You will be able to specify display properties. You will be able to specify any mathematical formula e.g

y = 500 (horizontal line)
y = 0.2x (line with inclination of 0.2)
y = x^2 (parabola)

Check out the new demo video:
https://www.youtube.com/watch?v=-0CgA3q39KI&feature=youtu.be

Couple screenshots:

Superset Annotations Config:

Configuring Interval Annotation

Dashboard with annotations:

Context Aware tooltips:

Error handling:

Loading indicators for annotations:

coveralls · 2017-09-23T17:46:26Z

Coverage decreased (-0.07%) to 69.467% when pulling 9a252c153f4d046ceb0c5a9f46f3cb9e622b42a1 on tc-dc:fmenges/annotations into 255ea69 on apache:master.

fabianmenges · 2017-09-23T18:21:41Z

Maybe it makes sense to rename all of this from: annotations => overlays

coveralls · 2017-09-23T18:56:25Z

Coverage decreased (-0.07%) to 69.467% when pulling 44e3444b61d0b960a9348a9ec0f27033c5a39b8a on tc-dc:fmenges/annotations into 255ea69 on apache:master.

mistercrunch · 2017-09-26T03:36:30Z

Nice, so these annotations would be stored inside the slice's json?

We'll need to make things clear and cohesive here between the server-side time "Annotations Layers" #3521 and the other types of markers or annotations that would live on the slice itself. I'll try to get Eli (our designer) to chime in here.

fabianmenges · 2017-09-26T13:01:53Z

No, it handles both. Formula overlays are client side (I would have had to do too much plumbing work for server side, b.c. they probably should be datasources then).

The slice annotations are server side, but read only. Basically it assumes you have a table with annotations in SQL (e.g. your deploy history from jenkins, teamcity, ....). You can now create a regular table slice in Superset with (timestamp and event) and then have these superimposed on your line chart as horizontal lines.

coveralls · 2017-09-26T16:19:52Z

Coverage decreased (-0.2%) to 69.327% when pulling 65040f91c0556e908a5e1bab80ea35e9c1c1c4c8 on tc-dc:fmenges/annotations into 255ea69 on apache:master.

coveralls · 2017-09-26T16:19:52Z

Coverage decreased (-0.2%) to 69.327% when pulling 65040f91c0556e908a5e1bab80ea35e9c1c1c4c8 on tc-dc:fmenges/annotations into 255ea69 on apache:master.

coveralls · 2017-09-26T16:19:52Z

Coverage decreased (-0.2%) to 69.327% when pulling 65040f91c0556e908a5e1bab80ea35e9c1c1c4c8 on tc-dc:fmenges/annotations into 255ea69 on apache:master.

mistercrunch · 2017-09-27T16:40:34Z

Bringing some of the comments from my last comment here #3521

using table viz slices as input for annotation is fragile, I'd like to see a new time_annotations viz which would be similar to table viz, but enforce having a time column, clearly point to columns for short and long descriptions, perhaps even a onClick link (optional) that defines where to go when click on the annotation. That viz could also look good on its own, as well as be used as input for other slices
can we break down the data driven annotations to a different PR?
I'm all for formula, vertical, and point markers, though I think we need a bit of a plan as to how all of these annotations come together. The Popover you have here is a very good starting point for this. I'd like to see the Popover have a dropdown for the type marker first, and then alter the form to only show the relevant options in that context.
for the data-driven annotations, there are considerations around whether they are fetched independently asynchronously or whether they become part of the existing data payload. If async, things get more complex on the visualization interface, having to deal with promise and such. Sounds like in many cases we can't render the markers until the actual data has landed.
to make all of this comprehensive, we'll need a s-ton of tooltips and some nice docs, this can come later as we settle on the design

@williaster had some thoughts and said something about providing mocks for the "Annotations and Markers" control section and popover

mistercrunch · 2017-09-27T16:43:16Z

BTW, I'm excited we got the ball rolling on all this and grateful for all the work that you're doing @fabianmenges. This is HUGE for Superset.

fabianmenges · 2017-09-27T17:58:00Z

@mistercrunch

using table viz slices as input for annotation is fragile

I'm using 'table viz' for horizontal lines and intervals,
I'm using 'line viz' for point markers and line overlays,
You will be able to pick your column for title and description in the 'Annotation overlay' configuration. Things like onclick should be handled in a separate PR, you would probably need to support some templating for this anyways.
If you still think we need a special slice for it, feel free to create one, I'll hook it up.

can we break down the data driven annotations to a different PR? ...

All the annotations types but Formula are data driven. Are you asking me to support only Formulas for now? This would result in deleting a whole bunch of code and make some of the architecture look pretty useless.

I'm all for formula, vertical, and point markers, though I think we need a bit of a plan as to how all of these annotations come together...

I'm making the popover dynamic for each Annotation type right now.
It already is, if you consider that when its a Formula you can enter the formula, but when it is any other type it is a "Slice Selector." For the slice selector you will be able to pass down 'Since' and 'Until', there is even more things we could pass down, e.g. (Filters, Mapping)
This is also where you could specify a 'Tilte' and 'Description' and 'Link' coloumn for a dropdown of a list of all columns, but I probably won't be able to get that stuff into this PR.

for the data-driven annotations, there are considerations around whether they are fetched...

Fetching them independently, asynchronously and on the client has many advantages.

We can fetch all annotation layers in parallel (async), I did not see anything in your current python project configuration that would allow me to do this in a reasonable way in python, (e.g. no gevent, etc). I feel very strongly about not doing this synchronously.
Separation of concerns: Your charts will still load fast even if there is something wrong with your annotations that might be stored on a different system. Even if we would implement this in python asynchronously we would still have to wait for the timeout of each annotation layer before we could return data.
If AnnotationLayers are hidden and you 'unhide' them you only have to load the missing data and not all the data

to make all of this comprehensive, we'll need a s-ton of tooltips

I can take care of this. Would like to get that into a 2nd PR.

This will get deployed to our production environment by Friday. It would be great if we could nail out the architecture questions beforehand. I hate writing JSON migration scripts.

fabianmenges · 2017-09-27T23:08:59Z

BTW, this library http://d3-annotation.susielu.com/ is only available for d3-v4, going to punt on point annotations for now

fabianmenges

Left a couple comments in the code

fabianmenges · 2017-09-29T15:37:25Z

superset/assets/javascripts/explore/actions/chartActions.js

-      error(err) {
+      timeout: timeout * 1000,
+    });
+    const queryPromise = queryRequest


I changes this to use promises to make everything composeable. I tested this a lot....
https://github.com/gaearon/redux-thunk#composition

fabianmenges · 2017-09-29T15:40:26Z

superset/assets/javascripts/modules/superset.js

              this.done(queryResponse);
+              // render when all the annotations are available


Currently the chart is only drawn on the dashboard when all annotations have been loaded. In a future PR I want display the chart as soon as its data becomes available and add an indicator that shows if not all annotations have been fetched and re-renders when all are available.

fabianmenges · 2017-09-29T15:41:00Z

superset/assets/spec/javascripts/explore/chartActions_spec.js

@@ -23,14 +23,16 @@ describe('chart actions', () => {
  });

  it('should handle query timeout', () => {
-    ajaxStub.yieldsTo('error', { statusText: 'timeout' });


Changing the test to work with async promises.

coveralls · 2017-09-29T15:48:39Z

Coverage decreased (-0.3%) to 69.905% when pulling 296b9c1275d091283c92476b8170008ccb5a60bd on tc-dc:fmenges/annotations into 03e2af8 on apache:master.

coveralls · 2017-09-29T15:55:05Z

Coverage decreased (-0.3%) to 69.905% when pulling 296b9c1275d091283c92476b8170008ccb5a60bd on tc-dc:fmenges/annotations into 03e2af8 on apache:master.

fabianmenges · 2017-09-29T16:11:56Z

@mistercrunch I addressed the issues you've brought up:

integrated the annotations from Time Series Annotation Layers #3521
columns for time, title and description are configurable are not hard coded, everything is configureable
tooltips and validation everywhere

I'm still fetching everything asynchronously but I've done extensive testing and error handling around this. I think the state is stable and polished enough for you to check it out...

coveralls · 2017-09-29T16:58:48Z

Coverage decreased (-0.3%) to 69.905% when pulling dc204e01429f933a33f2f39dc6709e3d6a92d37e on tc-dc:fmenges/annotations into 03e2af8 on apache:master.

coveralls · 2017-09-29T19:51:26Z

Coverage decreased (-0.1%) to 70.049% when pulling 5b23d7426cd06b81cf42879ed8cd40c551c4a127 on tc-dc:fmenges/annotations into 03e2af8 on apache:master.

fabianmenges · 2017-10-02T19:13:28Z

BTW, if you want to go forward with this, I will need to write a migration script or some other logic to handle the current annotation_layer formData variable.

coveralls · 2017-10-03T15:39:34Z

Coverage decreased (-0.1%) to 70.033% when pulling 70507486e423581ba15e8e6d5e12c602b9f7263a on tc-dc:fmenges/annotations into 18e459e on apache:master.

coveralls · 2017-10-03T15:40:33Z

Coverage decreased (-0.1%) to 70.033% when pulling 70507486e423581ba15e8e6d5e12c602b9f7263a on tc-dc:fmenges/annotations into 18e459e on apache:master.

tklaudel · 2017-10-26T08:49:55Z

@fabianmenges this looks great, any plans when it will be ready?

fabianmenges force-pushed the fmenges/annotations branch from 9a252c1 to 44e3444 Compare September 23, 2017 18:41

fabianmenges mentioned this pull request Sep 26, 2017

Time Series Annotation Layers #3521

Merged

fabianmenges force-pushed the fmenges/annotations branch 2 times, most recently from 907fdfe to 296b9c1 Compare September 29, 2017 15:34

fabianmenges changed the title ~~WIP Annotations~~ Full Annotation Framework Sep 29, 2017

fabianmenges commented Sep 29, 2017

View reviewed changes

fabianmenges force-pushed the fmenges/annotations branch from dc204e0 to 5b23d74 Compare September 29, 2017 19:37

fabianmenges force-pushed the fmenges/annotations branch 2 times, most recently from 26a7e9a to 7050748 Compare October 3, 2017 15:24

This was referenced Dec 27, 2023

[Snyk] Fix for 12 vulnerabilities ShareChat/superset#101

Open

[Snyk] Fix for 12 vulnerabilities ShareChat/superset#103

Open

[Snyk] Fix for 12 vulnerabilities ShareChat/superset#104

Open

This was referenced Jan 12, 2024

[Snyk] Fix for 13 vulnerabilities ShareChat/superset#122

Open

[Snyk] Fix for 13 vulnerabilities ShareChat/superset#123

Open

[Snyk] Fix for 13 vulnerabilities ShareChat/superset#125

Open

[Snyk] Fix for 13 vulnerabilities ShareChat/superset#126

Open

This was referenced Feb 7, 2024

[Snyk] Fix for 8 vulnerabilities ShareChat/superset#148

Open

[Snyk] Fix for 8 vulnerabilities ShareChat/superset#149

Open

m-ajay mentioned this pull request Feb 12, 2024

[Snyk] Fix for 1 vulnerabilities m-ajay/superset#263

Open

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.23.0 labels Feb 27, 2024

m-ajay mentioned this pull request Mar 22, 2024

[Snyk] Fix for 2 vulnerabilities m-ajay/superset#278

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Full Annotation Framework #3518

Full Annotation Framework #3518

fabianmenges commented Sep 23, 2017 •

edited

coveralls commented Sep 23, 2017 •

edited

fabianmenges commented Sep 23, 2017

coveralls commented Sep 23, 2017 •

edited

mistercrunch commented Sep 26, 2017

fabianmenges commented Sep 26, 2017 •

edited

coveralls commented Sep 26, 2017

coveralls commented Sep 26, 2017

coveralls commented Sep 26, 2017

mistercrunch commented Sep 27, 2017 •

edited

mistercrunch commented Sep 27, 2017

fabianmenges commented Sep 27, 2017

fabianmenges commented Sep 27, 2017 •

edited

fabianmenges left a comment

fabianmenges Sep 29, 2017

fabianmenges Sep 29, 2017

fabianmenges Sep 29, 2017

coveralls commented Sep 29, 2017 •

edited

coveralls commented Sep 29, 2017 •

edited

fabianmenges commented Sep 29, 2017

coveralls commented Sep 29, 2017 •

edited

coveralls commented Sep 29, 2017 •

edited

fabianmenges commented Oct 2, 2017

coveralls commented Oct 3, 2017 •

edited

coveralls commented Oct 3, 2017 •

edited

tklaudel commented Oct 26, 2017

		this.done(queryResponse);
		// render when all the annotations are available

Full Annotation Framework #3518

Full Annotation Framework #3518

Conversation

fabianmenges commented Sep 23, 2017 • edited

coveralls commented Sep 23, 2017 • edited

fabianmenges commented Sep 23, 2017

coveralls commented Sep 23, 2017 • edited

mistercrunch commented Sep 26, 2017

fabianmenges commented Sep 26, 2017 • edited

coveralls commented Sep 26, 2017

coveralls commented Sep 26, 2017

coveralls commented Sep 26, 2017

mistercrunch commented Sep 27, 2017 • edited

mistercrunch commented Sep 27, 2017

fabianmenges commented Sep 27, 2017

fabianmenges commented Sep 27, 2017 • edited

fabianmenges left a comment

Choose a reason for hiding this comment

fabianmenges Sep 29, 2017

Choose a reason for hiding this comment

fabianmenges Sep 29, 2017

Choose a reason for hiding this comment

fabianmenges Sep 29, 2017

Choose a reason for hiding this comment

coveralls commented Sep 29, 2017 • edited

coveralls commented Sep 29, 2017 • edited

fabianmenges commented Sep 29, 2017

coveralls commented Sep 29, 2017 • edited

coveralls commented Sep 29, 2017 • edited

fabianmenges commented Oct 2, 2017

coveralls commented Oct 3, 2017 • edited

coveralls commented Oct 3, 2017 • edited

tklaudel commented Oct 26, 2017

fabianmenges commented Sep 23, 2017 •

edited

coveralls commented Sep 23, 2017 •

edited

coveralls commented Sep 23, 2017 •

edited

fabianmenges commented Sep 26, 2017 •

edited

mistercrunch commented Sep 27, 2017 •

edited

fabianmenges commented Sep 27, 2017 •

edited

coveralls commented Sep 29, 2017 •

edited

coveralls commented Sep 29, 2017 •

edited

coveralls commented Sep 29, 2017 •

edited

coveralls commented Sep 29, 2017 •

edited

coveralls commented Oct 3, 2017 •

edited

coveralls commented Oct 3, 2017 •

edited