Time Series Annotation Layers #3521

graceguo-supercat · 2017-09-25T03:45:46Z

coveralls · 2017-09-25T04:03:13Z

Coverage increased (+0.2%) to 69.706% when pulling cc29e51 on graceguo-supercat:annotations into 255ea69 on apache:master.

mistercrunch

A few minor things, looks great!

mistercrunch · 2017-09-25T18:13:21Z

superset/assets/javascripts/explore/components/controls/SelectAsyncControl.jsx

+class SelectAsyncControl extends React.PureComponent {
+  constructor(props) {
+    super(props);
+    this.state = {


Many of the other controls to not maintain internal state and rely solely on the value prop and onChange to act as a controlled component. There's nothing bad with maintaining the state here but it adds a bid of complexity that is not needed.

mistercrunch · 2017-09-25T18:17:12Z

superset/assets/visualizations/nvd3_vis.js

+          .attr('height', 10)
+          .attr('patternTransform', 'rotate(45 50 50)')
+          .append('line')
+          .attr('stroke', '#00A699')


Looked like this color is @brand-primary, any way we can use a CSS class instead of hard-coding the color here?

mistercrunch · 2017-09-25T18:17:51Z

superset/assets/visualizations/nvd3_vis.js

+          .attr('fill', 'url(#diagonal)')
+          .attr('fill-opacity', 0.1)
+          .attr('stroke-width', 1)
+          .attr('stroke', '#00A699')


same here, let's not hard code the color

mistercrunch · 2017-09-25T18:19:13Z

superset/migrations/versions/f959a6652acd_.py

@@ -0,0 +1,22 @@
+"""empty message


I'm surprised to see 2 different merge migrations. Legit or mistake?

mistercrunch · 2017-09-25T18:20:39Z

superset/models/core.py

@@ -25,7 +25,7 @@

 from sqlalchemy import (
    Column, Integer, String, ForeignKey, Text, Boolean,
-    DateTime, Date, Table,
+    DateTime, Date, Table, Index,


Index is unused, I think I left that dirt behind in my commit...

mistercrunch · 2017-09-25T18:21:15Z

superset/viz.py

@@ -111,6 +112,7 @@ def get_extra_filters(self):

    def query_obj(self):
        """Building a query object"""
+        print("running query_obj============================")


Let's remove this line

mistercrunch · 2017-09-25T18:23:38Z

superset/assets/visualizations/nvd3_vis.js

+          .attr('class', 'd3-tip')
+          .direction('n')
+          .offset([-5, 0])
+          .html(d => (d && d.layer ? d.layer : ''));


I think we should surface more here, the layer name, the short and the long description.

mistercrunch · 2017-09-25T18:28:59Z

superset/assets/visualizations/nvd3_vis.js

+          })
+          .attr('height', hh)
+          .attr('fill', 'url(#diagonal)')
+          .attr('fill-opacity', 0.1)


note: I think the annotations look good on the gif, we can talk to Eli & Chris to fine tune their look if needed.

class could be called stroke-primary

coveralls · 2017-09-26T00:02:49Z

Coverage increased (+0.06%) to 69.615% when pulling 13ace05775b1313e150a49092a867fab06bd994b on graceguo-supercat:annotations into 3949d39 on apache:master.

coveralls · 2017-09-26T00:02:49Z

Coverage increased (+0.06%) to 69.615% when pulling 13ace05775b1313e150a49092a867fab06bd994b on graceguo-supercat:annotations into 3949d39 on apache:master.

coveralls · 2017-09-26T00:09:36Z

Coverage increased (+0.06%) to 69.615% when pulling 13ace05775b1313e150a49092a867fab06bd994b on graceguo-supercat:annotations into 3949d39 on apache:master.

- add annotation input sanity check before add and before update. - make SelectAsyncControl component statelesis, and generic - add annotation description in d3 tool tip - use less variable to replace hard-coded color

coveralls · 2017-09-26T00:24:11Z

Coverage increased (+0.06%) to 69.609% when pulling 1313151 on graceguo-supercat:annotations into 3949d39 on apache:master.

fabianmenges · 2017-09-26T13:10:24Z

superset/viz.py

@@ -57,6 +57,7 @@ def __init__(self, datasource, form_data):
            'token', 'token_' + uuid.uuid4().hex[:8])
        self.metrics = self.form_data.get('metrics') or []
        self.groupby = self.form_data.get('groupby') or []
+        self.annotation_layers = []


I don't think annotations should be part the viz object. For most viz types that I can think of annotations don't really make sense. If you make annotations accessible from a json endpoint they can be loaded client side. This would also make the responsiveness better, annotations could be loaded async independently of the data.

Here we decided to package the annotations from the backend along with the data payload. The visualizations' render method can then assume that all of the data + annotations + context is present when rendering. Note that the annotation query is fully indexed and should take milliseconds assuming a small-ish number of annotations for the time range.

I feel like if we wanted to go async on the annotations, we'd need to change the visualization's API to define 2 different methods and somehow guarantee to call the annotation one second. It makes things much more complicated on that side of things.

34ec8b3#diff-a85def8afdc240ac598637a26a7cdbbc I can probably clean this up a bit more, but this makes all http requests in parallel.

fabianmenges · 2017-09-26T13:12:25Z

superset/views/annotations.py

+from .base import SupersetModelView, DeleteMixin
+
+
+class AnnotationModelView(SupersetModelView, DeleteMixin):  # noqa


How are annotations written to that table? If you cannot write them from within superset, why are you not thinking of them as just another datasource?

This feature ships with the FAB CRUD UI that allows users to manually input/manage layers. FAB also exposes a CRUD REST API to maintain those programmatically, though I'm thinking at Airbnb most likely people will write Airflow jobs talking to our MySQL instance to load annotations dynamically.

I don't think that this table should be part of the Superset DB then. The way Superset is currently designed is that it is agnostic (as long as there is a Druid/SQL connector) to where the data is stored. We have a clean separation of meta data (slice definition, configuration, users, ...) and the data that superset is operating on.
This table inverses this fundamentally and now we have to store presentation data directly inside of the superset db. Currently no time series data is stored inside of the superset database. I'm also worried that this will cause scalability issues. Now my superset meta database is going to have very different performance requirements with regard to querying time series.

If this table was only used for manually created data from a user interface you could make the argument that we are talking about a very limited amount of data which originated in superset and therefore should be stored there. But this seems not to be the case.

Yeah it's at the frontier of data/metadata, though to me annotations are closer to metadata.

Though I agree that it would be sweet to support external data for annotations (as in your PR), let's do both. I have some thoughts on that that I'll put on your PR. The TLDR is I think we should create a new time_annotations viz_type to enforce a certain set of guaranties, as opposed to a convention on top of the table viz_type.

fabianmenges · 2017-09-26T14:10:05Z

superset/migrations/versions/ddd6ebdd853b_annotations.py

+        'annotation',
+        sa.Column('created_on', sa.DateTime(), nullable=True),
+        sa.Column('changed_on', sa.DateTime(), nullable=True),
+        sa.Column('id', sa.Integer(), nullable=False),


hoping we never go above 2,147,483,647 annotations, saving some bytes

If you have automated processes writing to this table (airflow) we should change this to BIGINT. The disk space you save is only relevant in cases when you have a lot of rows, which is also when you want to have BIGINTs as your IDs.

fabianmenges · 2017-09-26T16:07:46Z

I just created a couple youtube videos of the functionality of my changeset, #3518
I'd be happy to incorporate your annotation table and the ability to create "rectangular" annotations and just add it as another type of annotation. Currently my change set incorporates "Events", "Formulas", "Time Series". I could create another type "Annotation" that would pull the annotations from the Annotation table in the Superset DB. I could also then provide a UI where you can add annotations inside of Superset for that Layer.

mistercrunch · 2017-09-26T19:13:02Z

@fabianmenges going to comment on your PR to think holistically about annotations. We spoke about your approach internally and think it's very complementary and would love to integrate all this.

mistercrunch · 2017-09-26T20:52:33Z

@fabianmenges we may need to talk to sync up about annotations, we all really like the work that you've done and do want to support the stuff you added:

slice-defined formulas
slice-defined vertical lines
data-based annotations, using a specific slice-types with the right constraints and guarantees, reusable across slices

Throw in:

slice-defined data points (X/Y) annotations using https://d3-annotation.susielu.com/

On top of this PR, which I would describe as:

Superset-managed Annotation Layers, to be renamed "Time Annotation Layers" to clarify, these are highly re-usage across slices.

All this should be composable.

I'd like to break down 1-2-3 from 4 potentially, though if we can agree on the vision we could make your current PR evolve into 1-2-4 combo. That's a pretty massive feature set though.

fabianmenges · 2017-09-26T22:47:35Z

Lets move this to the dev mailing list...

fabianmenges · 2017-09-27T13:28:36Z

Go ahead and merge this in, I can integrate the UIs in my merge request. Lets have the design discussion in the mailing list.

* Adding annotations to backend * Auto fetching Annotations on the backend * Closing the loop * Adding missing files * annotation layers UI for apache#3502 * a few fixes per code review. - add annotation input sanity check before add and before update. - make SelectAsyncControl component statelesis, and generic - add annotation description in d3 tool tip - use less variable to replace hard-coded color

mistercrunch and others added 5 commits September 24, 2017 19:51

Adding annotations to backend

8ed1f4b

Auto fetching Annotations on the backend

e83c0e4

Closing the loop

6aaa4bc

Adding missing files

3aba1f2

annotation layers UI

cc29e51

for apache#3502

mistercrunch requested changes Sep 25, 2017

View reviewed changes

graceguo-supercat force-pushed the annotations branch from 60b9cbd to 13ace05 Compare September 25, 2017 23:55

a few fixes per code review.

1313151

- add annotation input sanity check before add and before update. - make SelectAsyncControl component statelesis, and generic - add annotation description in d3 tool tip - use less variable to replace hard-coded color

graceguo-supercat force-pushed the annotations branch from 13ace05 to 1313151 Compare September 26, 2017 00:11

mistercrunch mentioned this pull request Sep 26, 2017

Full Annotation Framework #3518

Merged

fabianmenges reviewed Sep 26, 2017

View reviewed changes

mistercrunch merged commit d1a7a7b into apache:master Sep 28, 2017

mistercrunch deleted the annotations branch September 28, 2017 03:40

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

Time Series Annotation Layers #3521

Time Series Annotation Layers #3521

graceguo-supercat commented Sep 25, 2017 •

edited by mistercrunch

Loading

coveralls commented Sep 25, 2017 •

edited

Loading

mistercrunch left a comment

mistercrunch Sep 25, 2017

graceguo-supercat Sep 25, 2017

mistercrunch Sep 25, 2017

mistercrunch Sep 25, 2017

mistercrunch Sep 25, 2017

mistercrunch Sep 25, 2017

mistercrunch Sep 25, 2017

mistercrunch Sep 25, 2017

mistercrunch Sep 25, 2017

mistercrunch Sep 25, 2017

coveralls commented Sep 26, 2017 •

edited

Loading

coveralls commented Sep 26, 2017

coveralls commented Sep 26, 2017 •

edited

Loading

coveralls commented Sep 26, 2017 •

edited

Loading

fabianmenges Sep 26, 2017 •

edited

Loading

mistercrunch Sep 26, 2017

fabianmenges Sep 26, 2017 •

edited

Loading

fabianmenges Sep 26, 2017

mistercrunch Sep 26, 2017 •

edited

Loading

fabianmenges Sep 26, 2017

mistercrunch Sep 26, 2017

mistercrunch Sep 26, 2017

fabianmenges Sep 26, 2017

mistercrunch Sep 26, 2017

fabianmenges Sep 26, 2017

fabianmenges commented Sep 26, 2017

mistercrunch commented Sep 26, 2017

mistercrunch commented Sep 26, 2017 •

edited

Loading

fabianmenges commented Sep 26, 2017

fabianmenges commented Sep 27, 2017

		from .base import SupersetModelView, DeleteMixin


		class AnnotationModelView(SupersetModelView, DeleteMixin): # noqa

Time Series Annotation Layers #3521

Time Series Annotation Layers #3521

Conversation

graceguo-supercat commented Sep 25, 2017 • edited by mistercrunch Loading

coveralls commented Sep 25, 2017 • edited Loading

mistercrunch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 26, 2017 • edited Loading

coveralls commented Sep 26, 2017

coveralls commented Sep 26, 2017 • edited Loading

coveralls commented Sep 26, 2017 • edited Loading

fabianmenges Sep 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianmenges Sep 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistercrunch Sep 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianmenges commented Sep 26, 2017

mistercrunch commented Sep 26, 2017

mistercrunch commented Sep 26, 2017 • edited Loading

fabianmenges commented Sep 26, 2017

fabianmenges commented Sep 27, 2017

graceguo-supercat commented Sep 25, 2017 •

edited by mistercrunch

Loading

coveralls commented Sep 25, 2017 •

edited

Loading

coveralls commented Sep 26, 2017 •

edited

Loading

coveralls commented Sep 26, 2017 •

edited

Loading

coveralls commented Sep 26, 2017 •

edited

Loading

fabianmenges Sep 26, 2017 •

edited

Loading

fabianmenges Sep 26, 2017 •

edited

Loading

mistercrunch Sep 26, 2017 •

edited

Loading

mistercrunch commented Sep 26, 2017 •

edited

Loading