Add collapsible failed task logs to prevent React error 185

[dheerajturaga]

This PR is an attempt to mitigate #52916. It does not "fix" the issue but prevents it from happening on the dag landing page.

• Recent failed task logs now start collapsed by default
• Added "Expand Logs"/"Hide Logs" toggle buttons with translations
• Implemented lazy loading - logs only fetch when expanded
• Prevents page crashes from large log content rendering
• Increased log container max height from 100px to 200px

Minified.React.Error.mp4

[dheerajturaga]

@potiuk , @pierrejeambrun I have this work around that will prevent the issue from happening on the DAG landing page. Please let me know if this is good.

[bbovenzi]

Let's move over to show/hide terminology and avoid the confusion with expand/collapse log groups. Otherwise lgtm

[dheerajturaga]

Let's move over to show/hide terminology and avoid the confusion with expand/collapse log groups. Otherwise lgtm

Done! Ive made the changes. Please let me know if any other update is needed

[bbovenzi]

I wonder if we still need this with #54462?

[dheerajturaga]

@bbovenzi , glad the original issue was resolved. I still think having these collapsed by default is useful incase there are dags with too many task failures. Given this is the default landing page for the dag its worth keeping the load times on this page fast

[guan404ming]

glad the original issue was resolved. I still think having these collapsed by default is useful incase there are dags with too many task failures. Given this is the default landing page for the dag its worth keeping the load times on this page fast

I think it would be clearer to show the error log directly instead of requiring an extra click. But I agree it could slow down load times if there are many error logs. How about limiting the number of displayed logs in overview page instead?

[dheerajturaga]

glad the original issue was resolved. I still think having these collapsed by default is useful incase there are dags with too many task failures. Given this is the default landing page for the dag its worth keeping the load times on this page fast

I think it would be clearer to show the error log directly instead of requiring an extra click. But I agree it could slow down load times if there are many error logs. How about limiting the number of displayed logs in overview page instead?

Im not sure about that aswell. Say we limit to 5 failed tasks and have them expanded by default. If the user is interested in the 6th task that isn't displayed here, they would still have to make multiple clicks to get the desired error log. Having them all available in the landing page but collapsed for performance seems to be better IMO.

Open to feedback here.

[pierrejeambrun]

If you try to do this with tasks that have big logs (10M) the collapsing will help, but after expanding a couple of them, the page still becomes unresponsive. (Without this, it's unresponsive/crash on load).

Yes, as mentionned by @guan404ming ideally I think on the 'overview' we only want to show a log sample, maybe the last 100 lines of the logs to be able to debug most cases, if not enough they will need to go specifically check the TI logs on the TI page to load full content. For now we can just do this truncation in the frontend. (for instance custom hook wrapping useLogs and truncating response based on a param right after the backend response is received, i.e onSuccess, useLogs(limit=100)).

The long term solution would be to implement a backend feature to be able to paginate logs as well as a read order (start to end, end to start), so we could read the most recent 100 lines of the log file and return that directly, I don't think we support that yet cc: @jason810496

[jason810496]

The long term solution would be to implement a backend feature to be able to paginate logs as well as a read order (start to end, end to start), so we could read the most recent 100 lines of the log file and return that directly, I don't think we support that yet cc: @jason810496

It could be possible, but the behavior will be quite heavy for API server. Since even we specify the "end to start" at the RestAPI level, the FileTaskHandler will still need to read from the beginning of the log stream then do the interleave and sorting stuff, and finally drop the output stream until "end to start" position before returning as API response.

From the API server aspect, it would be better to just have one Streaming API Call to continuously stream the logs with #54552 fix.

[dheerajturaga]

If you try to do this with tasks that have big logs (10M) the collapsing will help, but after expanding a couple of them, the page still becomes unresponsive. (Without this, it's unresponsive/crash on load).

Yes, as mentionned by @guan404ming ideally I think on the 'overview' we only want to show a log sample, maybe the last 100 lines of the logs to be able to debug most cases, if not enough they will need to go specifically check the TI logs on the TI page to load full content. For now we can just do this truncation in the frontend. (for instance custom hook wrapping useLogs and truncating response based on a param right after the backend response is received, i.e onSuccess, useLogs(limit=100)).

The long term solution would be to implement a backend feature to be able to paginate logs as well as a read order (start to end, end to start), so we could read the most recent 100 lines of the log file and return that directly, I don't think we support that yet cc: @jason810496

@pierrejeambrun , I have updated this to useLogs(limit=100) this should prevent the dag page ending up in a bad state.

[pierrejeambrun]

It could be possible, but the behavior will be quite heavy for API server. Since even we specify the "end to start" at the RestAPI level, the FileTaskHandler will still need to read from the beginning of the log stream then do the interleave and sorting stuff, and finally drop the output stream until "end to start" position before returning as API response.

Oh I didn't know that. I was naively thinking the log reading similarly to a file handler / file descriptor and that we we would be able to seek/tail the fail appropriately.

[pierrejeambrun]

A few suggestions before we can merge. Looking good otherwise.

[dheerajturaga]

@pierrejeambrun Thanks for the review! I updated the PR with your suggestions. Let me know if this looks good

[dheerajturaga]

Had to rebase to fix static check fails that was introduced and fixed on main by some other PRs

[pierrejeambrun]

Thanks, ready to merge.

[pierrejeambrun]

Backport will fail, need a manual one

[github-actions]

Backport failed to create: v3-0-test. View the failure log Run details

Status	Branch	Result
❌	v3-0-test

You can attempt to backport this manually by running:

cherry_picker 0578598 v3-0-test

This should apply the commit to the v3-0-test branch and leave the commit in conflict state marking
the files that need manual conflict resolution.

After you have resolved the conflicts, you can continue the backport process by running:

cherry_picker --continue

[pierrejeambrun]

Backport is not straight forward. This is not critical, marking for 3.1.0 which is just around the corner.

[dheerajturaga]

This PR is an attempt to mitigate #52916. It does not "fix" the issue but prevents it from happening on the dag landing page.

• Recent failed task logs now start collapsed by default
• Added "Expand Logs"/"Hide Logs" toggle buttons with translations
• Implemented lazy loading - logs only fetch when expanded
• Prevents page crashes from large log content rendering
• Increased log container max height from 100px to 200px

Minified.React.Error.mp4

[dheerajturaga]

@potiuk , @pierrejeambrun I have this work around that will prevent the issue from happening on the DAG landing page. Please let me know if this is good.

[bbovenzi]

Let's move over to show/hide terminology and avoid the confusion with expand/collapse log groups. Otherwise lgtm

[dheerajturaga]

Let's move over to show/hide terminology and avoid the confusion with expand/collapse log groups. Otherwise lgtm

Done! Ive made the changes. Please let me know if any other update is needed

[bbovenzi]

I wonder if we still need this with #54462?

[dheerajturaga]

@bbovenzi , glad the original issue was resolved. I still think having these collapsed by default is useful incase there are dags with too many task failures. Given this is the default landing page for the dag its worth keeping the load times on this page fast

[guan404ming]

glad the original issue was resolved. I still think having these collapsed by default is useful incase there are dags with too many task failures. Given this is the default landing page for the dag its worth keeping the load times on this page fast

I think it would be clearer to show the error log directly instead of requiring an extra click. But I agree it could slow down load times if there are many error logs. How about limiting the number of displayed logs in overview page instead?

[dheerajturaga]

glad the original issue was resolved. I still think having these collapsed by default is useful incase there are dags with too many task failures. Given this is the default landing page for the dag its worth keeping the load times on this page fast

I think it would be clearer to show the error log directly instead of requiring an extra click. But I agree it could slow down load times if there are many error logs. How about limiting the number of displayed logs in overview page instead?

Im not sure about that aswell. Say we limit to 5 failed tasks and have them expanded by default. If the user is interested in the 6th task that isn't displayed here, they would still have to make multiple clicks to get the desired error log. Having them all available in the landing page but collapsed for performance seems to be better IMO.

Open to feedback here.

[pierrejeambrun]

If you try to do this with tasks that have big logs (10M) the collapsing will help, but after expanding a couple of them, the page still becomes unresponsive. (Without this, it's unresponsive/crash on load).

Yes, as mentionned by @guan404ming ideally I think on the 'overview' we only want to show a log sample, maybe the last 100 lines of the logs to be able to debug most cases, if not enough they will need to go specifically check the TI logs on the TI page to load full content. For now we can just do this truncation in the frontend. (for instance custom hook wrapping useLogs and truncating response based on a param right after the backend response is received, i.e onSuccess, useLogs(limit=100)).

The long term solution would be to implement a backend feature to be able to paginate logs as well as a read order (start to end, end to start), so we could read the most recent 100 lines of the log file and return that directly, I don't think we support that yet cc: @jason810496

[jason810496]

The long term solution would be to implement a backend feature to be able to paginate logs as well as a read order (start to end, end to start), so we could read the most recent 100 lines of the log file and return that directly, I don't think we support that yet cc: @jason810496

It could be possible, but the behavior will be quite heavy for API server. Since even we specify the "end to start" at the RestAPI level, the FileTaskHandler will still need to read from the beginning of the log stream then do the interleave and sorting stuff, and finally drop the output stream until "end to start" position before returning as API response.

From the API server aspect, it would be better to just have one Streaming API Call to continuously stream the logs with #54552 fix.

[dheerajturaga]

If you try to do this with tasks that have big logs (10M) the collapsing will help, but after expanding a couple of them, the page still becomes unresponsive. (Without this, it's unresponsive/crash on load).

Yes, as mentionned by @guan404ming ideally I think on the 'overview' we only want to show a log sample, maybe the last 100 lines of the logs to be able to debug most cases, if not enough they will need to go specifically check the TI logs on the TI page to load full content. For now we can just do this truncation in the frontend. (for instance custom hook wrapping useLogs and truncating response based on a param right after the backend response is received, i.e onSuccess, useLogs(limit=100)).

The long term solution would be to implement a backend feature to be able to paginate logs as well as a read order (start to end, end to start), so we could read the most recent 100 lines of the log file and return that directly, I don't think we support that yet cc: @jason810496

@pierrejeambrun , I have updated this to useLogs(limit=100) this should prevent the dag page ending up in a bad state.

[pierrejeambrun]

It could be possible, but the behavior will be quite heavy for API server. Since even we specify the "end to start" at the RestAPI level, the FileTaskHandler will still need to read from the beginning of the log stream then do the interleave and sorting stuff, and finally drop the output stream until "end to start" position before returning as API response.

Oh I didn't know that. I was naively thinking the log reading similarly to a file handler / file descriptor and that we we would be able to seek/tail the fail appropriately.

[pierrejeambrun]

A few suggestions before we can merge. Looking good otherwise.

[dheerajturaga]

@pierrejeambrun Thanks for the review! I updated the PR with your suggestions. Let me know if this looks good

[dheerajturaga]

Had to rebase to fix static check fails that was introduced and fixed on main by some other PRs

[pierrejeambrun]

Thanks, ready to merge.

[pierrejeambrun]

Backport will fail, need a manual one

[github-actions]

Backport failed to create: v3-0-test. View the failure log Run details

Status	Branch	Result
❌	v3-0-test

You can attempt to backport this manually by running:

cherry_picker 0578598 v3-0-test

This should apply the commit to the v3-0-test branch and leave the commit in conflict state marking
the files that need manual conflict resolution.

After you have resolved the conflicts, you can continue the backport process by running:

cherry_picker --continue

[pierrejeambrun]

Backport is not straight forward. This is not critical, marking for 3.1.0 which is just around the corner.

[dheerajturaga]

This PR is an attempt to mitigate #52916. It does not "fix" the issue but prevents it from happening on the dag landing page.

• Recent failed task logs now start collapsed by default
• Added "Expand Logs"/"Hide Logs" toggle buttons with translations
• Implemented lazy loading - logs only fetch when expanded
• Prevents page crashes from large log content rendering
• Increased log container max height from 100px to 200px

Minified.React.Error.mp4

[dheerajturaga]

@potiuk , @pierrejeambrun I have this work around that will prevent the issue from happening on the DAG landing page. Please let me know if this is good.

[bbovenzi]

Let's move over to show/hide terminology and avoid the confusion with expand/collapse log groups. Otherwise lgtm

[dheerajturaga]

Let's move over to show/hide terminology and avoid the confusion with expand/collapse log groups. Otherwise lgtm

Done! Ive made the changes. Please let me know if any other update is needed

[bbovenzi]

I wonder if we still need this with #54462?

[dheerajturaga]

@bbovenzi , glad the original issue was resolved. I still think having these collapsed by default is useful incase there are dags with too many task failures. Given this is the default landing page for the dag its worth keeping the load times on this page fast

[guan404ming]

glad the original issue was resolved. I still think having these collapsed by default is useful incase there are dags with too many task failures. Given this is the default landing page for the dag its worth keeping the load times on this page fast

I think it would be clearer to show the error log directly instead of requiring an extra click. But I agree it could slow down load times if there are many error logs. How about limiting the number of displayed logs in overview page instead?

[dheerajturaga]

glad the original issue was resolved. I still think having these collapsed by default is useful incase there are dags with too many task failures. Given this is the default landing page for the dag its worth keeping the load times on this page fast

I think it would be clearer to show the error log directly instead of requiring an extra click. But I agree it could slow down load times if there are many error logs. How about limiting the number of displayed logs in overview page instead?

Im not sure about that aswell. Say we limit to 5 failed tasks and have them expanded by default. If the user is interested in the 6th task that isn't displayed here, they would still have to make multiple clicks to get the desired error log. Having them all available in the landing page but collapsed for performance seems to be better IMO.

Open to feedback here.

[pierrejeambrun]

If you try to do this with tasks that have big logs (10M) the collapsing will help, but after expanding a couple of them, the page still becomes unresponsive. (Without this, it's unresponsive/crash on load).

Yes, as mentionned by @guan404ming ideally I think on the 'overview' we only want to show a log sample, maybe the last 100 lines of the logs to be able to debug most cases, if not enough they will need to go specifically check the TI logs on the TI page to load full content. For now we can just do this truncation in the frontend. (for instance custom hook wrapping useLogs and truncating response based on a param right after the backend response is received, i.e onSuccess, useLogs(limit=100)).

The long term solution would be to implement a backend feature to be able to paginate logs as well as a read order (start to end, end to start), so we could read the most recent 100 lines of the log file and return that directly, I don't think we support that yet cc: @jason810496

[jason810496]

The long term solution would be to implement a backend feature to be able to paginate logs as well as a read order (start to end, end to start), so we could read the most recent 100 lines of the log file and return that directly, I don't think we support that yet cc: @jason810496

It could be possible, but the behavior will be quite heavy for API server. Since even we specify the "end to start" at the RestAPI level, the FileTaskHandler will still need to read from the beginning of the log stream then do the interleave and sorting stuff, and finally drop the output stream until "end to start" position before returning as API response.

From the API server aspect, it would be better to just have one Streaming API Call to continuously stream the logs with #54552 fix.

[dheerajturaga]

If you try to do this with tasks that have big logs (10M) the collapsing will help, but after expanding a couple of them, the page still becomes unresponsive. (Without this, it's unresponsive/crash on load).

Yes, as mentionned by @guan404ming ideally I think on the 'overview' we only want to show a log sample, maybe the last 100 lines of the logs to be able to debug most cases, if not enough they will need to go specifically check the TI logs on the TI page to load full content. For now we can just do this truncation in the frontend. (for instance custom hook wrapping useLogs and truncating response based on a param right after the backend response is received, i.e onSuccess, useLogs(limit=100)).

The long term solution would be to implement a backend feature to be able to paginate logs as well as a read order (start to end, end to start), so we could read the most recent 100 lines of the log file and return that directly, I don't think we support that yet cc: @jason810496

@pierrejeambrun , I have updated this to useLogs(limit=100) this should prevent the dag page ending up in a bad state.

[pierrejeambrun]

It could be possible, but the behavior will be quite heavy for API server. Since even we specify the "end to start" at the RestAPI level, the FileTaskHandler will still need to read from the beginning of the log stream then do the interleave and sorting stuff, and finally drop the output stream until "end to start" position before returning as API response.

Oh I didn't know that. I was naively thinking the log reading similarly to a file handler / file descriptor and that we we would be able to seek/tail the fail appropriately.

[pierrejeambrun]

A few suggestions before we can merge. Looking good otherwise.

[dheerajturaga]

@pierrejeambrun Thanks for the review! I updated the PR with your suggestions. Let me know if this looks good

[dheerajturaga]

Had to rebase to fix static check fails that was introduced and fixed on main by some other PRs

[pierrejeambrun]

Thanks, ready to merge.

[pierrejeambrun]

Backport will fail, need a manual one

[github-actions]

Backport failed to create: v3-0-test. View the failure log Run details

Status	Branch	Result
❌	v3-0-test

You can attempt to backport this manually by running:

cherry_picker 0578598 v3-0-test

This should apply the commit to the v3-0-test branch and leave the commit in conflict state marking
the files that need manual conflict resolution.

After you have resolved the conflicts, you can continue the backport process by running:

cherry_picker --continue

[pierrejeambrun]

Backport is not straight forward. This is not critical, marking for 3.1.0 which is just around the corner.

[dheerajturaga]

This PR is an attempt to mitigate #52916. It does not "fix" the issue but prevents it from happening on the dag landing page.

• Recent failed task logs now start collapsed by default
• Added "Expand Logs"/"Hide Logs" toggle buttons with translations
• Implemented lazy loading - logs only fetch when expanded
• Prevents page crashes from large log content rendering
• Increased log container max height from 100px to 200px

Minified.React.Error.mp4

[dheerajturaga]

@potiuk , @pierrejeambrun I have this work around that will prevent the issue from happening on the DAG landing page. Please let me know if this is good.

[bbovenzi]

Let's move over to show/hide terminology and avoid the confusion with expand/collapse log groups. Otherwise lgtm

[dheerajturaga]

Let's move over to show/hide terminology and avoid the confusion with expand/collapse log groups. Otherwise lgtm

Done! Ive made the changes. Please let me know if any other update is needed

[bbovenzi]

I wonder if we still need this with #54462?

[dheerajturaga]

@bbovenzi , glad the original issue was resolved. I still think having these collapsed by default is useful incase there are dags with too many task failures. Given this is the default landing page for the dag its worth keeping the load times on this page fast

[guan404ming]

glad the original issue was resolved. I still think having these collapsed by default is useful incase there are dags with too many task failures. Given this is the default landing page for the dag its worth keeping the load times on this page fast

I think it would be clearer to show the error log directly instead of requiring an extra click. But I agree it could slow down load times if there are many error logs. How about limiting the number of displayed logs in overview page instead?

[dheerajturaga]

glad the original issue was resolved. I still think having these collapsed by default is useful incase there are dags with too many task failures. Given this is the default landing page for the dag its worth keeping the load times on this page fast

I think it would be clearer to show the error log directly instead of requiring an extra click. But I agree it could slow down load times if there are many error logs. How about limiting the number of displayed logs in overview page instead?

Im not sure about that aswell. Say we limit to 5 failed tasks and have them expanded by default. If the user is interested in the 6th task that isn't displayed here, they would still have to make multiple clicks to get the desired error log. Having them all available in the landing page but collapsed for performance seems to be better IMO.

Open to feedback here.

[pierrejeambrun]

If you try to do this with tasks that have big logs (10M) the collapsing will help, but after expanding a couple of them, the page still becomes unresponsive. (Without this, it's unresponsive/crash on load).

Yes, as mentionned by @guan404ming ideally I think on the 'overview' we only want to show a log sample, maybe the last 100 lines of the logs to be able to debug most cases, if not enough they will need to go specifically check the TI logs on the TI page to load full content. For now we can just do this truncation in the frontend. (for instance custom hook wrapping useLogs and truncating response based on a param right after the backend response is received, i.e onSuccess, useLogs(limit=100)).

The long term solution would be to implement a backend feature to be able to paginate logs as well as a read order (start to end, end to start), so we could read the most recent 100 lines of the log file and return that directly, I don't think we support that yet cc: @jason810496

[jason810496]

The long term solution would be to implement a backend feature to be able to paginate logs as well as a read order (start to end, end to start), so we could read the most recent 100 lines of the log file and return that directly, I don't think we support that yet cc: @jason810496

It could be possible, but the behavior will be quite heavy for API server. Since even we specify the "end to start" at the RestAPI level, the FileTaskHandler will still need to read from the beginning of the log stream then do the interleave and sorting stuff, and finally drop the output stream until "end to start" position before returning as API response.

From the API server aspect, it would be better to just have one Streaming API Call to continuously stream the logs with #54552 fix.

[dheerajturaga]

If you try to do this with tasks that have big logs (10M) the collapsing will help, but after expanding a couple of them, the page still becomes unresponsive. (Without this, it's unresponsive/crash on load).

Yes, as mentionned by @guan404ming ideally I think on the 'overview' we only want to show a log sample, maybe the last 100 lines of the logs to be able to debug most cases, if not enough they will need to go specifically check the TI logs on the TI page to load full content. For now we can just do this truncation in the frontend. (for instance custom hook wrapping useLogs and truncating response based on a param right after the backend response is received, i.e onSuccess, useLogs(limit=100)).

The long term solution would be to implement a backend feature to be able to paginate logs as well as a read order (start to end, end to start), so we could read the most recent 100 lines of the log file and return that directly, I don't think we support that yet cc: @jason810496

@pierrejeambrun , I have updated this to useLogs(limit=100) this should prevent the dag page ending up in a bad state.

[pierrejeambrun]

It could be possible, but the behavior will be quite heavy for API server. Since even we specify the "end to start" at the RestAPI level, the FileTaskHandler will still need to read from the beginning of the log stream then do the interleave and sorting stuff, and finally drop the output stream until "end to start" position before returning as API response.

Oh I didn't know that. I was naively thinking the log reading similarly to a file handler / file descriptor and that we we would be able to seek/tail the fail appropriately.

[pierrejeambrun]

A few suggestions before we can merge. Looking good otherwise.

[dheerajturaga]

@pierrejeambrun Thanks for the review! I updated the PR with your suggestions. Let me know if this looks good

[dheerajturaga]

Had to rebase to fix static check fails that was introduced and fixed on main by some other PRs

[pierrejeambrun]

Thanks, ready to merge.

[pierrejeambrun]

Backport will fail, need a manual one

[github-actions]

Backport failed to create: v3-0-test. View the failure log Run details

Status	Branch	Result
❌	v3-0-test

You can attempt to backport this manually by running:

cherry_picker 0578598 v3-0-test

This should apply the commit to the v3-0-test branch and leave the commit in conflict state marking
the files that need manual conflict resolution.

After you have resolved the conflicts, you can continue the backport process by running:

cherry_picker --continue

[pierrejeambrun]

Backport is not straight forward. This is not critical, marking for 3.1.0 which is just around the corner.

[dheerajturaga]

This PR is an attempt to mitigate #52916. It does not "fix" the issue but prevents it from happening on the dag landing page.

• Recent failed task logs now start collapsed by default
• Added "Expand Logs"/"Hide Logs" toggle buttons with translations
• Implemented lazy loading - logs only fetch when expanded
• Prevents page crashes from large log content rendering
• Increased log container max height from 100px to 200px

Minified.React.Error.mp4

[dheerajturaga]

@potiuk , @pierrejeambrun I have this work around that will prevent the issue from happening on the DAG landing page. Please let me know if this is good.

[bbovenzi]

Let's move over to show/hide terminology and avoid the confusion with expand/collapse log groups. Otherwise lgtm

[dheerajturaga]

Let's move over to show/hide terminology and avoid the confusion with expand/collapse log groups. Otherwise lgtm

Done! Ive made the changes. Please let me know if any other update is needed

[bbovenzi]

I wonder if we still need this with #54462?

[dheerajturaga]

@bbovenzi , glad the original issue was resolved. I still think having these collapsed by default is useful incase there are dags with too many task failures. Given this is the default landing page for the dag its worth keeping the load times on this page fast

[guan404ming]

glad the original issue was resolved. I still think having these collapsed by default is useful incase there are dags with too many task failures. Given this is the default landing page for the dag its worth keeping the load times on this page fast

I think it would be clearer to show the error log directly instead of requiring an extra click. But I agree it could slow down load times if there are many error logs. How about limiting the number of displayed logs in overview page instead?

[dheerajturaga]

glad the original issue was resolved. I still think having these collapsed by default is useful incase there are dags with too many task failures. Given this is the default landing page for the dag its worth keeping the load times on this page fast

I think it would be clearer to show the error log directly instead of requiring an extra click. But I agree it could slow down load times if there are many error logs. How about limiting the number of displayed logs in overview page instead?

Im not sure about that aswell. Say we limit to 5 failed tasks and have them expanded by default. If the user is interested in the 6th task that isn't displayed here, they would still have to make multiple clicks to get the desired error log. Having them all available in the landing page but collapsed for performance seems to be better IMO.

Open to feedback here.

[pierrejeambrun]

If you try to do this with tasks that have big logs (10M) the collapsing will help, but after expanding a couple of them, the page still becomes unresponsive. (Without this, it's unresponsive/crash on load).

Yes, as mentionned by @guan404ming ideally I think on the 'overview' we only want to show a log sample, maybe the last 100 lines of the logs to be able to debug most cases, if not enough they will need to go specifically check the TI logs on the TI page to load full content. For now we can just do this truncation in the frontend. (for instance custom hook wrapping useLogs and truncating response based on a param right after the backend response is received, i.e onSuccess, useLogs(limit=100)).

The long term solution would be to implement a backend feature to be able to paginate logs as well as a read order (start to end, end to start), so we could read the most recent 100 lines of the log file and return that directly, I don't think we support that yet cc: @jason810496

[jason810496]

The long term solution would be to implement a backend feature to be able to paginate logs as well as a read order (start to end, end to start), so we could read the most recent 100 lines of the log file and return that directly, I don't think we support that yet cc: @jason810496

It could be possible, but the behavior will be quite heavy for API server. Since even we specify the "end to start" at the RestAPI level, the FileTaskHandler will still need to read from the beginning of the log stream then do the interleave and sorting stuff, and finally drop the output stream until "end to start" position before returning as API response.

From the API server aspect, it would be better to just have one Streaming API Call to continuously stream the logs with #54552 fix.

[dheerajturaga]

If you try to do this with tasks that have big logs (10M) the collapsing will help, but after expanding a couple of them, the page still becomes unresponsive. (Without this, it's unresponsive/crash on load).

Yes, as mentionned by @guan404ming ideally I think on the 'overview' we only want to show a log sample, maybe the last 100 lines of the logs to be able to debug most cases, if not enough they will need to go specifically check the TI logs on the TI page to load full content. For now we can just do this truncation in the frontend. (for instance custom hook wrapping useLogs and truncating response based on a param right after the backend response is received, i.e onSuccess, useLogs(limit=100)).

The long term solution would be to implement a backend feature to be able to paginate logs as well as a read order (start to end, end to start), so we could read the most recent 100 lines of the log file and return that directly, I don't think we support that yet cc: @jason810496

@pierrejeambrun , I have updated this to useLogs(limit=100) this should prevent the dag page ending up in a bad state.

[pierrejeambrun]

It could be possible, but the behavior will be quite heavy for API server. Since even we specify the "end to start" at the RestAPI level, the FileTaskHandler will still need to read from the beginning of the log stream then do the interleave and sorting stuff, and finally drop the output stream until "end to start" position before returning as API response.

Oh I didn't know that. I was naively thinking the log reading similarly to a file handler / file descriptor and that we we would be able to seek/tail the fail appropriately.

[pierrejeambrun]

A few suggestions before we can merge. Looking good otherwise.

[dheerajturaga]

@pierrejeambrun Thanks for the review! I updated the PR with your suggestions. Let me know if this looks good

[dheerajturaga]

Had to rebase to fix static check fails that was introduced and fixed on main by some other PRs

[pierrejeambrun]

Thanks, ready to merge.

[pierrejeambrun]

Backport will fail, need a manual one

[github-actions]

Backport failed to create: v3-0-test. View the failure log Run details

Status	Branch	Result
❌	v3-0-test

You can attempt to backport this manually by running:

cherry_picker 0578598 v3-0-test

This should apply the commit to the v3-0-test branch and leave the commit in conflict state marking
the files that need manual conflict resolution.

After you have resolved the conflicts, you can continue the backport process by running:

cherry_picker --continue

[pierrejeambrun]

Backport is not straight forward. This is not critical, marking for 3.1.0 which is just around the corner.

[dheerajturaga]

This PR is an attempt to mitigate #52916. It does not "fix" the issue but prevents it from happening on the dag landing page.

• Recent failed task logs now start collapsed by default
• Added "Expand Logs"/"Hide Logs" toggle buttons with translations
• Implemented lazy loading - logs only fetch when expanded
• Prevents page crashes from large log content rendering
• Increased log container max height from 100px to 200px

Minified.React.Error.mp4

[dheerajturaga]

@potiuk , @pierrejeambrun I have this work around that will prevent the issue from happening on the DAG landing page. Please let me know if this is good.

[bbovenzi]

Let's move over to show/hide terminology and avoid the confusion with expand/collapse log groups. Otherwise lgtm

[dheerajturaga]

Let's move over to show/hide terminology and avoid the confusion with expand/collapse log groups. Otherwise lgtm

Done! Ive made the changes. Please let me know if any other update is needed

[bbovenzi]

I wonder if we still need this with #54462?

[dheerajturaga]

@bbovenzi , glad the original issue was resolved. I still think having these collapsed by default is useful incase there are dags with too many task failures. Given this is the default landing page for the dag its worth keeping the load times on this page fast

[guan404ming]

glad the original issue was resolved. I still think having these collapsed by default is useful incase there are dags with too many task failures. Given this is the default landing page for the dag its worth keeping the load times on this page fast

I think it would be clearer to show the error log directly instead of requiring an extra click. But I agree it could slow down load times if there are many error logs. How about limiting the number of displayed logs in overview page instead?

[dheerajturaga]

glad the original issue was resolved. I still think having these collapsed by default is useful incase there are dags with too many task failures. Given this is the default landing page for the dag its worth keeping the load times on this page fast

I think it would be clearer to show the error log directly instead of requiring an extra click. But I agree it could slow down load times if there are many error logs. How about limiting the number of displayed logs in overview page instead?

Im not sure about that aswell. Say we limit to 5 failed tasks and have them expanded by default. If the user is interested in the 6th task that isn't displayed here, they would still have to make multiple clicks to get the desired error log. Having them all available in the landing page but collapsed for performance seems to be better IMO.

Open to feedback here.

[pierrejeambrun]

If you try to do this with tasks that have big logs (10M) the collapsing will help, but after expanding a couple of them, the page still becomes unresponsive. (Without this, it's unresponsive/crash on load).

Yes, as mentionned by @guan404ming ideally I think on the 'overview' we only want to show a log sample, maybe the last 100 lines of the logs to be able to debug most cases, if not enough they will need to go specifically check the TI logs on the TI page to load full content. For now we can just do this truncation in the frontend. (for instance custom hook wrapping useLogs and truncating response based on a param right after the backend response is received, i.e onSuccess, useLogs(limit=100)).

The long term solution would be to implement a backend feature to be able to paginate logs as well as a read order (start to end, end to start), so we could read the most recent 100 lines of the log file and return that directly, I don't think we support that yet cc: @jason810496

[jason810496]

The long term solution would be to implement a backend feature to be able to paginate logs as well as a read order (start to end, end to start), so we could read the most recent 100 lines of the log file and return that directly, I don't think we support that yet cc: @jason810496

It could be possible, but the behavior will be quite heavy for API server. Since even we specify the "end to start" at the RestAPI level, the FileTaskHandler will still need to read from the beginning of the log stream then do the interleave and sorting stuff, and finally drop the output stream until "end to start" position before returning as API response.

From the API server aspect, it would be better to just have one Streaming API Call to continuously stream the logs with #54552 fix.

[dheerajturaga]

If you try to do this with tasks that have big logs (10M) the collapsing will help, but after expanding a couple of them, the page still becomes unresponsive. (Without this, it's unresponsive/crash on load).

Yes, as mentionned by @guan404ming ideally I think on the 'overview' we only want to show a log sample, maybe the last 100 lines of the logs to be able to debug most cases, if not enough they will need to go specifically check the TI logs on the TI page to load full content. For now we can just do this truncation in the frontend. (for instance custom hook wrapping useLogs and truncating response based on a param right after the backend response is received, i.e onSuccess, useLogs(limit=100)).

The long term solution would be to implement a backend feature to be able to paginate logs as well as a read order (start to end, end to start), so we could read the most recent 100 lines of the log file and return that directly, I don't think we support that yet cc: @jason810496

@pierrejeambrun , I have updated this to useLogs(limit=100) this should prevent the dag page ending up in a bad state.

[pierrejeambrun]

It could be possible, but the behavior will be quite heavy for API server. Since even we specify the "end to start" at the RestAPI level, the FileTaskHandler will still need to read from the beginning of the log stream then do the interleave and sorting stuff, and finally drop the output stream until "end to start" position before returning as API response.

Oh I didn't know that. I was naively thinking the log reading similarly to a file handler / file descriptor and that we we would be able to seek/tail the fail appropriately.

[pierrejeambrun]

A few suggestions before we can merge. Looking good otherwise.

[dheerajturaga]

@pierrejeambrun Thanks for the review! I updated the PR with your suggestions. Let me know if this looks good

[dheerajturaga]

Had to rebase to fix static check fails that was introduced and fixed on main by some other PRs

[pierrejeambrun]

Thanks, ready to merge.

[pierrejeambrun]

Backport will fail, need a manual one

[github-actions]

Backport failed to create: v3-0-test. View the failure log Run details

Status	Branch	Result
❌	v3-0-test

You can attempt to backport this manually by running:

cherry_picker 0578598 v3-0-test

This should apply the commit to the v3-0-test branch and leave the commit in conflict state marking
the files that need manual conflict resolution.

After you have resolved the conflicts, you can continue the backport process by running:

cherry_picker --continue

[pierrejeambrun]

Backport is not straight forward. This is not critical, marking for 3.1.0 which is just around the corner.

[dheerajturaga]

This PR is an attempt to mitigate #52916. It does not "fix" the issue but prevents it from happening on the dag landing page.

• Recent failed task logs now start collapsed by default
• Added "Expand Logs"/"Hide Logs" toggle buttons with translations
• Implemented lazy loading - logs only fetch when expanded
• Prevents page crashes from large log content rendering
• Increased log container max height from 100px to 200px

Minified.React.Error.mp4

[dheerajturaga]

@potiuk , @pierrejeambrun I have this work around that will prevent the issue from happening on the DAG landing page. Please let me know if this is good.

[bbovenzi]

Let's move over to show/hide terminology and avoid the confusion with expand/collapse log groups. Otherwise lgtm

[dheerajturaga]

Let's move over to show/hide terminology and avoid the confusion with expand/collapse log groups. Otherwise lgtm

Done! Ive made the changes. Please let me know if any other update is needed

[bbovenzi]

I wonder if we still need this with #54462?

[dheerajturaga]

@bbovenzi , glad the original issue was resolved. I still think having these collapsed by default is useful incase there are dags with too many task failures. Given this is the default landing page for the dag its worth keeping the load times on this page fast

[guan404ming]

glad the original issue was resolved. I still think having these collapsed by default is useful incase there are dags with too many task failures. Given this is the default landing page for the dag its worth keeping the load times on this page fast

I think it would be clearer to show the error log directly instead of requiring an extra click. But I agree it could slow down load times if there are many error logs. How about limiting the number of displayed logs in overview page instead?

[dheerajturaga]

glad the original issue was resolved. I still think having these collapsed by default is useful incase there are dags with too many task failures. Given this is the default landing page for the dag its worth keeping the load times on this page fast

I think it would be clearer to show the error log directly instead of requiring an extra click. But I agree it could slow down load times if there are many error logs. How about limiting the number of displayed logs in overview page instead?

Im not sure about that aswell. Say we limit to 5 failed tasks and have them expanded by default. If the user is interested in the 6th task that isn't displayed here, they would still have to make multiple clicks to get the desired error log. Having them all available in the landing page but collapsed for performance seems to be better IMO.

Open to feedback here.

[pierrejeambrun]

If you try to do this with tasks that have big logs (10M) the collapsing will help, but after expanding a couple of them, the page still becomes unresponsive. (Without this, it's unresponsive/crash on load).

Yes, as mentionned by @guan404ming ideally I think on the 'overview' we only want to show a log sample, maybe the last 100 lines of the logs to be able to debug most cases, if not enough they will need to go specifically check the TI logs on the TI page to load full content. For now we can just do this truncation in the frontend. (for instance custom hook wrapping useLogs and truncating response based on a param right after the backend response is received, i.e onSuccess, useLogs(limit=100)).

The long term solution would be to implement a backend feature to be able to paginate logs as well as a read order (start to end, end to start), so we could read the most recent 100 lines of the log file and return that directly, I don't think we support that yet cc: @jason810496

[jason810496]

The long term solution would be to implement a backend feature to be able to paginate logs as well as a read order (start to end, end to start), so we could read the most recent 100 lines of the log file and return that directly, I don't think we support that yet cc: @jason810496

It could be possible, but the behavior will be quite heavy for API server. Since even we specify the "end to start" at the RestAPI level, the FileTaskHandler will still need to read from the beginning of the log stream then do the interleave and sorting stuff, and finally drop the output stream until "end to start" position before returning as API response.

From the API server aspect, it would be better to just have one Streaming API Call to continuously stream the logs with #54552 fix.

[dheerajturaga]

If you try to do this with tasks that have big logs (10M) the collapsing will help, but after expanding a couple of them, the page still becomes unresponsive. (Without this, it's unresponsive/crash on load).

Yes, as mentionned by @guan404ming ideally I think on the 'overview' we only want to show a log sample, maybe the last 100 lines of the logs to be able to debug most cases, if not enough they will need to go specifically check the TI logs on the TI page to load full content. For now we can just do this truncation in the frontend. (for instance custom hook wrapping useLogs and truncating response based on a param right after the backend response is received, i.e onSuccess, useLogs(limit=100)).

The long term solution would be to implement a backend feature to be able to paginate logs as well as a read order (start to end, end to start), so we could read the most recent 100 lines of the log file and return that directly, I don't think we support that yet cc: @jason810496

@pierrejeambrun , I have updated this to useLogs(limit=100) this should prevent the dag page ending up in a bad state.

[pierrejeambrun]

It could be possible, but the behavior will be quite heavy for API server. Since even we specify the "end to start" at the RestAPI level, the FileTaskHandler will still need to read from the beginning of the log stream then do the interleave and sorting stuff, and finally drop the output stream until "end to start" position before returning as API response.

Oh I didn't know that. I was naively thinking the log reading similarly to a file handler / file descriptor and that we we would be able to seek/tail the fail appropriately.

[pierrejeambrun]

A few suggestions before we can merge. Looking good otherwise.

[dheerajturaga]

@pierrejeambrun Thanks for the review! I updated the PR with your suggestions. Let me know if this looks good

[dheerajturaga]

Had to rebase to fix static check fails that was introduced and fixed on main by some other PRs

[pierrejeambrun]

Thanks, ready to merge.

[pierrejeambrun]

Backport will fail, need a manual one

[github-actions]

Backport failed to create: v3-0-test. View the failure log Run details

Status	Branch	Result
❌	v3-0-test

You can attempt to backport this manually by running:

cherry_picker 0578598 v3-0-test

This should apply the commit to the v3-0-test branch and leave the commit in conflict state marking
the files that need manual conflict resolution.

After you have resolved the conflicts, you can continue the backport process by running:

cherry_picker --continue

[pierrejeambrun]

Backport is not straight forward. This is not critical, marking for 3.1.0 which is just around the corner.

[dheerajturaga]

This PR is an attempt to mitigate #52916. It does not "fix" the issue but prevents it from happening on the dag landing page.

• Recent failed task logs now start collapsed by default
• Added "Expand Logs"/"Hide Logs" toggle buttons with translations
• Implemented lazy loading - logs only fetch when expanded
• Prevents page crashes from large log content rendering
• Increased log container max height from 100px to 200px

Minified.React.Error.mp4

[dheerajturaga]

@potiuk , @pierrejeambrun I have this work around that will prevent the issue from happening on the DAG landing page. Please let me know if this is good.

[bbovenzi]

Let's move over to show/hide terminology and avoid the confusion with expand/collapse log groups. Otherwise lgtm

[dheerajturaga]

Let's move over to show/hide terminology and avoid the confusion with expand/collapse log groups. Otherwise lgtm

Done! Ive made the changes. Please let me know if any other update is needed

[bbovenzi]

I wonder if we still need this with #54462?

[dheerajturaga]

@bbovenzi , glad the original issue was resolved. I still think having these collapsed by default is useful incase there are dags with too many task failures. Given this is the default landing page for the dag its worth keeping the load times on this page fast

[guan404ming]

glad the original issue was resolved. I still think having these collapsed by default is useful incase there are dags with too many task failures. Given this is the default landing page for the dag its worth keeping the load times on this page fast

I think it would be clearer to show the error log directly instead of requiring an extra click. But I agree it could slow down load times if there are many error logs. How about limiting the number of displayed logs in overview page instead?

[dheerajturaga]

glad the original issue was resolved. I still think having these collapsed by default is useful incase there are dags with too many task failures. Given this is the default landing page for the dag its worth keeping the load times on this page fast

I think it would be clearer to show the error log directly instead of requiring an extra click. But I agree it could slow down load times if there are many error logs. How about limiting the number of displayed logs in overview page instead?

Im not sure about that aswell. Say we limit to 5 failed tasks and have them expanded by default. If the user is interested in the 6th task that isn't displayed here, they would still have to make multiple clicks to get the desired error log. Having them all available in the landing page but collapsed for performance seems to be better IMO.

Open to feedback here.

[pierrejeambrun]

If you try to do this with tasks that have big logs (10M) the collapsing will help, but after expanding a couple of them, the page still becomes unresponsive. (Without this, it's unresponsive/crash on load).

Yes, as mentionned by @guan404ming ideally I think on the 'overview' we only want to show a log sample, maybe the last 100 lines of the logs to be able to debug most cases, if not enough they will need to go specifically check the TI logs on the TI page to load full content. For now we can just do this truncation in the frontend. (for instance custom hook wrapping useLogs and truncating response based on a param right after the backend response is received, i.e onSuccess, useLogs(limit=100)).

The long term solution would be to implement a backend feature to be able to paginate logs as well as a read order (start to end, end to start), so we could read the most recent 100 lines of the log file and return that directly, I don't think we support that yet cc: @jason810496

[jason810496]

The long term solution would be to implement a backend feature to be able to paginate logs as well as a read order (start to end, end to start), so we could read the most recent 100 lines of the log file and return that directly, I don't think we support that yet cc: @jason810496

It could be possible, but the behavior will be quite heavy for API server. Since even we specify the "end to start" at the RestAPI level, the FileTaskHandler will still need to read from the beginning of the log stream then do the interleave and sorting stuff, and finally drop the output stream until "end to start" position before returning as API response.

From the API server aspect, it would be better to just have one Streaming API Call to continuously stream the logs with #54552 fix.

[dheerajturaga]

If you try to do this with tasks that have big logs (10M) the collapsing will help, but after expanding a couple of them, the page still becomes unresponsive. (Without this, it's unresponsive/crash on load).

Yes, as mentionned by @guan404ming ideally I think on the 'overview' we only want to show a log sample, maybe the last 100 lines of the logs to be able to debug most cases, if not enough they will need to go specifically check the TI logs on the TI page to load full content. For now we can just do this truncation in the frontend. (for instance custom hook wrapping useLogs and truncating response based on a param right after the backend response is received, i.e onSuccess, useLogs(limit=100)).

The long term solution would be to implement a backend feature to be able to paginate logs as well as a read order (start to end, end to start), so we could read the most recent 100 lines of the log file and return that directly, I don't think we support that yet cc: @jason810496

@pierrejeambrun , I have updated this to useLogs(limit=100) this should prevent the dag page ending up in a bad state.

[pierrejeambrun]

It could be possible, but the behavior will be quite heavy for API server. Since even we specify the "end to start" at the RestAPI level, the FileTaskHandler will still need to read from the beginning of the log stream then do the interleave and sorting stuff, and finally drop the output stream until "end to start" position before returning as API response.

Oh I didn't know that. I was naively thinking the log reading similarly to a file handler / file descriptor and that we we would be able to seek/tail the fail appropriately.

[pierrejeambrun]

A few suggestions before we can merge. Looking good otherwise.

[dheerajturaga]

@pierrejeambrun Thanks for the review! I updated the PR with your suggestions. Let me know if this looks good

[dheerajturaga]

Had to rebase to fix static check fails that was introduced and fixed on main by some other PRs

[pierrejeambrun]

Thanks, ready to merge.

[pierrejeambrun]

Backport will fail, need a manual one

[github-actions]

Backport failed to create: v3-0-test. View the failure log Run details

Status	Branch	Result
❌	v3-0-test

You can attempt to backport this manually by running:

cherry_picker 0578598 v3-0-test

This should apply the commit to the v3-0-test branch and leave the commit in conflict state marking
the files that need manual conflict resolution.

After you have resolved the conflicts, you can continue the backport process by running:

cherry_picker --continue

[pierrejeambrun]

Backport is not straight forward. This is not critical, marking for 3.1.0 which is just around the corner.