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

Improve error handling and UI for launcher "offline" #2318

Merged
merged 5 commits into from
Jun 21, 2023

Conversation

Steve-Mcl
Copy link
Contributor

@Steve-Mcl Steve-Mcl commented Jun 20, 2023

closes #1083

Description

  • Curates the error responses from the launcher to provide better info to the frontend (prevents the default technical "catch all" Error 500 toast notification)
  • Makes the toast message more friendly and ONLY displays it once
  • Adds a friendly / less alarming banner above the log panel to inform users that the logs are currently unavailable
  • Stylises the log panel to indicate it may be stale
  • Adds a timer mixin for controlling polling
    • This was something discussed in a meeting some time back. This component can be (should be) reused wherever polling is required as it can be easily controlled and automatically stops when component is unmounted etc.
logs-offline.mp4

Related Issue(s)

#1083

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on flowforge/helm to update ConfigMap Template
    • Issue/PR raised on flowforge/CloudProject to update values for Staging/Production

Labels

  • Backport needed? -> add the backport label
  • Includes a DB migration? -> add the area:migration label

@Steve-Mcl Steve-Mcl requested a review from Pezmc June 20, 2023 07:38
forge/routes/api/project.js Outdated Show resolved Hide resolved
@Steve-Mcl Steve-Mcl requested a review from Pezmc June 21, 2023 11:43
Copy link
Contributor

@Pezmc Pezmc left a comment

Choose a reason for hiding this comment

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

The timer mixin is a good way to handle this, though currently a bit complex for our needs, I also note it doesn't have any sort of backing off build in, which I believe we should have by default to reduce the risk of self DDOS, but I can add that down the line, so happy to move forward!

@Steve-Mcl
Copy link
Contributor Author

though currently a bit complex for our needs

I understand your point however the ugly work to make polling work (and stop working when you leave a page) in vue is just awful & we need a way to simplify this and have a common approach IMO. So while the timer logic (in the mixin) is slightly more complex (internally), implementing timers (on a page) that dont get "left running" and are easy to start/stop on demand outweighs that (I hope).

Are you going to merge or shall I Pez?

@Steve-Mcl
Copy link
Contributor Author

I also note it doesn't have any sort of backing off build in, which I believe we should have by default to reduce the risk of self DDOS

TBF, neither does the random implementations of setInterval we currently do 🤷

We could add an options with .timeout to the API calls to ensure we poll < timer set interval?

@Steve-Mcl Steve-Mcl merged commit 7727670 into main Jun 21, 2023
@Steve-Mcl Steve-Mcl deleted the 1083-improve-error-handling branch June 21, 2023 15:25
@Pezmc
Copy link
Contributor

Pezmc commented Jun 22, 2023

@Steve-Mcl There's a best practice setTimeout implementation as part of instance status polling, that was the standard I wanted our codebase to reach (this is admittedly in my head and not documented anywhere). It has backing off, and handles mount/unmount events to start/stop the timer in a succinct way.

We can merge that implementation with this new timer library and have the best of both worlds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Instance Logs page doesn't handle errors well
2 participants