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

Grid logs for mapped instances #25610

Merged
merged 12 commits into from
Aug 14, 2022

Conversation

pierrejeambrun
Copy link
Member

@pierrejeambrun pierrejeambrun commented Aug 8, 2022

#25568 added support for retrieving mapped instance task logs.

This PR adds support for displaying these logs in the Grid details side panel.

  • Mapped Instances rows in the table are not selectable anymore. (Actions are done on all of them)
  • Mapped Instances rows are clickable and bring a new details page for mapped instances. (We can see the Map Index on top of other detailed information).
  • For the new details, logs tab is available and show the logs for this particular map index.
  • Breadcrumbs get extended with the map index and the taskInstance breadcrumbs becomes clickable in this particular case.
  • Extracting tasks actions into its own component.
  • Add a button to go back to the dynamic task summary (thanks @bbovenzi)
  • Add navigation link the same way we have for normal taks (xcom, details, templates, ...) and remove those links from MappedInstance Table (thanks @bbovenzi)
  • Move the MappedInstance Table to its own tab (again thanks @bbovenzi)
  • Added a few unit tests.

Tested on:

  • Mapped Tasks (obviously) ✔️
  • Groups with mapped task ✔️
  • Logs display, See More and Download links, and logs filters ✔️
  • When there is multiple pages in the MappedInstances Table ✔️
  • Breadcrumbs navigation ✔️
  • Task actions for all mapped task are working ✔️
  • Task actions from the details of a mapped task instance does not work with the SequentialExecutor but the calls seems fine. (I didn't modify that part)

Also fixes: #25616

image
image

@boring-cyborg boring-cyborg bot added area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels Aug 8, 2022
Copy link
Contributor

@bbovenzi bbovenzi left a comment

Choose a reason for hiding this comment

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

Looking good. A few ideas:

  • Let's move the mapped task table to its own tab so on a dynamic task summary you see Details | Mapped Tasks (count)
  • Let's add an extra "go back to dynamic task summary" button. you can use the breadcrumb but not everyone always realizes that
  • let's remove the link buttons from the mapped task table and put that in the mapped task details page like how a normal task instance works. we can then replace that space in the table with a "direct to mapped task logs" button

I'm happy to help pull some of this apart so we can simplify the task instance component.

@pierrejeambrun
Copy link
Member Author

I updated the PR and the description.

  • Nav link is done, and removed them from the table, nice suggestion. :)
    I'm not sure about direct log access, I feel like it is already covered by the preferred tab. If the user preferred tab is 'logs" he will directly land on logs by clicking on the table row otherwise he will land on details.

  • Added the button to go back to the dynamic task summary, good idea!

  • Separate the mapped task table on a new tab only available for mapped task summary. Unfortunately I do not have the number of mapped instance info available atm. (this could be an improvement).

Logs & Mapped Tasks tab are mutually exclusive. In terms of 'preferredTab' they act as the same. Meaning you can go from 'mapped' -> 'logs' and reverse without changing tabs.

@bbovenzi
Copy link
Contributor

bbovenzi commented Aug 10, 2022

Since we store the map_index in the url, we need to make sure we load an individual mapped task instance details.
Try refreshing the page when you have a single mapped task selected and the details panel will be blank.

Also, a task instance might not have logs (ie: it is scheduled but hasn't run yet). We should handle that case instead of just showing a blank page to a user.

Have you tested this with auto-refresh on? I don't think the mapped instance details are updating.

@pierrejeambrun
Copy link
Member Author

pierrejeambrun commented Aug 10, 2022

Since we store the map_index in the url, we need to make sure we load an individual mapped task instance details. Try refreshing the page when you have a single mapped task selected and the details panel will be blank.

I broke that in my last commit 🤦... sorry for that.

@pierrejeambrun
Copy link
Member Author

pierrejeambrun commented Aug 10, 2022

Maybe this is the source of some of the confusion. (I checked auto refresh for mapped task and tasks without logs, it's working now).

@pierrejeambrun pierrejeambrun marked this pull request as draft August 10, 2022 21:45
@pierrejeambrun
Copy link
Member Author

pierrejeambrun commented Aug 10, 2022

I don't think we should be using useEffect here. The Details component should probably get the mapped instance info from the API: /dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/{map_index}

Good idea that would indeed simplify things.

Converted to draft while I implement the suggested change to the API.

edit: This endpoint already exists get_mapped_task_instance

@pierrejeambrun pierrejeambrun force-pushed the grid-logs-for-mapped-instances branch 2 times, most recently from a2e1cd7 to b7d0786 Compare August 12, 2022 19:30
@pierrejeambrun pierrejeambrun marked this pull request as ready for review August 12, 2022 19:51
@pierrejeambrun
Copy link
Member Author

With your suggestions I was able to come up with a much simpler implementation, that I feel will be more robust as well.

Thanks

Copy link
Contributor

@bbovenzi bbovenzi left a comment

Choose a reason for hiding this comment

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

Great work!

@bbovenzi bbovenzi merged commit 8829283 into apache:main Aug 14, 2022
@pierrejeambrun pierrejeambrun deleted the grid-logs-for-mapped-instances branch August 14, 2022 22:55
@pierrejeambrun
Copy link
Member Author

Thank you Brent :)

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. area:webserver Webserver related Issues type:new-feature Changelog: New Features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

grid view: first mapped task logs link points to map index: -1, should point to 0
3 participants