Skip to content

Fix Start Date tooltip on DAGs page not showing actual start_date #10637

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.

Sign up for GitHub

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

Merged
ashb merged 1 commit into from
Oct 20, 2020
Merged

Fix Start Date tooltip on DAGs page not showing actual start_date #10637

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions airflow/www/templates/airflow/dags.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,14 +370,15 @@ <h2>DAGs</h2>
function lastDagRunsHandler(error, json) {
Copy link
Member

@kaxil kaxil Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at this point this function name needs changing too

Copy link
Contributor Author

@alexbegg alexbegg Sep 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I am not adding new functionality, just fixing a bug/regression. I am just putting back a value lost when the values were changed to load asynchronously. But it is a valid point as that function was added at that point it was changed to be asynchronous.

This function appears to be named the same as the /last_dagruns endpoint that returns json for these dates, which is in turn seems to be named after the "Last Run" column this tooltip is in. So the column is showing the last run's execution date and the last run's start date. What name should it be instead?

Copy link
Contributor Author

@alexbegg alexbegg Sep 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction, the function is named after the dag.get_last_dagrun method which was used in the pre-asynchronous code for the value of last_run, which then was used for the values last_run.execution_date and last_run.start_date, but was incorrectly changed to be last_run = execution_date when it went asynchronous in commit 6607e48#diff-f38558559ea1b4c30ddf132b7f223cf9L122. This PR just puts the values for the asynchronous code back to as they were.

for(var safe_dag_id in json) {
dag_id = json[safe_dag_id].dag_id;
last_run = json[safe_dag_id].last_run;
execution_date = json[safe_dag_id].execution_date;
start_date = json[safe_dag_id].start_date;
g = d3.select('div#last-run-' + safe_dag_id)
g.selectAll('a')
.attr('href', '{{ url_for('Airflow.graph') }}?dag_id=' + encodeURIComponent(dag_id) + '&execution_date=' + encodeURIComponent(last_run))
.insert(isoDateToTimeEl.bind(null, last_run, {title: false}));
.attr('href', '{{ url_for('Airflow.graph') }}?dag_id=' + encodeURIComponent(dag_id) + '&execution_date=' + encodeURIComponent(execution_date))
.insert(isoDateToTimeEl.bind(null, execution_date, {title: false}));
g.selectAll('span')
// We don't translate the timezone in the tooltip, that stays in UTC.
.attr('data-original-title', 'Start Date: ' + last_run)
.attr('data-original-title', 'Start Date: ' + start_date)
.style('display', null);
g.selectAll('.loading-last-run').remove();
}
Expand Down
7 changes: 5 additions & 2 deletions airflow/www/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -726,7 +726,9 @@ def last_dagruns(self, session=None):
return wwwutils.json_response({})

query = session.query(
DagRun.dag_id, sqla.func.max(DagRun.execution_date).label('last_run')
DagRun.dag_id,
sqla.func.max(DagRun.execution_date).label('execution_date'),
sqla.func.max(DagRun.start_date).label('start_date'),
Comment on lines +729 to +731
Copy link
Member

@ashb ashb Sep 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be incorrect an some (unlikely) circumstance:

If I create an dag run for an older execution_date, this would show mixed values.

Unfortunately doing this correctly in a single SQL query is harder.

Copy link
Contributor Author

@alexbegg alexbegg Sep 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can test for that, thanks for the heads up. However I would think the value above it, the execution_date, would be wrong to get the max value in the query, not the run_date. I don't recall that being the case as that query has been the same for a long time now (I just renamed the column alais) but if it is wrong, I'll try and fix it.

Copy link
Contributor Author

@alexbegg alexbegg Sep 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, got busy with other stuff, I'll look into this soon

Copy link
Member

@ashb ashb Oct 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point -- if you could rebase this PR we can merge this as is, as this is right in most cases, and better than the current broken behavior.

Copy link
Contributor Author

@alexbegg alexbegg Oct 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rebased now

).group_by(DagRun.dag_id)

# Filter to only ask for accessible and selected dags
Expand All @@ -735,7 +737,8 @@ def last_dagruns(self, session=None):
resp = {
r.dag_id.replace('.', '__dot__'): {
'dag_id': r.dag_id,
'last_run': r.last_run.isoformat(),
'execution_date': r.execution_date.isoformat(),
'start_date': r.start_date.isoformat(),
} for r in query
}
return wwwutils.json_response(resp)
Expand Down