You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/04/18 21:39:12 UTC
[GitHub] [airflow] BenoitHanotte opened a new pull request #15423: UI: DAG Calendar View
BenoitHanotte opened a new pull request #15423:
URL: https://github.com/apache/airflow/pull/15423
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
# Motivation
Introduce a DAG Calendar View to provide visibility over the full state of the dag by displaying the aggregated dag runs' states in a calendar.
This makes it possible to monitor the state of thousands of dag runs in a single view that is concise and easy to understand. It is particularly useful to monitor the state of large backfills:
Each day is displayed with a color according to the dag runs' states for that day:
- If at least one dag run has failed for a day, the day will be displayed as "failed".
- If all dag runs have succeeded, the day will be shown as "succeeded".
- If there are still running dag runs (and no failed dag run) for that day, the day will be shown as "running".
A tooltip, for each day, gives the exact number of dag runs in each state for the day.
Clicking on a day redirects to the tree view for that day to show the task instances for that day.
# Implementation
- New `/calendar` endpoint in `views.py`.
This view returns, for each day, the number of dag run in each state. Thus, only the `dag_run` table is queried, and the query returns at most 3 * days records. Thus, this view puts little strain on the database, even for dags with a large number of task instances.
- New `calendar.html` template, `calendar.js` and `calendar.css` static files.
The calendar view is implemented usng d3, thus no additional dependency has been introduced.
The proposal has been discussed on the dev mailing list:
https://lists.apache.org/thread.html/r6f2ea3f83d268a665ac63219553718ed089a01e9530627cca0572ab1%40%3Cdev.airflow.apache.org%3E
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BenoitHanotte edited a comment on pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
BenoitHanotte edited a comment on pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#issuecomment-826398624
> Technical: Overall looks good. In addition to the comments I left, the `calendar.js` file needs a linting pass. There were too many little issues to comment on.
>
> General: I still feel like we could make this view more data-rich. It would be great to do a heatmap of the ratio of successful and failed runs. After that, an indication of number of total runs a day. It may take some trial & error to get it both of those to look right within the same chart.
Thanks for the review, I applied `eslint` and addressed the comments.
I also made a change to use a heatmap to fill the day status, here is how it currently looks like:
If there is at least one failure for a day, the color will be a mix of green and red according to the ratio of failed runs over the number of runs for that day (https://github.com/apache/airflow/pull/15423/files#diff-4bd7c1010027099f09d8d824b871bfc65c54843dc5283d3ec336182c623712b6R216)
On the screenshot above you can see 3 days with faile DAG runs: Jan 16 has 1 failed run, Jan 17 has 4 and Jan 18 has 8 failed DAG runs. The other days only have successful DAG runs, thus they are displayed as green.
Don't hesitate to let me know what you think! :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BenoitHanotte commented on a change in pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
BenoitHanotte commented on a change in pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#discussion_r621021682
##########
File path: airflow/www/static/js/calendar.js
##########
@@ -0,0 +1,269 @@
+/* eslint-disable func-names */
+/* eslint-disable no-underscore-dangle */
Review comment:
@uranusjr thanks, `eslint` seems to be ok with that, I have updated the code and removed the eslint exception :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb merged pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #15423:
URL: https://github.com/apache/airflow/pull/15423
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BenoitHanotte commented on a change in pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
BenoitHanotte commented on a change in pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#discussion_r619884436
##########
File path: airflow/www/static/css/calendar.css
##########
@@ -0,0 +1,60 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+text.year-label {
+ font-size: 16px;
+ font-weight: bold;
+}
+
+text.day-label {
+ font-size: 12px;
+}
+
+path.month {
+ stroke: black;
+ stroke-width: 2px;
+}
+
+rect.day {
+ stroke: #ccc;
+ stroke-width: 1px;
+ cursor: pointer;
+}
+
+rect.success {
+ fill: green;
+}
+
+rect.running {
+ fill: lime;
+}
+
+rect.failed {
+ fill: red;
+}
+
+rect.no_status {
+ fill: #eee;
Review comment:
Thanks, I have fixed it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BenoitHanotte edited a comment on pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
BenoitHanotte edited a comment on pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#issuecomment-830876792
> Way better! Good call on `interpolateHsl` We should update the legend to explain the range of colors. That should help mitigate your first concern.
@bbovenzi I have updated the legend, it adds a bit of complexity as to display the "gradient" in the legend it required moving it to the svg graph itself:
> Would it be more useful to reverse the year ordering? Seems like the current year is likely to be the most relevant to the user.
@ryanahamilton I experimented with it but I fear it might be a bit confusing as consecutive days are not juxtaposed from left-to-right and top-to-bottom anymore:
Let me know which option you prefer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BenoitHanotte edited a comment on pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
BenoitHanotte edited a comment on pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#issuecomment-828186014
> The red-green mixing doesn't seem right. I don't know if we just have to play with the function you already have or use a different one like this: https://www.geeksforgeeks.org/d3-js-interpolaterdylgn-function/
>
> Code-wise things look good to me. I think we still need to refine the UI. I'll pull it down again and experiment a bit.
The interpolation function `interpolaterdylgn` does not interpolate between green and red but between 3 colors: green, yellow and red. We can't hardcode these colors in the UI as the backend is responsible for providing the status colors to the UI.
I can however easily change the interpolation function from `interpolateRgb` to `interpolateHsl` which does the interpolation in the HSL space and thus goes from green to red through yellow. It gives the following colors:
I have 2 concerns with this approach though:
1. yellow is already used as the color for the `up_for_retry` and `upstream_failed` task instance statuses. Displaying a day as yellow might be misleading to users as they might think there is a just task that will be retried that day?
2. As you can see on the screenshot above, day with just a few failures (Jan 15 and 16) are displayed as light green, which might not make it obvious that these days have not been processed successfully. I fear this might be misleading and misunderstood by users.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a change in pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#discussion_r618864815
##########
File path: airflow/www/static/js/calendar.js
##########
@@ -0,0 +1,269 @@
+/* eslint-disable func-names */
+/* eslint-disable no-underscore-dangle */
Review comment:
We should remove both `eslint-disable`. Neither rule is too cumbersome for this file.
##########
File path: airflow/www/static/css/calendar.css
##########
@@ -0,0 +1,60 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+text.year-label {
+ font-size: 16px;
+ font-weight: bold;
+}
+
+text.day-label {
+ font-size: 12px;
+}
+
+path.month {
+ stroke: black;
+ stroke-width: 2px;
+}
+
+rect.day {
+ stroke: #ccc;
+ stroke-width: 1px;
+ cursor: pointer;
+}
+
+rect.success {
+ fill: green;
+}
+
+rect.running {
+ fill: lime;
+}
+
+rect.failed {
+ fill: red;
+}
+
+rect.no_status {
+ fill: #eee;
Review comment:
No Status is white elsewhere in the app. We should stay consistent here too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BenoitHanotte edited a comment on pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
BenoitHanotte edited a comment on pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#issuecomment-830876792
> Way better! Good call on `interpolateHsl` We should update the legend to explain the range of colors. That should help mitigate your first concern.
I have updated the legend, it adds a bit of complexity as to display the "gradient" in the legend it required moving it to the svg graph itself:
> Would it be more useful to reverse the year ordering? Seems like the current year is likely to be the most relevant to the user.
I experimented with it might be a bit confusing as consecutive days are not juxtaposed from left-to-right and top-to-bottom anymore:
Let me know which option you prefer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BenoitHanotte edited a comment on pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
BenoitHanotte edited a comment on pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#issuecomment-828186014
> The red-green mixing doesn't seem right. I don't know if we just have to play with the function you already have or use a different one like this: https://www.geeksforgeeks.org/d3-js-interpolaterdylgn-function/
>
> Code-wise things look good to me. I think we still need to refine the UI. I'll pull it down again and experiment a bit.
The interpolation function `interpolaterdylgn` does not interpolate between green and red but between 3 colors: green, yellow and red. We can't hardcode these colors in the UI as the backend is responsible for providing the status colors to the UI.
I can however easily change the interpolation function from `interpolateRgb` to `interpolateHsl` which does the interpolation in the HSL space and thus goes from green to red through yellow. It gives the following colors:
I have 2 concerns with this approach though:
1. yellow is already used as the color for the `up_for_retry` and `upstream_failed` task instance statuses. Displaying a day as yellow might be misleading to users as they might think there is a just task that will be retried that day?
2. As you can see on the screenshot above, day with just a few failures (Jan 15 and 16) are displayed as light green, which might not make it obvious that these days have not been processed successfully. I fear this might be misleading and misunderstood by users. We can tweak the interpolation ratio "floor" value (named `minColorInterpolation` in my code) to prevent using the light green part of the scale. For instance a value of 0.5 gives the following colors:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#issuecomment-828636983
Way better! Good call on `interpolateHsl` We should update the legend to reflect this which should help mitigate your first concern.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BenoitHanotte edited a comment on pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
BenoitHanotte edited a comment on pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#issuecomment-828186014
> The red-green mixing doesn't seem right. I don't know if we just have to play with the function you already have or use a different one like this: https://www.geeksforgeeks.org/d3-js-interpolaterdylgn-function/
>
> Code-wise things look good to me. I think we still need to refine the UI. I'll pull it down again and experiment a bit.
The interpolation function `interpolaterdylgn` does not interpolate between green and red but between 3 colors: green, yellow and red. We can't hardcode these colors in the UI as the backend is responsible for providing the status colors to the UI.
I can however easily change the interpolation function from `interpolateRgb` to `interpolateHsl` which does the interpolation in the HSL space and thus goes from green to red through yellow. It gives the following colors:
I have 2 concerns with this approach though:
1. yellow is already used as the color for the `up_for_retry` and `upstream_failed` task instance statuses. Displaying a day as yellow might be misleading to users as they might think there is a just task that will be retried that day?
2. As you can see on the screenshot above, a day with just one failure is displayed as light green, which might not make it obvious that that day has not been processed correctly. I fear this might be misleading and misunderstood by users.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#issuecomment-832793173
The gradient legend is perfect.
I think the original top-to-bottom arrangement was better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#discussion_r626670584
##########
File path: airflow/www/views.py
##########
@@ -2078,6 +2078,71 @@ def tree(self):
external_log_name=external_log_name,
)
+ @expose('/calendar')
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_LOG),
Review comment:
```suggestion
```
I don't think this makes sense to include, as it doesn't directly read the task logs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BenoitHanotte commented on a change in pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
BenoitHanotte commented on a change in pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#discussion_r619884436
##########
File path: airflow/www/static/css/calendar.css
##########
@@ -0,0 +1,60 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+text.year-label {
+ font-size: 16px;
+ font-weight: bold;
+}
+
+text.day-label {
+ font-size: 12px;
+}
+
+path.month {
+ stroke: black;
+ stroke-width: 2px;
+}
+
+rect.day {
+ stroke: #ccc;
+ stroke-width: 1px;
+ cursor: pointer;
+}
+
+rect.success {
+ fill: green;
+}
+
+rect.running {
+ fill: lime;
+}
+
+rect.failed {
+ fill: red;
+}
+
+rect.no_status {
+ fill: #eee;
Review comment:
Thanks, I have fixed it
##########
File path: airflow/www/static/js/calendar.js
##########
@@ -0,0 +1,269 @@
+/* eslint-disable func-names */
+/* eslint-disable no-underscore-dangle */
Review comment:
I removed the second one, but disabling `func-names` is required to be able to use d3-tip as:
```
.on('mouseover', function (data) {
const tt = tipHtml(data);
dayTip.direction('n');
dayTip.show(tt, this);
})
.on('mouseout', function (data) {
dayTip.hide(data, this);
});
```
Using arrow-style functions won't work for some reason.
Removing `func-names` requires extracting the anonymous functions, but that also makes the code a bit heavier. I believe this is the reason `func-names` is currently disabled in `tree.js` as well.
What do you think? should we keep it this way or should we extract the tooltip functions?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a change in pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#discussion_r626696754
##########
File path: airflow/www/templates/airflow/calendar.html
##########
@@ -0,0 +1,55 @@
+{#
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+#}
+
+{% extends "airflow/dag.html" %}
+{% block page_title %}{{ dag.dag_id }} - Tree - {{ appbuilder.app_name }}{% endblock %}
+
+{% block head_css %}
+ {{ super() }}
+ <meta name="dag_id" content="{{ dag.dag_id }}">
+ <meta name="tree_url" content="{{ url_for('Airflow.tree') }}">
+ <link rel="stylesheet" type="text/css" href="{{ url_for_asset('calendar.css') }}">
+ </style>
+{% endblock %}
+
+{% block content %}
+ {{ super() }}
+ <hr>
+ <div id="svg_container">
+ <img id='loading' width="50" src="{{ url_for('static', filename='loading.gif') }}">
+ <svg id="calendar-svg">
+ </svg>
+ </div>
+{% endblock %}
+
+{% block tail_js %}
+ {{ super() }}
+ <script src="{{ url_for_asset('d3.min.js') }}"></script>
+ <script src="{{ url_for_asset('d3-tip.js') }}"></script>
+ <script src="{{ url_for_asset('calendar.js') }}"></script>
+ <script>
+ const statesColors = {};
+ statesColors["no_status"] = "white";
+ {% for state in ['failed', 'success', 'running'] %}
+ statesColors["{{state}}"] = "{{state_color_mapping[state]}}";
+ {% endfor %}
Review comment:
```suggestion
{% for state in ['failed', 'success', 'running'] %}
statesColors["{{state}}"] = "{{state_color_mapping[state]}}";
{% endfor %}
```
##########
File path: airflow/www/templates/airflow/calendar.html
##########
@@ -0,0 +1,55 @@
+{#
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+#}
+
+{% extends "airflow/dag.html" %}
+{% block page_title %}{{ dag.dag_id }} - Tree - {{ appbuilder.app_name }}{% endblock %}
+
+{% block head_css %}
+ {{ super() }}
+ <meta name="dag_id" content="{{ dag.dag_id }}">
+ <meta name="tree_url" content="{{ url_for('Airflow.tree') }}">
+ <link rel="stylesheet" type="text/css" href="{{ url_for_asset('calendar.css') }}">
+ </style>
+{% endblock %}
+
+{% block content %}
+ {{ super() }}
+ <hr>
+ <div id="svg_container">
+ <img id='loading' width="50" src="{{ url_for('static', filename='loading.gif') }}">
+ <svg id="calendar-svg">
+ </svg>
Review comment:
```suggestion
<svg id="calendar-svg"></svg>
```
##########
File path: airflow/www/templates/airflow/calendar.html
##########
@@ -0,0 +1,55 @@
+{#
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+#}
+
+{% extends "airflow/dag.html" %}
+{% block page_title %}{{ dag.dag_id }} - Tree - {{ appbuilder.app_name }}{% endblock %}
+
+{% block head_css %}
+ {{ super() }}
+ <meta name="dag_id" content="{{ dag.dag_id }}">
+ <meta name="tree_url" content="{{ url_for('Airflow.tree') }}">
+ <link rel="stylesheet" type="text/css" href="{{ url_for_asset('calendar.css') }}">
+ </style>
Review comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BenoitHanotte edited a comment on pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
BenoitHanotte edited a comment on pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#issuecomment-826398624
> Technical: Overall looks good. In addition to the comments I left, the `calendar.js` file needs a linting pass. There were too many little issues to comment on.
>
> General: I still feel like we could make this view more data-rich. It would be great to do a heatmap of the ratio of successful and failed runs. After that, an indication of number of total runs a day. It may take some trial & error to get it both of those to look right within the same chart.
Thanks for the review, I applied `eslint` and addressed the comments.
I also made a change to use a heatmap to fill the day status, here is how it currently looks like:
If there is at least one failure for a day, the color will be a mix of green and red according to the ratio of failed runs over the number of runs for that day (https://github.com/apache/airflow/pull/15423/files#diff-4bd7c1010027099f09d8d824b871bfc65c54843dc5283d3ec336182c623712b6R216)
On the screenshot above you can see 3 days with faile DAG runs: Jan 16 has 1 failed run, Jan 17 has 4 and Jan 18 has 8 failed DAG runs. The other days only have successful DAG runs, thus they are displayed as green.
Don't hesitate to let me know what you think! :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BenoitHanotte commented on pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
BenoitHanotte commented on pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#issuecomment-826398624
> Technical: Overall looks good. In addition to the comments I left, the `calendar.js` file needs a linting pass. There were too many little issues to comment on.
>
> General: I still feel like we could make this view more data-rich. It would be great to do a heatmap of the ratio of successful and failed runs. After that, an indication of number of total runs a day. It may take some trial & error to get it both of those to look right within the same chart.
Thanks for the review, I applied `eslint` and addressed the comments.
I also made a change to use a heatmap to fill the day status, here is how it currently looks like:
If there is at least one failure for a day, the color will be a mix of green and red according to the ratio of failed runs over the number of runs for that day (https://github.com/apache/airflow/pull/15423/files#diff-4bd7c1010027099f09d8d824b871bfc65c54843dc5283d3ec336182c623712b6R216)
On the screenshot above you can see 3 days with faile DAG runs: Jan 16 has 1 failed run, Jan 17 has 4 and Jan 18 has 8 failed DAG runs.
Don't hesitate to let me know what you think! :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BenoitHanotte commented on pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
BenoitHanotte commented on pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#issuecomment-826398624
> Technical: Overall looks good. In addition to the comments I left, the `calendar.js` file needs a linting pass. There were too many little issues to comment on.
>
> General: I still feel like we could make this view more data-rich. It would be great to do a heatmap of the ratio of successful and failed runs. After that, an indication of number of total runs a day. It may take some trial & error to get it both of those to look right within the same chart.
Thanks for the review, I applied `eslint` and addressed the comments.
I also made a change to use a heatmap to fill the day status, here is how it currently looks like:
If there is at least one failure for a day, the color will be a mix of green and red according to the ratio of failed runs over the number of runs for that day (https://github.com/apache/airflow/pull/15423/files#diff-4bd7c1010027099f09d8d824b871bfc65c54843dc5283d3ec336182c623712b6R216)
On the screenshot above you can see 3 days with faile DAG runs: Jan 16 has 1 failed run, Jan 17 has 4 and Jan 18 has 8 failed DAG runs.
Don't hesitate to let me know what you think! :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BenoitHanotte commented on pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
BenoitHanotte commented on pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#issuecomment-828186014
> The red-green mixing doesn't seem right. I don't know if we just have to play with the function you already have or use a different one like this: https://www.geeksforgeeks.org/d3-js-interpolaterdylgn-function/
>
> Code-wise things look good to me. I think we still need to refine the UI. I'll pull it down again and experiment a bit.
The interpolation function `interpolaterdylgn` does not interpolate between green and red but between 3 colors: green, yellow and red. We can't hardcode these colors in the UI as the backend is responsible for providing the status colors to the UI.
I can however easily change the interpolation function from `interpolateRgb` to `interpolateHsl` which does the interpolation in the HSL space and thus goes from green to red through yellow. It gives the following colors:
However, yellow is already used as the color for the `up_for_retry` and `upstream_failed` task instance statuses. Displaying a day as yellow might be misleading to users as they might think there is a just task that will be retried that day?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BenoitHanotte commented on pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
BenoitHanotte commented on pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#issuecomment-828232975
@bbovenzi I have pushed the change to use `interpolateHsl` (so that we go from green to read going through yellow), and raised the interpolation floor to 0.5 so that days with just a few failures don't appear in a green that is close to the "success" green and would thus "hide" the failures.
The current result is the following:
Let me know what you think :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#discussion_r620995532
##########
File path: airflow/www/static/js/calendar.js
##########
@@ -0,0 +1,269 @@
+/* eslint-disable func-names */
+/* eslint-disable no-underscore-dangle */
Review comment:
I’m far from fluent in JavaScript, but would sound like this pass?
```js
.on('mouseout, function hideDayTipOnMouseOut(data) {
dayTip.hide(data, this)
});
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#issuecomment-834600843
@bbovenzi Are you happy with this PR now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BenoitHanotte edited a comment on pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
BenoitHanotte edited a comment on pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#issuecomment-828186014
> The red-green mixing doesn't seem right. I don't know if we just have to play with the function you already have or use a different one like this: https://www.geeksforgeeks.org/d3-js-interpolaterdylgn-function/
>
> Code-wise things look good to me. I think we still need to refine the UI. I'll pull it down again and experiment a bit.
The interpolation function `interpolaterdylgn` does not interpolate between green and red but between 3 colors: green, yellow and red. We can't hardcode these colors in the UI as the backend is responsible for providing the status colors to the UI.
I can however easily change the interpolation function from `interpolateRgb` to `interpolateHsl` which does the interpolation in the HSL space and thus goes from green to red through yellow. It gives the following colors:
I have 2 concerns with this approach though:
1. yellow is already used as the color for the `up_for_retry` and `upstream_failed` task instance statuses. Displaying a day as yellow might be misleading to users as they might think there is a just task that will be retried that day?
2. As you can see on the screenshot above, day with just a few failures (Jan 15 and 16) are displayed as light green, which might not make it obvious that that day has not been processed correctly. I fear this might be misleading and misunderstood by users.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#issuecomment-834610712
Awesome work, congrats on your first merged pull request!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi edited a comment on pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
bbovenzi edited a comment on pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#issuecomment-828636983
Way better! Good call on `interpolateHsl` We should update the legend to explain the range of colors. That should help mitigate your first concern.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BenoitHanotte commented on a change in pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
BenoitHanotte commented on a change in pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#discussion_r619885706
##########
File path: airflow/www/static/js/calendar.js
##########
@@ -0,0 +1,269 @@
+/* eslint-disable func-names */
+/* eslint-disable no-underscore-dangle */
Review comment:
I removed the second one, but disabling `func-names` is required to be able to use d3-tip as:
```
.on('mouseover', function (data) {
const tt = tipHtml(data);
dayTip.direction('n');
dayTip.show(tt, this);
})
.on('mouseout', function (data) {
dayTip.hide(data, this);
});
```
Using arrow-style functions won't work for some reason.
Removing `func-names` requires extracting the anonymous functions, but that also makes the code a bit heavier. I believe this is the reason `func-names` is currently disabled in `tree.js` as well.
What do you think? should we keep it this way or should we extract the tooltip functions?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#issuecomment-827902420
The red-green mixing doesn't seem right. I don't know if we just have to play with the function you already have or use a different one like this: https://www.geeksforgeeks.org/d3-js-interpolaterdylgn-function/
Code-wise things look good to me. I think we still need to refine the UI. I'll pull it down again and experiment a bit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ryanahamilton commented on pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
ryanahamilton commented on pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#issuecomment-828695034
Would it be more useful to reverse the year ordering? Seems like the current year is likely to be the most relevant to the user.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BenoitHanotte commented on pull request #15423: UI: DAG Calendar View
Posted by GitBox <gi...@apache.org>.
BenoitHanotte commented on pull request #15423:
URL: https://github.com/apache/airflow/pull/15423#issuecomment-830876792
> Way better! Good call on `interpolateHsl` We should update the legend to explain the range of colors. That should help mitigate your first concern.
I have updated the legend, it adds a bit of complexity as to display the "gradient" in the legend it required moving the legend in the svg object:
> Would it be more useful to reverse the year ordering? Seems like the current year is likely to be the most relevant to the user.
I experimented with it might be a bit confusing as consecutive days are not juxtaposed from left-to-right and top-to-bottom anymore:
Let me know which option you prefer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org