Skip to content

freshness ui #29297

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

Merged
merged 4 commits into from
Apr 16, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion js_modules/dagster-ui/packages/ui-core/client.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,9 @@ export const ASSETS_HEALTH_INFO_QUERY = gql`
...AssetHealthCheckUnknownMetaFragment
}
freshnessStatus
freshnessStatusMetadata {
...AssetHealthFreshnessMetaFragment
}
}
}

Expand Down Expand Up @@ -114,6 +117,10 @@ export const ASSETS_HEALTH_INFO_QUERY = gql`
numNotExecutedChecks
totalNumChecks
}

fragment AssetHealthFreshnessMetaFragment on AssetHealthFreshnessMeta {
lastMaterializedTimestamp
}
`;

// For tests
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
Tag,
ifPlural,
} from '@dagster-io/ui-components';
import dayjs from 'dayjs';
import React, {useMemo} from 'react';
import {Link} from 'react-router-dom';

Expand All @@ -23,6 +24,7 @@ import {
AssetHealthCheckDegradedMetaFragment,
AssetHealthCheckUnknownMetaFragment,
AssetHealthCheckWarningMetaFragment,
AssetHealthFreshnessMetaFragment,
AssetHealthMaterializationDegradedNotPartitionedMetaFragment,
AssetHealthMaterializationDegradedPartitionedMetaFragment,
AssetHealthMaterializationWarningPartitionedMetaFragment,
Expand Down Expand Up @@ -85,6 +87,12 @@ export const AssetHealthSummary = React.memo(
assetKey={key}
text="Has no freshness violations"
status={health?.freshnessStatus}
metadata={health?.freshnessStatusMetadata}
explanation={
!health || health?.freshnessStatus === AssetHealthStatus.NOT_APPLICABLE
? 'No freshness policy defined'
: undefined
}
/>
<Criteria
assetKey={key}
Expand Down Expand Up @@ -126,6 +134,7 @@ const Criteria = React.memo(
| AssetHealthMaterializationDegradedNotPartitionedMetaFragment
| AssetHealthMaterializationDegradedPartitionedMetaFragment
| AssetHealthMaterializationWarningPartitionedMetaFragment
| AssetHealthFreshnessMetaFragment
| undefined
| null;
explanation?: string;
Expand Down Expand Up @@ -236,6 +245,17 @@ const Criteria = React.memo(
</Link>
</Body>
);
case 'AssetHealthFreshnessMeta':
if (metadata.lastMaterializedTimestamp === null) {
return <Body>No materializations</Body>;
}

return (
<Body>
Last materialized {dayjs(Number(metadata.lastMaterializedTimestamp * 1000)).fromNow()}{' '}
ago
Comment on lines +255 to +256
Copy link
Contributor

Choose a reason for hiding this comment

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

There appears to be an issue with the timestamp conversion. The code is multiplying metadata.lastMaterializedTimestamp by 1000 (converting seconds to milliseconds), but dayjs.fromNow() already expects the timestamp to be in the correct format.

If lastMaterializedTimestamp is in Unix seconds (which appears to be the case based on the multiplication), the correct approach would be to use:

dayjs.unix(metadata.lastMaterializedTimestamp).fromNow()

This will properly handle the Unix timestamp in seconds without needing manual conversion. Alternatively, if manual conversion is preferred, ensure the conversion is appropriate for how fromNow() expects to receive timestamps.

The current implementation will display times that are off by a factor of 1000, which could lead to confusing user experiences.

Suggested change
Last materialized {dayjs(Number(metadata.lastMaterializedTimestamp * 1000)).fromNow()}{' '}
ago
Last materialized {dayjs.unix(metadata.lastMaterializedTimestamp).fromNow()}{' '}

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

</Body>
);
case undefined:
return null;
default:
Expand Down
13 changes: 13 additions & 0 deletions python_modules/dagster-test/dagster_test/toys/asset_health.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
from datetime import timedelta

import dagster as dg
from dagster._core.definitions.freshness import InternalFreshnessPolicy
from dagster._time import get_current_timestamp


Expand Down Expand Up @@ -118,6 +121,15 @@ def observable_source_asset_random_execution_error(context):
return dg.DataVersion("5")


@dg.asset(
internal_freshness_policy=InternalFreshnessPolicy.time_window(
fail_window=timedelta(minutes=5), warn_window=timedelta(minutes=1)
)
)
def asset_with_freshness_and_warning():
return 1


def get_assets_and_checks():
return [
random_1,
Expand All @@ -136,4 +148,5 @@ def get_assets_and_checks():
observable_source_asset_always_observes,
observable_source_asset_execution_error,
observable_source_asset_random_execution_error,
asset_with_freshness_and_warning,
]