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

Add asset events to dashboard #44961

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

tirkarthi
Copy link
Contributor

@tirkarthi tirkarthi commented Dec 16, 2024

Add asset events to dashboard to display the top 6 events sorted by default newest first (latest) asset events. The source links to the task instance the created the event. The selected time from filter is applied to the API to get only events in last 12 hours, past week etc. The triggered dagruns links to the dagruns triggered due to the asset event.

Notes to reviewer and self

  1. The UI prototype had small circle to denote the task instance and dagrun status but this might cause more API calls.
  2. The first dagrun is linked with the prototype a link to "+n more" where n are the remaining dagruns. But currently there is no asset page to link to for better information and how do we display the same.
  3. When there is from_rest_api it's usually from API and source is displayed as API. Similar can be done for asset events from trigger. Opened Add from_trigger field to AssetEvent extra for events from Trigger #44944 for discussion.
  4. The UI prototype displays asset.uri for the card. Do we need to display asset.name or asset.uri ?
  5. The variant should be flushed but typescript doesn't agree with the variant to remove border as per design.
  6. In case of no events does this section needs to be displayed?
  7. Padding, margin and other style comments welcome.

Related #42700

image

image

@boring-cyborg boring-cyborg bot added the area:UI Related to UI/UX. For Frontend Developers. label Dec 16, 2024
@tirkarthi
Copy link
Contributor Author

cc: @uranusjr @Lee-W for feedback on asset events in UI

@Lee-W Lee-W self-requested a review December 16, 2024 15:36
@@ -102,6 +102,7 @@ class AssetEventResponse(BaseModel):

id: int
asset_id: int
uri: str | None = Field(alias="uri", default=None)
Copy link
Member

Choose a reason for hiding this comment

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

if we're to add uri here, we'll probably need to add name (and maybe group?)

Copy link
Member

Choose a reason for hiding this comment

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

I’m not sure we should add uri here. If we need it in the UI, it’d be better to include the entire asset object instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is as per the initial UI design for dashboard. I am okay with changing this as per consensus.

Copy link
Contributor

Choose a reason for hiding this comment

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

The designs were quite preliminary so I am happy with adding everything suggested

@jscheffl
Copy link
Contributor

jscheffl commented Jan 1, 2025

Note: As PR #45312 has been merged, the code formatting rules have changed for new UI. Please rebase and re-run pre-commit checks to ensure that formatting in folder airflow/ui is adjusted.

Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Really cool.

Code looks good. No more suggestions than what's already been highlighted.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants