Skip to content

Conversation

@ryanahamilton
Copy link
Contributor

Closes: #10350

On the DAGs index view, the previous i tooltip displayed the UTC timestamp regardless of the currently selected display timezone. This could be redundant if UTC was selected.

I've removed the i tooltip and instead enabled the title attribute on the existing <time></time> element which will conditionally show a UTC tooltip on hover if the display timezone is not UTC. The "UTC:" prefix instead of "Start Date:" also helps clarify the relevance of the timestamp.

Before After
image image

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Aug 25, 2020
@ryanahamilton
Copy link
Contributor Author

@JeffryMAC WDYT?

@ashb
Copy link
Member

ashb commented Aug 25, 2020

I think the start date prefix is still needed.

The column shows the execution_date, which can be wildly different to the start date.

I think

@ryanahamilton
Copy link
Contributor Author

@ashb both values in the existing implementation are keyed off of last_run—the timezone display formatting is the only difference.

@ryw
Copy link
Member

ryw commented Aug 25, 2020

here's an example where the information is diverged:

Screen Shot 2020-08-25 at 3 32 30 PM

maybe the (i) shows when the task run actually started vs. when it was expected to start?

@ryanahamilton
Copy link
Contributor Author

@ryw not sure how they could diverge given they sue the same data value. Is that from an older Airflow version? The different/shorter formatting of the the tooltip date stands out to me.

@JeffryMAC
Copy link

@ryw The behaviour as you see it was in older airflow versions.
I saw this in 1.10.2 somewhere along the line if by design or by regression this behavior was changed and now on the new airflow versions (1.10.9 +) the execution date and the start date are identical in the UI.

I do belive that the better way to solve this is to fix the regression(?) to restore the behavior in older airflow versions but if not possible then at least Airflow shouldn't show duplicated redundant information.

@ryw
Copy link
Member

ryw commented Aug 26, 2020

@ashb do you know if we changed this intentionally, to simplify Airflow UI.

An idea is that we could show the (i) icon if there is a was a difference between last_run and start_date, could make it red if it was significant.

@ashb
Copy link
Member

ashb commented Aug 26, 2020

I don't think it was intentional, no, but someone would have to git bisect to work out when the change was made to be sure

@alexbegg
Copy link
Contributor

alexbegg commented Aug 28, 2020

@ryw not sure how they could diverge given they sue the same data value. Is that from an older Airflow version? The different/shorter formatting of the the tooltip date stands out to me.

@ryanahamilton and @ryw I found the issue/mixup. Yes they diverge in airflow 1.10.x if RBAC UI disabled one is execution_date and one is start_date, but they are the same if RBAC UI is enabled both are execution_date. I think I know how to fix it in the RBAC UI to be different values again so I'll make a PR (see the end of this comment)

  • For the old Non-RBAC UI while they are both derived off of the "last run" they are definitely two different properties. The timestamp shown directly in the column is last_run.execution_date (line 111), whereas the timestamp in the (i) tooltip is last_run.start_date (line 114):

    <!-- Column 7: Last Run -->
    <td class="text-nowrap latest_dag_run {{ dag.dag_id }}">
    {% set last_run = dag.get_last_dagrun(include_externally_triggered=True) %}
    {% if last_run and last_run.execution_date %}
    <a href="{{ url_for('airflow.graph', dag_id=dag.dag_id, execution_date=last_run.execution_date) }}">
    {{ last_run.execution_date.strftime("%Y-%m-%d %H:%M") }}
    </a>
    <span aria-hidden="true" id="statuses_info" title="Start Date: {{ last_run.start_date.strftime("%Y-%m-%d %H:%M") }}" class="glyphicon glyphicon-info-sign"></span>
    {% endif %}
    </td>

  • For the newer RBAC UI the two values are the same. See the last_run variable (defined on line 366) used in both places, for both the link's text (line 370) as well as the (i)'s (span) tooltip (line 373):

    function lastDagRunsHandler(error, json) {
    for(var safe_dag_id in json) {
    dag_id = json[safe_dag_id].dag_id;
    last_run = json[safe_dag_id].last_run;
    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=" + last_run)
    .insert(isoDateToTimeEl.bind(null, last_run, {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)
    .style('display', null);
    g.selectAll(".loading-last-run").remove();
    }
    d3.selectAll(".loading-last-run").remove();
    }

I found out it was the commit that changed the DAGs view to have the latest DAG runs be loaded asynchronously that might have inadvertently changed both dates to the the same value: ba9c035

I found the source of the json passed to the lastDagRunsHandler function for RBAC UI, and if we also pass the start_date value in that json then we can fix the (i) tooltip to actually use the start_date. I am going to make a seperate PR to fix this after some testing:

query = session.query(
DagRun.dag_id, sqla.func.max(DagRun.execution_date).label('last_run')
).group_by(DagRun.dag_id)
# Filter to only ask for accessible and selected dags
query = query.filter(DagRun.dag_id.in_(filter_dag_ids))
resp = {
r.dag_id.replace('.', '__dot__'): {
'dag_id': r.dag_id,
'last_run': r.last_run.isoformat(),
} for r in query
}
return wwwutils.json_response(resp)

@alexbegg
Copy link
Contributor

I don't think it was intentional, no, but someone would have to git bisect to work out when the change was made to be sure

@ashb I did the investigation using git blame, see my above comment

@ryanahamilton
Copy link
Contributor Author

Thanks for the leg-work @alexbegg!

Closing this PR given the discovery that this is a regression that requires a slightly different fix (that @alexbegg intends on offering).

@ryanahamilton ryanahamilton deleted the utc_tooltip branch August 28, 2020 18:00
@ryanahamilton ryanahamilton added the area:UI Related to UI/UX. For Frontend Developers. label Nov 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Start Date in main DAG view shows redundant information

5 participants