Skip to content

[dashboard] Don't always print 'Connecting to workspace logs...' (it's somewhat misleading) #8558

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

Merged
merged 1 commit into from
Mar 3, 2022

Conversation

jankeromnes
Copy link
Contributor

@jankeromnes jankeromnes commented Mar 3, 2022

Description

Don't always print 'Connecting to workspace logs...' (it's somewhat misleading)

Context: We initially added it to show the user that the front-end is loaded, and now the backend is working to fetch what the user wants. However, we've observed that the backend is unreliable, and can fail in various different stages, so it's not always actually connecting (e.g. sometimes there is no workspace to connect to, other times it cannot find the logs for some reason).

Maybe showing a blank logs view is better than a somewhat misleading message.

A better, long-term fix would be to implement the Headless Logs (again) RFC, i.e. "central log ingestion service to content-service, make supervisor a writing client to this service".

Related Issue(s)

Cosmetic UI fix. Fixes #6305

How to test

  1. Open any logs in the dashboard
  2. The first line should not be "Connecting to workspace logs"

Release Notes

[dashboard] Don't always print 'Connecting to workspace logs...' (it's somewhat misleading)

Documentation

@jankeromnes jankeromnes requested a review from a team March 3, 2022 08:56
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Mar 3, 2022
@geropl
Copy link
Member

geropl commented Mar 3, 2022

A better, long-term fix would be to implement the Headless Logs (again) RFC, i.e. "central log ingestion service to content-service, make supervisor a writing client to this service".

I slightly disagree: The implementation of the backend service is pretty independent of how we display the content on the frontend side. 🙂 I feel most of the confusion here could be avoided by simply displaying the same "prebuild status indicator" that we already show on the projects prebuild page. 👍

@codecov
Copy link

codecov bot commented Mar 3, 2022

Codecov Report

Merging #8558 (ad570c5) into main (4b67f8e) will decrease coverage by 1.13%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8558      +/-   ##
==========================================
- Coverage   12.31%   11.17%   -1.14%     
==========================================
  Files          20       18       -2     
  Lines        1161      993     -168     
==========================================
- Hits          143      111      -32     
+ Misses       1014      880     -134     
+ Partials        4        2       -2     
Flag Coverage Δ
components-gitpod-cli-app 11.17% <ø> (ø)
components-local-app-app-darwin-amd64 ?
components-local-app-app-darwin-arm64 ?
components-local-app-app-linux-amd64 ?
components-local-app-app-linux-arm64 ?
components-local-app-app-windows-386 ?
components-local-app-app-windows-amd64 ?
components-local-app-app-windows-arm64 ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
components/local-app/pkg/auth/pkce.go
components/local-app/pkg/auth/auth.go

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b67f8e...ad570c5. Read the comment docs.

@roboquat roboquat merged commit 6947fe0 into main Mar 3, 2022
@roboquat roboquat deleted the jx/not-connecting branch March 3, 2022 10:49
@jankeromnes
Copy link
Contributor Author

jankeromnes commented Mar 3, 2022

Many thanks for the quick review @geropl! 🙏

A better, long-term fix would be to implement the Headless Logs (again) RFC, i.e. "central log ingestion service to content-service, make supervisor a writing client to this service".

I slightly disagree: The implementation of the backend service is pretty independent of how we display the content on the frontend side. 🙂

I was referring to the very unreliable nature of our current prebuild logs service:

  • Sometimes it takes an extremely long time before you get any logs (what is the backend even doing that takes so long? 🤔)
  • Sometimes you don't get any logs at all, ever (the backend seems unable to find logs for a prebuild we know exists? 😳)
  • Sometimes you get logs, but they're gone again when you refresh
  • This isn't even consistent between page loads (a few times I've had the same prebuild open in two different tabs: One with logs but the wrong status 🙈, and another tab opened just seconds later without any logs but the correct status 🙈)

It seems to me that a central logs service, which reliably responds to log requests (always in the same way), and to which supervisor can write whenever new logs come in, would have many advantages:

  • It would always reliably and immediately respond with any past logs (i.e. no awkward hand-off period between multiple sources of truth)
  • It could accept and forward/stream new logs as they come in (regardless of whether the workspace is starting or running or stopping / backing up)

To me, a much simpler logs design (central, serve and stream any time, write whenever you want) would remove all the awkward bugs and unreliability we see today.

@geropl
Copy link
Member

geropl commented Mar 3, 2022

To me, a much simpler logs design (central, serve and stream any time, write whenever you want) would remove all the awkward bugs and unreliability we see today.

...buts add additional burden to operate and maintain, and additional dependencies between workspace and app clusters. 🙂 IMO in principle the current approach is nice because we do not need to take care of that.

I was referring to the very unreliable nature of our current prebuild logs service:

I 💯 agree that in general, having a separate component would put more emphasize on this feature. 👍
But nearly all of the current problems are based on the distributed nature, and the fact that we have db-sync in between. Once that is gone, I expect most of the flakiness to just vanish.

Anyway, was just mentioning this here to separate two things:

  • let's first focus on the correct and stable representation in the UI: we need that anyway, and will for some time before we pour more effort into anything else
  • I'm in favor of adding the idea of a "log service" of sorts onto our long-term roadmap backlog thingy (going to share on Monday during sync)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note size/XS team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Project Configuration - Run Prebuild always shows "Connecting to workspace Logs..."
3 participants