Skip to content

[historical-runs 3/n] Make runs obey event log entry timestamp #29382

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

Jump to bottom
Open
wants to merge 1 commit into
base: dpeng817/add_history_import_run_method
Choose a base branch
from
Open

[historical-runs 3/n] Make runs obey event log entry timestamp #29382

wants to merge 1 commit into from

Conversation

dpeng817
Copy link
Contributor

Summary & Motivation

How I Tested These Changes

Changelog

Insert changelog entry or delete this section.

@dpeng817 Graphite App
Copy link
Contributor Author

dpeng817 commented Apr 17, 2025 •
edited
Loading

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

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

@dpeng817 dpeng817 mentioned this pull request Apr 17, 2025
Open
@dpeng817 dpeng817 changed the title Make runs obey event log entry timestamp [historical-runs 3/n] Make runs obey event log entry timestamp Apr 17, 2025
@dpeng817 dpeng817 requested review from OwenKephart and prha April 17, 2025 23:29
@dpeng817 dpeng817 marked this pull request as ready for review April 17, 2025 23:29
This was referenced Apr 18, 2025
Open
Open
@dpeng817 dpeng817 force-pushed the dpeng817/add_history_import_run_method branch from f03f254 to 90135e9 Compare April 18, 2025 17:33
@dpeng817 dpeng817 force-pushed the dpeng817/runs_obey_event_timestamp branch from 16ce37e to 558fac0 Compare April 18, 2025 17:33
@dpeng817 dpeng817 force-pushed the dpeng817/add_history_import_run_method branch from 90135e9 to 3dfbac9 Compare April 18, 2025 18:00
@dpeng817 dpeng817 force-pushed the dpeng817/runs_obey_event_timestamp branch from 558fac0 to 18ec729 Compare April 18, 2025 18:00
@prha prha requested review from alangenfeld and jmsanders April 22, 2025 18:14
@prha
Copy link
Member

prha commented Apr 22, 2025

Worth getting more eyes on this. This changes the run start/end to be in terms of the event timestamps rather than by the time that the instance handles the event.

@shalabhc
Copy link
Contributor

Is the update_timestamp user provided? If yes, does that mean users now control the run duration metric?

@dpeng817
Copy link
Contributor Author

dpeng817 commented Apr 22, 2025 •
edited
Loading

@shalabhc the intended use case is for that to come from the event log entry - so not quite user provided but may be calculated by user code.

We'll likely fork implementations outside of OSS to maintain a cleaner user code vs db layer but this seems reasonable for OSS

@dpeng817 dpeng817 force-pushed the dpeng817/add_history_import_run_method branch from 3dfbac9 to 4b3b452 Compare April 23, 2025 16:52
@dpeng817 dpeng817 force-pushed the dpeng817/runs_obey_event_timestamp branch from 18ec729 to e2cf599 Compare April 23, 2025 16:52
@dpeng817 dpeng817 force-pushed the dpeng817/add_history_import_run_method branch from 4b3b452 to 103f752 Compare April 23, 2025 22:00
@dpeng817 dpeng817 force-pushed the dpeng817/runs_obey_event_timestamp branch from e2cf599 to dcd8450 Compare April 23, 2025 22:00
@dpeng817 dpeng817 force-pushed the dpeng817/add_history_import_run_method branch from 103f752 to 4503c8c Compare April 23, 2025 22:28
@dpeng817 dpeng817 force-pushed the dpeng817/runs_obey_event_timestamp branch from dcd8450 to ad88867 Compare April 23, 2025 22:28
@dpeng817 dpeng817 force-pushed the dpeng817/add_history_import_run_method branch from 4503c8c to 1a5d483 Compare April 23, 2025 23:10
@dpeng817 dpeng817 force-pushed the dpeng817/runs_obey_event_timestamp branch from ad88867 to 1042d1d Compare April 23, 2025 23:10
@dpeng817 dpeng817 force-pushed the dpeng817/add_history_import_run_method branch from 1a5d483 to 850a289 Compare April 23, 2025 23:36
@dpeng817 dpeng817 force-pushed the dpeng817/runs_obey_event_timestamp branch from 1042d1d to f6b9f49 Compare April 23, 2025 23:36
@dpeng817 dpeng817 force-pushed the dpeng817/add_history_import_run_method branch from 850a289 to 4433fb7 Compare April 24, 2025 00:08
@dpeng817 dpeng817 force-pushed the dpeng817/runs_obey_event_timestamp branch from f6b9f49 to 2923b33 Compare April 24, 2025 00:08
@dpeng817 dpeng817 force-pushed the dpeng817/add_history_import_run_method branch from 4433fb7 to 1df7391 Compare April 24, 2025 00:51
@dpeng817 dpeng817 force-pushed the dpeng817/runs_obey_event_timestamp branch from 2923b33 to 6125bf2 Compare April 24, 2025 00:51
@dpeng817 dpeng817 force-pushed the dpeng817/add_history_import_run_method branch from 1df7391 to 0e8e74e Compare April 24, 2025 00:59
@dpeng817 dpeng817 force-pushed the dpeng817/runs_obey_event_timestamp branch from 6125bf2 to ad57047 Compare April 24, 2025 00:59
@dpeng817 dpeng817 force-pushed the dpeng817/add_history_import_run_method branch from 0e8e74e to 49297e7 Compare April 24, 2025 04:19
@dpeng817 dpeng817 force-pushed the dpeng817/runs_obey_event_timestamp branch from ad57047 to c33f358 Compare April 24, 2025 04:19
@dpeng817 dpeng817 force-pushed the dpeng817/add_history_import_run_method branch from 49297e7 to 6cc9aa6 Compare April 24, 2025 04:29
@dpeng817 dpeng817 force-pushed the dpeng817/runs_obey_event_timestamp branch from c33f358 to 9417cf2 Compare April 24, 2025 04:29
@dpeng817 dpeng817 force-pushed the dpeng817/add_history_import_run_method branch from 6cc9aa6 to a89a412 Compare April 24, 2025 05:07
@dpeng817 dpeng817 force-pushed the dpeng817/runs_obey_event_timestamp branch from 9417cf2 to 813d40f Compare April 24, 2025 05:07
@dpeng817 dpeng817 force-pushed the dpeng817/add_history_import_run_method branch from a89a412 to 101bdca Compare April 24, 2025 16:54
@dpeng817 dpeng817 force-pushed the dpeng817/runs_obey_event_timestamp branch from 813d40f to 2a1333e Compare April 24, 2025 16:54
alangenfeld
alangenfeld reviewed Apr 24, 2025
View reviewed changes
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.

i think this is probably fine

python_modules/dagster/dagster/_core/storage/runs/sql_run_storage.py
Comment on lines -192 to -193
# consider changing the `handle_run_event` signature to get timestamp off of the
# EventLogEntry instead of the DagsterEvent, for consistency
Copy link
Member

Choose a reason for hiding this comment

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

the fact this comment was already here i think helps this changes case

python_modules/dagster/dagster/_core/storage/runs/sql_run_storage.py
Comment on lines +172 to +174
def handle_run_event(
self, run_id: str, event: DagsterEvent, update_timestamp: Optional[datetime] = None
) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

will have to update the ABC

https://github.com/dagster-io/dagster/blob/master/python_modules/dagster/dagster/_core/storage/runs/base.py#L57-L64

theoretically could break custom run storages, not sure how much to worry about that

python_modules/dagster/dagster/_core/storage/runs/sql_run_storage.py
# consider changing the `handle_run_event` signature to get timestamp off of the
# EventLogEntry instead of the DagsterEvent, for consistency
now = get_current_datetime()
update_timestamp = update_timestamp or get_current_datetime()
Copy link
Member

Choose a reason for hiding this comment

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

comments here or below to clarify what "update_timestamp" means to us ie not when the db row was last touched but the time of the latest event to change the status of the run

@dpeng817 dpeng817 force-pushed the dpeng817/add_history_import_run_method branch from 101bdca to 3deac73 Compare April 24, 2025 23:48
@dpeng817 dpeng817 force-pushed the dpeng817/runs_obey_event_timestamp branch from 2a1333e to 7e32499 Compare April 24, 2025 23:48
@dpeng817 dpeng817 force-pushed the dpeng817/add_history_import_run_method branch from 3deac73 to aa14550 Compare April 25, 2025 00:15
@dpeng817 dpeng817 force-pushed the dpeng817/runs_obey_event_timestamp branch from 7e32499 to e30a5d8 Compare April 25, 2025 00:15
@dpeng817 dpeng817 force-pushed the dpeng817/add_history_import_run_method branch from aa14550 to cc1adfe Compare April 25, 2025 00:30
@dpeng817 dpeng817 force-pushed the dpeng817/runs_obey_event_timestamp branch from e30a5d8 to bafa107 Compare April 25, 2025 00:30
@dpeng817 dpeng817 force-pushed the dpeng817/add_history_import_run_method branch from cc1adfe to eae751a Compare April 25, 2025 02:06
@dpeng817 dpeng817 force-pushed the dpeng817/runs_obey_event_timestamp branch from bafa107 to 560b46c Compare April 25, 2025 02:06
This was referenced Apr 25, 2025
Draft
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alangenfeld alangenfeld alangenfeld left review comments

@OwenKephart OwenKephart Awaiting requested review from OwenKephart

@prha prha Awaiting requested review from prha

@jmsanders jmsanders Awaiting requested review from jmsanders

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dpeng817 @prha @shalabhc @alangenfeld