RFC - [computelogmanager] add fields to pass uri, path or shell cmd #26723
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
mlarose
changed the title
| external_stdout_url=log_context.external_stdout_url, | ||
| external_stderr_url=log_context.external_stderr_url, | ||
| stdout_shell_cmd=log_context.stdout_shell_cmd, |
There was a problem hiding this comment.
is adding fields to a LOGS_CAPTURED event payload problematic?
| {externalUrl} | ||
| </a> | ||
| <Icon name="open_in_new" color={Colors.tooltipText()} size={20} style={{marginTop: 2}} /> | ||
| {externalUrl ? ( |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit ce0b204. |
There was a problem hiding this comment.
The broad idea is likely reasonable but I think this PR's scope is too broad to be able to talk about coherently (since as you note, there are both storage considerations and UI considerations to be taken here, which will involve two different audiences likely).
Ideally, could we split this PR into a backend and a frontend section? So that we can have that discussion regarding storage implications, and then upstream talk about UI.
A few general things:
- Would like to see coverage in the integration tests
- When dealing with full-stack features like this we typically split across backend + frontend - I think in this case it's warranted, and shouldn't be too hard to do given that this is all purely additive? So ideally I'd like the backend and frontend pieces here to be two different PRs
- Would like to see the UI
|
Maybe I've misremembered but I never encountered not being able to download the logs or access them in Azure blob with the direct link🤔 |
Good point, but that's a specific to Dagster+ context, in which customers often have a compliance requirements to not ship logs to us. If the storage account/container is private, the direct url we can generate is not usable (error page). In its current state, the users has to look at the url and manually navigate through the Azure portal to download the logs, which is not a very friendly user experience. |
Fair points, I meant to get early directional feedback before going too far in a given direction. I will open a PR stack and separate those. As for the current state of the UI: it's not pretty - i hoped to discuss the content AND perhaps gets recommendations of what components/style to use from the frontend folks. |
|
Makes sense. I think it's fine now that I understand the intent @mlarose |
Summary & Motivation
The Azure Blob Storage Container do not provide direct urls to objects on private storage accounts, even if the user is authenticated.
This PR provide new fields on the captured logs entry that such ComputeLogManager could populate to improve the user experience by providing a path or uri and/or a shell command to access those logs.
How I Tested These Changes
Changelog