You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "tirkarthi (via GitHub)" <gi...@apache.org> on 2024/12/23 16:27:43 UTC
[PR] Add full screen view for logs [airflow]
tirkarthi opened a new pull request, #45183:
URL: https://github.com/apache/airflow/pull/45183
Related https://github.com/apache/airflow/issues/44663#issuecomment-2520512667
It will be useful to have logs open in full screen view to read the logs. Currently the tab and task details are still present on scroll taking more space. Legacy logs page had more screen space which was useful for our users internally. The implementation is simple that uses dialog to display the same code block with wrap and other styles applied in full screen mode.
Notes for reviewer and self :
* I would opt for removing Dialog.Header section too to get more space or replace log with some other useful information.
* Using an IconButton but can change to button with text "Full Screen" for uniformity.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add full screen view for logs [airflow]
Posted by "tirkarthi (via GitHub)" <gi...@apache.org>.
tirkarthi commented on code in PR #45183:
URL: https://github.com/apache/airflow/pull/45183#discussion_r1896905325
##########
airflow/ui/src/pages/TaskInstance/Logs.tsx:
##########
@@ -110,6 +136,26 @@ export const Logs = () => {
<Code overflow="auto" py={3} textWrap={wrap ? "pre" : "nowrap"}>
<VStack alignItems="flex-start">{data.parsedLogs}</VStack>
</Code>
+ <Dialog.Root
+ onOpenChange={onOpenChange}
+ open={fullscreen}
+ scrollBehavior="inside"
+ size="full"
+ >
+ <Dialog.Content backdrop>
+ <Dialog.Header>
+ <Heading size="xl">Log</Heading>
+ </Dialog.Header>
+
+ <Dialog.CloseTrigger />
+
+ <Dialog.Body>
Review Comment:
I tried copy pasting code to try this out. IMO it still takes up some space with tries, I would prefer selecting the log and then clicking on the fullscreen to utilise the screen space fully.
Full screen with task tries and wrap
Fullscreen without dialog header
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add full screen view for logs [airflow]
Posted by "tirkarthi (via GitHub)" <gi...@apache.org>.
tirkarthi commented on code in PR #45183:
URL: https://github.com/apache/airflow/pull/45183#discussion_r1897425445
##########
airflow/ui/src/pages/TaskInstance/Logs.tsx:
##########
@@ -110,6 +136,26 @@ export const Logs = () => {
<Code overflow="auto" py={3} textWrap={wrap ? "pre" : "nowrap"}>
<VStack alignItems="flex-start">{data.parsedLogs}</VStack>
</Code>
+ <Dialog.Root
+ onOpenChange={onOpenChange}
+ open={fullscreen}
+ scrollBehavior="inside"
+ size="full"
+ >
+ <Dialog.Content backdrop>
+ <Dialog.Header>
+ <Heading size="xl">Log</Heading>
+ </Dialog.Header>
+
+ <Dialog.CloseTrigger />
+
+ <Dialog.Body>
Review Comment:
Thanks @bbovenzi and @pierrejeambrun for the feedback. It also makes it consistent with other pages if this is the intended design. I will wait for @bbovenzi comment to see if there is any way to do this in react since without a component or if a component is the best way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add full screen view for logs [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on code in PR #45183:
URL: https://github.com/apache/airflow/pull/45183#discussion_r1895981951
##########
airflow/ui/src/pages/TaskInstance/Logs.tsx:
##########
@@ -110,6 +136,26 @@ export const Logs = () => {
<Code overflow="auto" py={3} textWrap={wrap ? "pre" : "nowrap"}>
<VStack alignItems="flex-start">{data.parsedLogs}</VStack>
</Code>
+ <Dialog.Root
+ onOpenChange={onOpenChange}
+ open={fullscreen}
+ scrollBehavior="inside"
+ size="full"
+ >
+ <Dialog.Content backdrop>
+ <Dialog.Header>
+ <Heading size="xl">Log</Heading>
+ </Dialog.Header>
+
+ <Dialog.CloseTrigger />
+
+ <Dialog.Body>
Review Comment:
We should keep the try selector and wrap/unwrap button in the modal
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add full screen view for logs [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #45183:
URL: https://github.com/apache/airflow/pull/45183#discussion_r1905726336
##########
airflow/ui/src/pages/TaskInstance/Logs.tsx:
##########
@@ -77,26 +84,47 @@ export const Logs = () => {
return (
<Box p={2}>
- <HStack justifyContent="space-between" mb={2}>
- {taskInstance === undefined || tryNumber === undefined || taskInstance.try_number <= 1 ? (
- <div />
- ) : (
- <TaskTrySelect
- onSelectTryNumber={onSelectTryNumber}
- selectedTryNumber={tryNumber}
- taskInstance={taskInstance}
- />
- )}
- <Button aria-label={wrap ? "Unwrap" : "Wrap"} bg="bg.panel" onClick={toggleWrap} variant="outline">
- {wrap ? "Unwrap" : "Wrap"}
- </Button>
- </HStack>
+ <TaskLogHeader
+ onSelectTryNumber={onSelectTryNumber}
+ taskInstance={taskInstance}
+ toggleFullscreen={toggleFullscreen}
+ toggleWrap={toggleWrap}
+ tryNumber={tryNumber}
+ wrap={wrap}
+ />
<ErrorAlert error={error ?? logError} />
<Skeleton />
<ProgressBar size="xs" visibility={isLoading || isLoadingLogs ? "visible" : "hidden"} />
<Code overflow="auto" py={3} textWrap={wrap ? "pre" : "nowrap"}>
<VStack alignItems="flex-start">{data.parsedLogs}</VStack>
</Code>
+ <Dialog.Root onOpenChange={onOpenChange} open={fullscreen} scrollBehavior="inside" size="full">
+ <Dialog.Content backdrop>
+ <Dialog.Header>
+ <Heading size="xl">{taskId}</Heading>
+ <TaskLogHeader
+ isFullscreen
+ onSelectTryNumber={onSelectTryNumber}
+ taskInstance={taskInstance}
+ toggleFullscreen={toggleFullscreen}
+ toggleWrap={toggleWrap}
+ tryNumber={tryNumber}
+ wrap={wrap}
+ />
+ </Dialog.Header>
+
+ <Dialog.CloseTrigger />
+
+ <Dialog.Body>
+ <ErrorAlert error={error ?? logError} />
+ <Skeleton />
+ <ProgressBar size="xs" visibility={isLoading || isLoadingLogs ? "visible" : "hidden"} />
+ <Code overflow="auto" py={3} textWrap={wrap ? "pre" : "nowrap"}>
+ <VStack alignItems="flex-start">{data.parsedLogs}</VStack>
+ </Code>
Review Comment:
This should be factorized, maybe inside a `Logs/LogContent.tsx` Component. Because for now it repeats, and it will grow bigger and bigger with log parsing, and colors, and groups. We will need it at some point.
Maybe even bring in the skeleton and progressbar.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add full screen view for logs [airflow]
Posted by "tirkarthi (via GitHub)" <gi...@apache.org>.
tirkarthi commented on code in PR #45183:
URL: https://github.com/apache/airflow/pull/45183#discussion_r1903279862
##########
airflow/ui/src/pages/TaskInstance/Logs.tsx:
##########
@@ -110,6 +136,26 @@ export const Logs = () => {
<Code overflow="auto" py={3} textWrap={wrap ? "pre" : "nowrap"}>
<VStack alignItems="flex-start">{data.parsedLogs}</VStack>
</Code>
+ <Dialog.Root
+ onOpenChange={onOpenChange}
+ open={fullscreen}
+ scrollBehavior="inside"
+ size="full"
+ >
+ <Dialog.Content backdrop>
+ <Dialog.Header>
+ <Heading size="xl">Log</Heading>
+ </Dialog.Header>
+
+ <Dialog.CloseTrigger />
+
+ <Dialog.Body>
Review Comment:
Thanks @bbovenzi and @pierrejeambrun . Added task_id in the heading. Refactored the header component as suggested for reuse and updated screenshot as below.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add full screen view for logs [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun merged PR #45183:
URL: https://github.com/apache/airflow/pull/45183
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add full screen view for logs [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #45183:
URL: https://github.com/apache/airflow/pull/45183#discussion_r1905726336
##########
airflow/ui/src/pages/TaskInstance/Logs.tsx:
##########
@@ -77,26 +84,47 @@ export const Logs = () => {
return (
<Box p={2}>
- <HStack justifyContent="space-between" mb={2}>
- {taskInstance === undefined || tryNumber === undefined || taskInstance.try_number <= 1 ? (
- <div />
- ) : (
- <TaskTrySelect
- onSelectTryNumber={onSelectTryNumber}
- selectedTryNumber={tryNumber}
- taskInstance={taskInstance}
- />
- )}
- <Button aria-label={wrap ? "Unwrap" : "Wrap"} bg="bg.panel" onClick={toggleWrap} variant="outline">
- {wrap ? "Unwrap" : "Wrap"}
- </Button>
- </HStack>
+ <TaskLogHeader
+ onSelectTryNumber={onSelectTryNumber}
+ taskInstance={taskInstance}
+ toggleFullscreen={toggleFullscreen}
+ toggleWrap={toggleWrap}
+ tryNumber={tryNumber}
+ wrap={wrap}
+ />
<ErrorAlert error={error ?? logError} />
<Skeleton />
<ProgressBar size="xs" visibility={isLoading || isLoadingLogs ? "visible" : "hidden"} />
<Code overflow="auto" py={3} textWrap={wrap ? "pre" : "nowrap"}>
<VStack alignItems="flex-start">{data.parsedLogs}</VStack>
</Code>
+ <Dialog.Root onOpenChange={onOpenChange} open={fullscreen} scrollBehavior="inside" size="full">
+ <Dialog.Content backdrop>
+ <Dialog.Header>
+ <Heading size="xl">{taskId}</Heading>
+ <TaskLogHeader
+ isFullscreen
+ onSelectTryNumber={onSelectTryNumber}
+ taskInstance={taskInstance}
+ toggleFullscreen={toggleFullscreen}
+ toggleWrap={toggleWrap}
+ tryNumber={tryNumber}
+ wrap={wrap}
+ />
+ </Dialog.Header>
+
+ <Dialog.CloseTrigger />
+
+ <Dialog.Body>
+ <ErrorAlert error={error ?? logError} />
+ <Skeleton />
+ <ProgressBar size="xs" visibility={isLoading || isLoadingLogs ? "visible" : "hidden"} />
+ <Code overflow="auto" py={3} textWrap={wrap ? "pre" : "nowrap"}>
+ <VStack alignItems="flex-start">{data.parsedLogs}</VStack>
+ </Code>
Review Comment:
This should be factorized, maybe inside a `Logs/LogContent.tsx` Component. Because for now its a few lines repeated, and it will grow bigger and bigger with log parsing, colors, and collapsible log groups. We will need it at some point.
Maybe even bring in the skeleton and progress bar.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add full screen view for logs [airflow]
Posted by "tirkarthi (via GitHub)" <gi...@apache.org>.
tirkarthi commented on code in PR #45183:
URL: https://github.com/apache/airflow/pull/45183#discussion_r1896903158
##########
airflow/ui/src/pages/TaskInstance/Logs.tsx:
##########
@@ -110,6 +136,26 @@ export const Logs = () => {
<Code overflow="auto" py={3} textWrap={wrap ? "pre" : "nowrap"}>
<VStack alignItems="flex-start">{data.parsedLogs}</VStack>
</Code>
+ <Dialog.Root
+ onOpenChange={onOpenChange}
+ open={fullscreen}
+ scrollBehavior="inside"
+ size="full"
+ >
+ <Dialog.Content backdrop>
+ <Dialog.Header>
+ <Heading size="xl">Log</Heading>
+ </Dialog.Header>
+
+ <Dialog.CloseTrigger />
+
+ <Dialog.Body>
Review Comment:
Thanks, I suppose the entire content from can become a component like `<LogComponent>` and then show `FullScreen` based on the present inside the dialog or not like below. Are there any other ways to do this idiomatic in react without a separate component?
```html
<Box>
<LogComponent showFullScreenButton />
<Dialog>
<LogComponent />
</Dialog>
</Box>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add full screen view for logs [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #45183:
URL: https://github.com/apache/airflow/pull/45183#discussion_r1905726336
##########
airflow/ui/src/pages/TaskInstance/Logs.tsx:
##########
@@ -77,26 +84,47 @@ export const Logs = () => {
return (
<Box p={2}>
- <HStack justifyContent="space-between" mb={2}>
- {taskInstance === undefined || tryNumber === undefined || taskInstance.try_number <= 1 ? (
- <div />
- ) : (
- <TaskTrySelect
- onSelectTryNumber={onSelectTryNumber}
- selectedTryNumber={tryNumber}
- taskInstance={taskInstance}
- />
- )}
- <Button aria-label={wrap ? "Unwrap" : "Wrap"} bg="bg.panel" onClick={toggleWrap} variant="outline">
- {wrap ? "Unwrap" : "Wrap"}
- </Button>
- </HStack>
+ <TaskLogHeader
+ onSelectTryNumber={onSelectTryNumber}
+ taskInstance={taskInstance}
+ toggleFullscreen={toggleFullscreen}
+ toggleWrap={toggleWrap}
+ tryNumber={tryNumber}
+ wrap={wrap}
+ />
<ErrorAlert error={error ?? logError} />
<Skeleton />
<ProgressBar size="xs" visibility={isLoading || isLoadingLogs ? "visible" : "hidden"} />
<Code overflow="auto" py={3} textWrap={wrap ? "pre" : "nowrap"}>
<VStack alignItems="flex-start">{data.parsedLogs}</VStack>
</Code>
+ <Dialog.Root onOpenChange={onOpenChange} open={fullscreen} scrollBehavior="inside" size="full">
+ <Dialog.Content backdrop>
+ <Dialog.Header>
+ <Heading size="xl">{taskId}</Heading>
+ <TaskLogHeader
+ isFullscreen
+ onSelectTryNumber={onSelectTryNumber}
+ taskInstance={taskInstance}
+ toggleFullscreen={toggleFullscreen}
+ toggleWrap={toggleWrap}
+ tryNumber={tryNumber}
+ wrap={wrap}
+ />
+ </Dialog.Header>
+
+ <Dialog.CloseTrigger />
+
+ <Dialog.Body>
+ <ErrorAlert error={error ?? logError} />
+ <Skeleton />
+ <ProgressBar size="xs" visibility={isLoading || isLoadingLogs ? "visible" : "hidden"} />
+ <Code overflow="auto" py={3} textWrap={wrap ? "pre" : "nowrap"}>
+ <VStack alignItems="flex-start">{data.parsedLogs}</VStack>
+ </Code>
Review Comment:
This should be factorized, maybe inside a `LogContent` Component. Inside that same `Logs` folder. Because for now it repeats, and it will grow bigger and bigger with log parsing, and colors, and groups. We will need it at some point.
Maybe even bring in the skeleton and progressbar.
##########
airflow/ui/src/components/TaskLogHeader.tsx:
##########
@@ -0,0 +1,66 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { HStack, IconButton } from "@chakra-ui/react";
+import { MdOutlineOpenInFull } from "react-icons/md";
+
+import type { TaskInstanceResponse } from "openapi/requests/types.gen";
+import { TaskTrySelect } from "src/components/TaskTrySelect";
+import { Button } from "src/components/ui";
+
+type Props = {
+ readonly isFullscreen?: boolean;
+ readonly onSelectTryNumber: (tryNumber: number) => void;
+ readonly taskInstance?: TaskInstanceResponse;
+ readonly toggleFullscreen: () => void;
+ readonly toggleWrap: () => void;
+ readonly tryNumber?: number;
+ readonly wrap: boolean;
+};
+
+export const TaskLogHeader = ({
+ isFullscreen = false,
+ onSelectTryNumber,
+ taskInstance,
+ toggleFullscreen,
+ toggleWrap,
+ tryNumber,
+ wrap,
+}: Props) => (
Review Comment:
This component is necessary and nice to not overwhelm the `Logs` file. But I don't imagine it being reusable. This is really specific to the `Logs.tsx` one. Maybe that should be inside `Logs/index.html, Logs/TaskLogHeader.tsx`.
##########
airflow/ui/src/pages/TaskInstance/Logs.tsx:
##########
@@ -110,6 +136,26 @@ export const Logs = () => {
<Code overflow="auto" py={3} textWrap={wrap ? "pre" : "nowrap"}>
<VStack alignItems="flex-start">{data.parsedLogs}</VStack>
</Code>
+ <Dialog.Root
+ onOpenChange={onOpenChange}
+ open={fullscreen}
+ scrollBehavior="inside"
+ size="full"
+ >
+ <Dialog.Content backdrop>
+ <Dialog.Header>
+ <Heading size="xl">Log</Heading>
+ </Dialog.Header>
+
+ <Dialog.CloseTrigger />
+
+ <Dialog.Body>
Review Comment:
Nice
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add full screen view for logs [airflow]
Posted by "tirkarthi (via GitHub)" <gi...@apache.org>.
tirkarthi commented on PR #45183:
URL: https://github.com/apache/airflow/pull/45183#issuecomment-2577806752
Thanks @pierrejeambrun and @bbovenzi
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add full screen view for logs [airflow]
Posted by "tirkarthi (via GitHub)" <gi...@apache.org>.
tirkarthi commented on code in PR #45183:
URL: https://github.com/apache/airflow/pull/45183#discussion_r1906487229
##########
airflow/ui/src/components/TaskLogHeader.tsx:
##########
@@ -0,0 +1,66 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { HStack, IconButton } from "@chakra-ui/react";
+import { MdOutlineOpenInFull } from "react-icons/md";
+
+import type { TaskInstanceResponse } from "openapi/requests/types.gen";
+import { TaskTrySelect } from "src/components/TaskTrySelect";
+import { Button } from "src/components/ui";
+
+type Props = {
+ readonly isFullscreen?: boolean;
+ readonly onSelectTryNumber: (tryNumber: number) => void;
+ readonly taskInstance?: TaskInstanceResponse;
+ readonly toggleFullscreen: () => void;
+ readonly toggleWrap: () => void;
+ readonly tryNumber?: number;
+ readonly wrap: boolean;
+};
+
+export const TaskLogHeader = ({
+ isFullscreen = false,
+ onSelectTryNumber,
+ taskInstance,
+ toggleFullscreen,
+ toggleWrap,
+ tryNumber,
+ wrap,
+}: Props) => (
Review Comment:
Thanks, done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add full screen view for logs [airflow]
Posted by "tirkarthi (via GitHub)" <gi...@apache.org>.
tirkarthi commented on code in PR #45183:
URL: https://github.com/apache/airflow/pull/45183#discussion_r1906487736
##########
airflow/ui/src/pages/TaskInstance/Logs.tsx:
##########
@@ -77,26 +84,47 @@ export const Logs = () => {
return (
<Box p={2}>
- <HStack justifyContent="space-between" mb={2}>
- {taskInstance === undefined || tryNumber === undefined || taskInstance.try_number <= 1 ? (
- <div />
- ) : (
- <TaskTrySelect
- onSelectTryNumber={onSelectTryNumber}
- selectedTryNumber={tryNumber}
- taskInstance={taskInstance}
- />
- )}
- <Button aria-label={wrap ? "Unwrap" : "Wrap"} bg="bg.panel" onClick={toggleWrap} variant="outline">
- {wrap ? "Unwrap" : "Wrap"}
- </Button>
- </HStack>
+ <TaskLogHeader
+ onSelectTryNumber={onSelectTryNumber}
+ taskInstance={taskInstance}
+ toggleFullscreen={toggleFullscreen}
+ toggleWrap={toggleWrap}
+ tryNumber={tryNumber}
+ wrap={wrap}
+ />
<ErrorAlert error={error ?? logError} />
<Skeleton />
<ProgressBar size="xs" visibility={isLoading || isLoadingLogs ? "visible" : "hidden"} />
<Code overflow="auto" py={3} textWrap={wrap ? "pre" : "nowrap"}>
<VStack alignItems="flex-start">{data.parsedLogs}</VStack>
</Code>
+ <Dialog.Root onOpenChange={onOpenChange} open={fullscreen} scrollBehavior="inside" size="full">
+ <Dialog.Content backdrop>
+ <Dialog.Header>
+ <Heading size="xl">{taskId}</Heading>
+ <TaskLogHeader
+ isFullscreen
+ onSelectTryNumber={onSelectTryNumber}
+ taskInstance={taskInstance}
+ toggleFullscreen={toggleFullscreen}
+ toggleWrap={toggleWrap}
+ tryNumber={tryNumber}
+ wrap={wrap}
+ />
+ </Dialog.Header>
+
+ <Dialog.CloseTrigger />
+
+ <Dialog.Body>
+ <ErrorAlert error={error ?? logError} />
+ <Skeleton />
+ <ProgressBar size="xs" visibility={isLoading || isLoadingLogs ? "visible" : "hidden"} />
+ <Code overflow="auto" py={3} textWrap={wrap ? "pre" : "nowrap"}>
+ <VStack alignItems="flex-start">{data.parsedLogs}</VStack>
+ </Code>
Review Comment:
Thanks, refactored to `TaskLogContent.tsx` for reuse inside Logs folder.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add full screen view for logs [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on code in PR #45183:
URL: https://github.com/apache/airflow/pull/45183#discussion_r1901867434
##########
airflow/ui/src/pages/TaskInstance/Logs.tsx:
##########
@@ -110,6 +136,26 @@ export const Logs = () => {
<Code overflow="auto" py={3} textWrap={wrap ? "pre" : "nowrap"}>
<VStack alignItems="flex-start">{data.parsedLogs}</VStack>
</Code>
+ <Dialog.Root
+ onOpenChange={onOpenChange}
+ open={fullscreen}
+ scrollBehavior="inside"
+ size="full"
+ >
+ <Dialog.Content backdrop>
+ <Dialog.Header>
+ <Heading size="xl">Log</Heading>
+ </Dialog.Header>
+
+ <Dialog.CloseTrigger />
+
+ <Dialog.Body>
Review Comment:
Yes, we can make a TaskLogHeader component. But based on your screenshot, I am fine with reducing the padding so it doesn't take up too much space.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add full screen view for logs [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on PR #45183:
URL: https://github.com/apache/airflow/pull/45183#issuecomment-2566760185
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add full screen view for logs [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #45183:
URL: https://github.com/apache/airflow/pull/45183#discussion_r1897423205
##########
airflow/ui/src/pages/TaskInstance/Logs.tsx:
##########
@@ -110,6 +136,26 @@ export const Logs = () => {
<Code overflow="auto" py={3} textWrap={wrap ? "pre" : "nowrap"}>
<VStack alignItems="flex-start">{data.parsedLogs}</VStack>
</Code>
+ <Dialog.Root
+ onOpenChange={onOpenChange}
+ open={fullscreen}
+ scrollBehavior="inside"
+ size="full"
+ >
+ <Dialog.Content backdrop>
+ <Dialog.Header>
+ <Heading size="xl">Log</Heading>
+ </Dialog.Header>
+
+ <Dialog.CloseTrigger />
+
+ <Dialog.Body>
Review Comment:
+1 for keeping the header. It's taking really few real estate in the page and allowing to navigate without having to close and then open again the full screen mode.
That's also consistant with how we build the graph,grid, etc... views. (there's a header). Also maybe the Task id too would be helpful. (for the grid graph etc... we have the dag_id). I can easily imagine someone losing track of what logs he is looking at after a while reading in the full screen page. Let's imagine we have multiple opened tabs like this at the same time, it's difficult to navigate and remember about.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add full screen view for logs [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #45183:
URL: https://github.com/apache/airflow/pull/45183#discussion_r1905726336
##########
airflow/ui/src/pages/TaskInstance/Logs.tsx:
##########
@@ -77,26 +84,47 @@ export const Logs = () => {
return (
<Box p={2}>
- <HStack justifyContent="space-between" mb={2}>
- {taskInstance === undefined || tryNumber === undefined || taskInstance.try_number <= 1 ? (
- <div />
- ) : (
- <TaskTrySelect
- onSelectTryNumber={onSelectTryNumber}
- selectedTryNumber={tryNumber}
- taskInstance={taskInstance}
- />
- )}
- <Button aria-label={wrap ? "Unwrap" : "Wrap"} bg="bg.panel" onClick={toggleWrap} variant="outline">
- {wrap ? "Unwrap" : "Wrap"}
- </Button>
- </HStack>
+ <TaskLogHeader
+ onSelectTryNumber={onSelectTryNumber}
+ taskInstance={taskInstance}
+ toggleFullscreen={toggleFullscreen}
+ toggleWrap={toggleWrap}
+ tryNumber={tryNumber}
+ wrap={wrap}
+ />
<ErrorAlert error={error ?? logError} />
<Skeleton />
<ProgressBar size="xs" visibility={isLoading || isLoadingLogs ? "visible" : "hidden"} />
<Code overflow="auto" py={3} textWrap={wrap ? "pre" : "nowrap"}>
<VStack alignItems="flex-start">{data.parsedLogs}</VStack>
</Code>
+ <Dialog.Root onOpenChange={onOpenChange} open={fullscreen} scrollBehavior="inside" size="full">
+ <Dialog.Content backdrop>
+ <Dialog.Header>
+ <Heading size="xl">{taskId}</Heading>
+ <TaskLogHeader
+ isFullscreen
+ onSelectTryNumber={onSelectTryNumber}
+ taskInstance={taskInstance}
+ toggleFullscreen={toggleFullscreen}
+ toggleWrap={toggleWrap}
+ tryNumber={tryNumber}
+ wrap={wrap}
+ />
+ </Dialog.Header>
+
+ <Dialog.CloseTrigger />
+
+ <Dialog.Body>
+ <ErrorAlert error={error ?? logError} />
+ <Skeleton />
+ <ProgressBar size="xs" visibility={isLoading || isLoadingLogs ? "visible" : "hidden"} />
+ <Code overflow="auto" py={3} textWrap={wrap ? "pre" : "nowrap"}>
+ <VStack alignItems="flex-start">{data.parsedLogs}</VStack>
+ </Code>
Review Comment:
This should be factorized, maybe inside a `Logs/LogContent.tsx` Component. Because for now its a few lines repeated, and it will grow bigger and bigger with log parsing, colors, and collapsible log groups. We will need it at some point.
Maybe even bring in the skeleton and progress bar. That I can easily imagine it being reused outside of that page. When people do need the Log content codebock somewhere else, we can extract it to the shared `components` folder.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add full screen view for logs [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #45183:
URL: https://github.com/apache/airflow/pull/45183#discussion_r1905726336
##########
airflow/ui/src/pages/TaskInstance/Logs.tsx:
##########
@@ -77,26 +84,47 @@ export const Logs = () => {
return (
<Box p={2}>
- <HStack justifyContent="space-between" mb={2}>
- {taskInstance === undefined || tryNumber === undefined || taskInstance.try_number <= 1 ? (
- <div />
- ) : (
- <TaskTrySelect
- onSelectTryNumber={onSelectTryNumber}
- selectedTryNumber={tryNumber}
- taskInstance={taskInstance}
- />
- )}
- <Button aria-label={wrap ? "Unwrap" : "Wrap"} bg="bg.panel" onClick={toggleWrap} variant="outline">
- {wrap ? "Unwrap" : "Wrap"}
- </Button>
- </HStack>
+ <TaskLogHeader
+ onSelectTryNumber={onSelectTryNumber}
+ taskInstance={taskInstance}
+ toggleFullscreen={toggleFullscreen}
+ toggleWrap={toggleWrap}
+ tryNumber={tryNumber}
+ wrap={wrap}
+ />
<ErrorAlert error={error ?? logError} />
<Skeleton />
<ProgressBar size="xs" visibility={isLoading || isLoadingLogs ? "visible" : "hidden"} />
<Code overflow="auto" py={3} textWrap={wrap ? "pre" : "nowrap"}>
<VStack alignItems="flex-start">{data.parsedLogs}</VStack>
</Code>
+ <Dialog.Root onOpenChange={onOpenChange} open={fullscreen} scrollBehavior="inside" size="full">
+ <Dialog.Content backdrop>
+ <Dialog.Header>
+ <Heading size="xl">{taskId}</Heading>
+ <TaskLogHeader
+ isFullscreen
+ onSelectTryNumber={onSelectTryNumber}
+ taskInstance={taskInstance}
+ toggleFullscreen={toggleFullscreen}
+ toggleWrap={toggleWrap}
+ tryNumber={tryNumber}
+ wrap={wrap}
+ />
+ </Dialog.Header>
+
+ <Dialog.CloseTrigger />
+
+ <Dialog.Body>
+ <ErrorAlert error={error ?? logError} />
+ <Skeleton />
+ <ProgressBar size="xs" visibility={isLoading || isLoadingLogs ? "visible" : "hidden"} />
+ <Code overflow="auto" py={3} textWrap={wrap ? "pre" : "nowrap"}>
+ <VStack alignItems="flex-start">{data.parsedLogs}</VStack>
+ </Code>
Review Comment:
This should be factorized, maybe inside a `Logs/LogContent.tsx` Component. Because for now its a few lines repeated, and it will grow bigger and bigger with log parsing, colors, and collapsible log groups. We will need it at some point.
Maybe even bring in the skeleton and progress bar. That I can easily imagine it being reused outside of that page. When people do need the Log content codebock somewhere else, we can extract it to the component library.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add full screen view for logs [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #45183:
URL: https://github.com/apache/airflow/pull/45183#discussion_r1905711282
##########
airflow/ui/src/components/TaskLogHeader.tsx:
##########
@@ -0,0 +1,66 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { HStack, IconButton } from "@chakra-ui/react";
+import { MdOutlineOpenInFull } from "react-icons/md";
+
+import type { TaskInstanceResponse } from "openapi/requests/types.gen";
+import { TaskTrySelect } from "src/components/TaskTrySelect";
+import { Button } from "src/components/ui";
+
+type Props = {
+ readonly isFullscreen?: boolean;
+ readonly onSelectTryNumber: (tryNumber: number) => void;
+ readonly taskInstance?: TaskInstanceResponse;
+ readonly toggleFullscreen: () => void;
+ readonly toggleWrap: () => void;
+ readonly tryNumber?: number;
+ readonly wrap: boolean;
+};
+
+export const TaskLogHeader = ({
+ isFullscreen = false,
+ onSelectTryNumber,
+ taskInstance,
+ toggleFullscreen,
+ toggleWrap,
+ tryNumber,
+ wrap,
+}: Props) => (
Review Comment:
This component is necessary and nice to not overwhelm the `Logs` file. But I don't imagine it being reusable. This is really specific to the `Logs.tsx` one. Maybe that should be inside Logs folder and have a nested hierarchy for this complex component. `Logs/index.html, Logs/TaskLogHeader.tsx, Logs/Logs.tsx`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org