-
Notifications
You must be signed in to change notification settings - Fork 557
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
Improved connection progress #7385
Conversation
"go.opentelemetry.io/otel/trace" | ||
) | ||
|
||
func URLForTrace(ctx context.Context) (string, bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fun little idea about this - this function could imply the existence of a URLForSpan
function π
On a failure, we could print out the URL of a failed span (or even use OSC 8 to display it inline!
078505f
to
87c6da8
Compare
87c6da8
to
e2b5dc1
Compare
This improves visibility of what happens at each step, which can really help users if the engine is failing to start for some reason. Signed-off-by: Justin Chadwell <me@jedevc.com>
If the engine can't be provision in 10 minutes, it's likely something is wrong. 10 minutes is chosen because it might be feasible that on a slow internet connection, the image might take several minutes to pull. Signed-off-by: Justin Chadwell <me@jedevc.com>
This is already visually indicated with the wrapping span completing in a progress view - there's no need to show this additional little message. Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
If the primary span logs have mixed stdout/stderr, we were accidentally not printing out the required trailing newline: 12:35:47 WRN failed to resolve image; falling back to leftover engine error="GET https://registry.dagger.io/v2/engine/manifests/dev-fc0a2c7a85e412b1995e2f088571b56415a3d807: MANIFEST_UNKNOWN: manifest unknown" <nil>% This was because we were incorrectly checking for the presence of a newline - we should be checking if the last log element has a trailing newline, not that *any* log element contains a newline. Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
e2b5dc1
to
b84b9ba
Compare
Signed-off-by: Justin Chadwell <me@jedevc.com>
@@ -243,6 +228,7 @@ func (fe *Frontend) renderMessages(out *termenv.Output, full bool) (bool, error) | |||
fe.messagesView.SetHeight(10) | |||
} | |||
_, err := fmt.Fprint(out, fe.messagesView.View()) | |||
fmt.Fprintln(out) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this print on purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup without this, there's no break between the messages view and the progress.
When we finalRender
later, we already have this, so we need this one for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! The Engine info & Dagger Cloud Trace URL are particular nice touches. The structure of the output is also much neater. Great job π
First 1m 30s
- top is this change, bottom is v0.11.4
pr-7385-first-1m30s-nvidia.mp4
Last 1m
- top is this change, bottom is v0.11.4
pr-7385-last-1m.mp4
Sorry for being late here, but I think the "Connected to engine" line should be at debug level. |
@helderco I think that's fair - the only issue is that then this will only display with the Previously, we always displayed the engine connection information in plain progress, it was just missing in the TUI progress. Is this worth blocking on? I don't want to remove it in the plain progress, I think it would be a regression there, but I can definitely see it being not as useful for the local TUI (it gets repetitive there). |
The thing is, we used to have both the version and the cloud url in a single, more compact line, but it was constantly being flagged as noisy, especially by Solomon. Maybe having it at the top is better, but at least the version is not necessary to see every time. Doesn't have to be a debug record, can you put it at |
Yeah, that's possible requires a little bit of undoing some of the refactors of this PR - I've put something somewhat reasonable in |
* fix: split out connect step to separate steps This improves visibility of what happens at each step, which can really help users if the engine is failing to start for some reason. Signed-off-by: Justin Chadwell <me@jedevc.com> * fix: add timeout of 10 minutes to start engine If the engine can't be provision in 10 minutes, it's likely something is wrong. 10 minutes is chosen because it might be feasible that on a slow internet connection, the image might take several minutes to pull. Signed-off-by: Justin Chadwell <me@jedevc.com> * chore: remove OK! message when starting the engine This is already visually indicated with the wrapping span completing in a progress view - there's no need to show this additional little message. Signed-off-by: Justin Chadwell <me@jedevc.com> * feat: log engine/cloud connection logs Signed-off-by: Justin Chadwell <me@jedevc.com> * fix: trailing newline with combined stdout/stderr If the primary span logs have mixed stdout/stderr, we were accidentally not printing out the required trailing newline: 12:35:47 WRN failed to resolve image; falling back to leftover engine error="GET https://registry.dagger.io/v2/engine/manifests/dev-fc0a2c7a85e412b1995e2f088571b56415a3d807: MANIFEST_UNKNOWN: manifest unknown" <nil>% This was because we were incorrectly checking for the presence of a newline - we should be checking if the last log element has a trailing newline, not that *any* log element contains a newline. Signed-off-by: Justin Chadwell <me@jedevc.com> * fix: ensure terminal profile propagates to context/global loggers Signed-off-by: Justin Chadwell <me@jedevc.com> * fix: render messages first to match finalRender style Signed-off-by: Justin Chadwell <me@jedevc.com> --------- Signed-off-by: Justin Chadwell <me@jedevc.com>
* fix: split out connect step to separate steps This improves visibility of what happens at each step, which can really help users if the engine is failing to start for some reason. Signed-off-by: Justin Chadwell <me@jedevc.com> * fix: add timeout of 10 minutes to start engine If the engine can't be provision in 10 minutes, it's likely something is wrong. 10 minutes is chosen because it might be feasible that on a slow internet connection, the image might take several minutes to pull. Signed-off-by: Justin Chadwell <me@jedevc.com> * chore: remove OK! message when starting the engine This is already visually indicated with the wrapping span completing in a progress view - there's no need to show this additional little message. Signed-off-by: Justin Chadwell <me@jedevc.com> * feat: log engine/cloud connection logs Signed-off-by: Justin Chadwell <me@jedevc.com> * fix: trailing newline with combined stdout/stderr If the primary span logs have mixed stdout/stderr, we were accidentally not printing out the required trailing newline: 12:35:47 WRN failed to resolve image; falling back to leftover engine error="GET https://registry.dagger.io/v2/engine/manifests/dev-fc0a2c7a85e412b1995e2f088571b56415a3d807: MANIFEST_UNKNOWN: manifest unknown" <nil>% This was because we were incorrectly checking for the presence of a newline - we should be checking if the last log element has a trailing newline, not that *any* log element contains a newline. Signed-off-by: Justin Chadwell <me@jedevc.com> * fix: ensure terminal profile propagates to context/global loggers Signed-off-by: Justin Chadwell <me@jedevc.com> * fix: render messages first to match finalRender style Signed-off-by: Justin Chadwell <me@jedevc.com> --------- Signed-off-by: Justin Chadwell <me@jedevc.com> Signed-off-by: Vikram Vaswani <vikram@dagger.io>
@jedevc This is awesome, thank you! One post merge nit: If we're running with a "legacy" token, instead of not printing, any chance we could say something like "Blah blah trace sent to https://dagger.cloud/. Please rotate your Cloud token to get the trace URL from the CLI" |
Continued progress exploration! I just kinda kept pulling on #7375, and kept finding more things to tidy up π
Fixes:
This changes a few things about the initial connection:
connect
step into separate parts - this allows for improved debugability if this step is hanging - we can see exactly which part is being slow.Connected to engine
messages correctly appear with our pretty progress (they weren't being displayed)Connected to cloud
messages.Before:
After: