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

Failed flows card should use start_time timestamp instead of updated #1028

Merged
merged 11 commits into from
Sep 2, 2021

Conversation

znicholasbrown
Copy link
Contributor

PR Checklist:

  • add a short description of what's changed to the top of the CHANGELOG.md
  • add/update tests (or don't, for reasons explained below)

Describe this PR

Switches the filter timestamp on the dashboard failed flow tile.

Copy link
Member

@zhen0 zhen0 left a comment

Choose a reason for hiding this comment

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

Can you give a bit more info about the reasoning for the switch @znicholasbrown? My main concern is that this switch means we no longer show flows that did not actually start in the flow failures tile - if there's a storage error for example (which as a user I'd like to know about).

Example below where a flow failed within the last 24 hours because 'Failed to load and execute Flow's environment...' and is not captured in flow failures tile. (In the current tile query it is captured).

@znicholasbrown
Copy link
Contributor Author

znicholasbrown commented Sep 2, 2021

Hm that's an interesting point - the reason for the switch is that anytime a row in the database is touched (in migrations, for example), the updated timestamp is updated to match. This means that runs that could be weeks or months old (depending on retention) will show up in this tile if anything has touched that row, regardless of the run itself. Given that info I think it makes more sense to use the state_timestamp instead but before I make that switch let me confirm that the state_timestamp is an indexed field.

@znicholasbrown
Copy link
Contributor Author

Ok it looks like that's not an indexed field, so I've updated this to use scheduled_start_time, which is index and should capture those runs that don't have start_time populated.

Copy link
Member

@zhen0 zhen0 left a comment

Choose a reason for hiding this comment

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

Thanks @znicholasbrown - LGTM

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.

2 participants