airflow icon indicating copy to clipboard operation
airflow copied to clipboard

Grid logs for mapped instances

Open pierrejeambrun opened this issue 2 years ago • 5 comments

https://github.com/apache/airflow/pull/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) :heavy_check_mark:
  • Groups with mapped task :heavy_check_mark:
  • Logs display, See More and Download links, and logs filters :heavy_check_mark:
  • When there is multiple pages in the MappedInstances Table :heavy_check_mark:
  • Breadcrumbs navigation :heavy_check_mark:
  • Task actions for all mapped task are working :heavy_check_mark:
  • 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)

Notes: This adds logic to the TaskInstance which is already quite complicated. I believe it went from 'complexe' to 'too complexe', but I do not see a trivial way to refactor it. I do not want to add a lot of changes on top of that PR, I'll most certainly open a follow up PR to this end. Let me know what you think about that.

Also fixes: https://github.com/apache/airflow/issues/25616

image image

pierrejeambrun avatar Aug 08 '22 17:08 pierrejeambrun

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.

pierrejeambrun avatar Aug 09 '22 19:08 pierrejeambrun

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.

bbovenzi avatar Aug 10 '22 20:08 bbovenzi

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 :facepalm:... sorry for that.

pierrejeambrun avatar Aug 10 '22 21:08 pierrejeambrun

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 avatar Aug 10 '22 21:08 pierrejeambrun

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 avatar Aug 10 '22 21:08 pierrejeambrun

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

Thanks

pierrejeambrun avatar Aug 12 '22 20:08 pierrejeambrun

Thank you Brent :)

pierrejeambrun avatar Aug 15 '22 09:08 pierrejeambrun