Skip to content
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.

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

[bugfix] Show actual creation timestamp instead of start_time in runs table #22788

Merged
merged 1 commit into from
Jul 3, 2024

Conversation

smackesey
Copy link
Collaborator

@smackesey smackesey commented Jul 1, 2024

Summary & Motivation

A recent user inquiry about the sorting of runs in the backfill runs table
(https://dagsterlabs.slack.com/archives/C064933ANQL/p1719328567183329) led to the discovery that what is listed as the "Create date" in the runs table UI is not actually the "Create time" of the underlying run record, but rather the start_time (when the run was launched) if it was defined, or if not the update_time.

This is both inaccurate and also causes the sorting in the table to be inscrutable in some cases, where there is a subtle deviation from sorting by "Created date".

This PR updates the UI to use the run record creation_time instead of start_time or updated_time. creation_time is always defined since it is set on record creation. This change both fixes the inaccuracy on the runs page and results in more straightforward sorting.

Note that creation time was not exposed on the GrapheneRun GQL type so it is added here.

How I Tested These Changes

Viewed backfill runs page linked by user in shadow dagit with this applied, confirmed sorting was fixed.

Before:

image

After:

image

Copy link
Collaborator Author

smackesey commented Jul 1, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @smackesey and the rest of your teammates on Graphite Graphite

@smackesey smackesey changed the title Sort backfill runs by start time [RFC] Sort backfill runs by start time Jul 1, 2024
@smackesey smackesey force-pushed the sean/backfill-runs-sorting branch 2 times, most recently from ff6e2fc to 86dc60d Compare July 1, 2024 17:40
@smackesey smackesey changed the title [RFC] Sort backfill runs by start time [bugfix] Show actual creation timestamp instead of start_time in runs table Jul 1, 2024
Copy link

github-actions bot commented Jul 1, 2024

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-1ysoo4r67-elementl.vercel.app
https://sean-backfill-runs-sorting.core-storybook.dagster-docs.io

Built with commit fc8374e.
This pull request is being automatically deployed with vercel-action

@smackesey smackesey force-pushed the sean/backfill-runs-sorting branch 2 times, most recently from 1a82012 to b880fb1 Compare July 1, 2024 20:43
@smackesey
Copy link
Collaborator Author

Addressing backfill sorting issue from Foundations standup today. Tagged:

@smackesey smackesey marked this pull request as ready for review July 1, 2024 20:45
Copy link
Member

@alangenfeld alangenfeld left a comment

Choose a reason for hiding this comment

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

nice thx

@@ -586,6 +587,10 @@ def resolve_updateTime(self, graphene_info: ResolveInfo):
run_record = self._get_run_record(graphene_info.context.instance)
return datetime_as_float(run_record.update_timestamp)

def resolve_creationTime(self, graphene_info: ResolveInfo):
run_record = self._get_run_record(graphene_info.context.instance)
Copy link
Member

Choose a reason for hiding this comment

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

looking at __init__ it seems we are always constructed with record now, seems like we can remove this _get_run_record jonx

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point #22805

Copy link
Collaborator

@bengotow bengotow left a comment

Choose a reason for hiding this comment

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

This looks great to me! It seems like <RunTime> is used in a variety of places outside the list, but I scanned through and I think it's fine / better to use the createdAt time in all those spots too.

@smackesey smackesey merged commit c47ac83 into master Jul 3, 2024
2 checks passed
@smackesey smackesey deleted the sean/backfill-runs-sorting branch July 3, 2024 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants