Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC - [computelogmanager] add fields to pass uri, path or shell cmd #26723

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions js_modules/dagster-ui/packages/ui-core/client.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 16 additions & 0 deletions js_modules/dagster-ui/packages/ui-core/src/graphql/types.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {Box, Colors, Icon} from '@dagster-io/ui-components';
import {Box, Colors, Icon, Tooltip, UnstyledButton} from '@dagster-io/ui-components';
import * as React from 'react';

import {RawLogContent} from './RawLogContent';
Expand All @@ -14,7 +14,9 @@ import {
CapturedLogsSubscriptionVariables,
} from './types/CapturedLogPanel.types';
import {AppContext} from '../app/AppContext';
import {showSharedToaster} from '../app/DomUtils';
import {WebSocketContext} from '../app/WebSocketProvider';
import {useCopyToClipboard} from '../app/browser';

interface CapturedLogProps {
logKey: string[];
Expand All @@ -33,28 +35,85 @@ export const CapturedOrExternalLogPanel = React.memo(
(props.visibleIOType === 'stdout'
? logCaptureInfo.externalStdoutUrl
: logCaptureInfo.externalStderrUrl);
if (externalUrl) {
const uriOrPath =
logCaptureInfo &&
(props.visibleIOType === 'stdout'
? logCaptureInfo.stdoutUriOrPath
: logCaptureInfo.stderrUriOrPath);
const shellCmd =
logCaptureInfo &&
(props.visibleIOType === 'stdout'
? logCaptureInfo.stdoutShellCmd
: logCaptureInfo.stderrShellCmd);

const copy = useCopyToClipboard();
const onClickFn = async (key: string, value: string | undefined) => {
if (!value) {
return;
}
copy(value);
await showSharedToaster({
intent: 'success',
icon: 'done',
message: `${key} string copied!`,
});
};
const onClickExternalUri = async () => onClickFn('log artefact URI', uriOrPath);
const onClickShellCmd = async () => onClickFn('shell command', shellCmd);

if (externalUrl || uriOrPath || shellCmd) {
return (
<Box
flex={{direction: 'row', alignItems: 'center', justifyContent: 'center', gap: 1}}
flex={{direction: 'column', alignItems: 'center', justifyContent: 'center', gap: 1}}
background={Colors.tooltipBackground()}
style={{color: Colors.tooltipText(), flex: 1, minHeight: 0}}
>
View logs at
<a
href={externalUrl}
target="_blank"
rel="noreferrer"
style={{
color: Colors.tooltipText(),
textDecoration: 'underline',
marginLeft: 4,
marginRight: 4,
}}
>
{externalUrl}
</a>
<Icon name="open_in_new" color={Colors.tooltipText()} size={20} style={{marginTop: 2}} />
{externalUrl ? (
<div>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this will definitely need css+html rework, i was just passing information and making the click to copy work.

when thinking about this ui, should we provide even more information, such as additional text about to use the shell command (ie, requiring a preconfigured azure cli, for example) ? I stopped short of this because I was already concerned about increasing the event size, but maybe I could add a 'log type' field that the UI could use to render that additional information without producing it from the ComputeLogManager

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's reasonable to have a log_type field or something like that, that the UI could use rather than put that information in the event body itself.

View logs at
<a
href={externalUrl}
target="_blank"
rel="noreferrer"
style={{
color: Colors.tooltipText(),
textDecoration: 'underline',
marginLeft: 4,
marginRight: 4,
}}
>
{externalUrl}
</a>
<Icon
name="open_in_new"
color={Colors.tooltipText()}
size={20}
style={{marginTop: 2}}
/>
</div>
) : undefined}

{uriOrPath ? (
<div>
Logs artefact URI:
<Tooltip content="Click to copy log artefact URI" placement="top" display="block">
<UnstyledButton onClick={onClickExternalUri}>{uriOrPath}</UnstyledButton>
</Tooltip>
</div>
) : undefined}

{shellCmd ? (
<div>
Shell command to download logs:
<Tooltip
content="Click to copy shell command to download logs"
placement="top"
display="block"
>
<UnstyledButton onClick={onClickShellCmd}>{shellCmd}</UnstyledButton>
</Tooltip>
</div>
) : undefined}
</Box>
);
}
Expand Down
4 changes: 4 additions & 0 deletions js_modules/dagster-ui/packages/ui-core/src/runs/LogsRow.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,10 @@ export const LOGS_ROW_STRUCTURED_FRAGMENT = gql`
externalUrl
externalStdoutUrl
externalStderrUrl
stdoutUriOrPath
stderrUriOrPath
stdoutShellCmd
stderrShellCmd
}
... on AssetCheckEvaluationEvent {
evaluation {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ export interface ILogCaptureInfo {
pid?: string;
externalStdoutUrl?: string;
externalStderrUrl?: string;
stdoutUriOrPath?: string;
stderrUriOrPath?: string;
stdoutShellCmd?: string;
stderrShellCmd?: string;
}

export interface IRunMetadataDict {
Expand Down Expand Up @@ -277,6 +281,10 @@ export function extractMetadataFromLogs(
pid: String(log.pid),
externalStdoutUrl: log.externalStdoutUrl || undefined,
externalStderrUrl: log.externalStderrUrl || undefined,
stdoutUriOrPath: log.stdoutUriOrPath || undefined,
stderrUriOrPath: log.stderrUriOrPath || undefined,
stdoutShellCmd: log.stdoutShellCmd || undefined,
stderrShellCmd: log.stderrShellCmd || undefined,
};
}

Expand Down Expand Up @@ -454,6 +462,10 @@ export const RUN_METADATA_PROVIDER_MESSAGE_FRAGMENT = gql`
pid
externalStdoutUrl
externalStderrUrl
stdoutUriOrPath
stderrUriOrPath
stdoutShellCmd
stderrShellCmd
}
}

Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,10 @@ def from_dagster_event_record(event_record: EventLogEntry, pipeline_name: str) -
externalUrl=data.external_url,
externalStdoutUrl=data.external_stdout_url or data.external_url,
externalStderrUrl=data.external_stderr_url or data.external_url,
stdoutUriOrPath=data.stdout_uri_or_path,
stderrUriOrPath=data.stderr_uri_or_path,
stdoutShellCmd=data.stdout_shell_cmd,
stderrShellCmd=data.stderr_shell_cmd,
pid=dagster_event.pid,
**basic_params,
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,6 @@ def get_captured_log_metadata(
stdoutLocation=metadata.stdout_location,
stderrDownloadUrl=metadata.stderr_download_url,
stderrLocation=metadata.stderr_location,
stdoutShellCmd=metadata.stdout_shell_cmd,
stderrShellCmd=metadata.stderr_shell_cmd,
)
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,10 @@ class Meta:
externalUrl = graphene.String()
externalStdoutUrl = graphene.String()
externalStderrUrl = graphene.String()
stderrUriOrPath = graphene.String()
stdoutUriOrPath = graphene.String()
stdoutShellCmd = graphene.String()
stderrShellCmd = graphene.String()
pid = graphene.Int()
# legacy name for compute log file key... required for back-compat reasons, but has been
# renamed to fileKey for newer versions of the Dagster UI
Expand Down
16 changes: 16 additions & 0 deletions python_modules/dagster/dagster/_core/events/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -1493,8 +1493,12 @@ def capture_logs(
event_specific_data=ComputeLogsCaptureData(
step_keys=step_keys,
file_key=file_key,
stdout_uri_or_path=log_context.stdout_uri_or_path,
stderr_uri_or_path=log_context.stderr_uri_or_path,
external_stdout_url=log_context.external_stdout_url,
external_stderr_url=log_context.external_stderr_url,
stdout_shell_cmd=log_context.stdout_shell_cmd,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is adding fields to a LOGS_CAPTURED event payload problematic?

stderr_shell_cmd=log_context.stderr_shell_cmd,
external_url=log_context.external_url,
),
)
Expand Down Expand Up @@ -1877,6 +1881,10 @@ class ComputeLogsCaptureData(
("external_url", Optional[str]),
("external_stdout_url", Optional[str]),
("external_stderr_url", Optional[str]),
("stdout_uri_or_path", Optional[str]),
("stderr_uri_or_path", Optional[str]),
("stdout_shell_cmd", Optional[str]),
("stderr_shell_cmd", Optional[str]),
],
)
):
Expand All @@ -1887,6 +1895,10 @@ def __new__(
external_url: Optional[str] = None,
external_stdout_url: Optional[str] = None,
external_stderr_url: Optional[str] = None,
stdout_uri_or_path: Optional[str] = None,
stderr_uri_or_path: Optional[str] = None,
stdout_shell_cmd: Optional[str] = None,
stderr_shell_cmd: Optional[str] = None,
):
return super(ComputeLogsCaptureData, cls).__new__(
cls,
Expand All @@ -1895,6 +1907,10 @@ def __new__(
external_url=check.opt_str_param(external_url, "external_url"),
external_stdout_url=check.opt_str_param(external_stdout_url, "external_stdout_url"),
external_stderr_url=check.opt_str_param(external_stderr_url, "external_stderr_url"),
stdout_uri_or_path=check.opt_str_param(stdout_uri_or_path, "stdout_uri_or_path"),
stderr_uri_or_path=check.opt_str_param(stderr_uri_or_path, "stderr_uri_or_path"),
stdout_shell_cmd=check.opt_str_param(stdout_shell_cmd, "stdout_shell_cmd"),
stderr_shell_cmd=check.opt_str_param(stderr_shell_cmd, "stderr_shell_cmd"),
)


Expand Down
Loading
Loading