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

622 backend health endpoint #639

Merged
merged 4 commits into from
Dec 12, 2023
Merged

Conversation

gsproston-scottlogic
Copy link
Contributor

@gsproston-scottlogic gsproston-scottlogic commented Dec 8, 2023

Description

Added a health check endpoint on the backend, and now showing an error on the frontend if the backend isn't running.

Screenshots

image

Concerns

  • The error shows twice because of React rendering things twice in debug mode. - no longer, now just setting the whole chat history to be the error message, rather than adding the error message.
  • I wasn't sure exactly where to put the health check in the frontend. I settled on the MainComponent as that's where the chat messages are declared. Honestly I think this will change once we do Streamline frontend refresh network calls 🛠🛠🛠 #300 as we can check if the backend is running as part of the call that gets the backend state.
  • I tried to add some frontend tests but ran into some issues... I decided against adding them in the end as I think the location of the backend health check is likely to change (after Streamline frontend refresh network calls 🛠🛠🛠 #300). Regardless, I've pushed the branch that has some progress towards the tests should we decide we want them for this PR.

Checklist

Have you done the following?

  • Linked the relevant Issue
  • Added tests
  • Ensured the workflow steps are passing
  • Requested reviews

@gsproston-scottlogic gsproston-scottlogic linked an issue Dec 8, 2023 that may be closed by this pull request
@gsproston-scottlogic gsproston-scottlogic force-pushed the 622-backend-health-endpoint branch 2 times, most recently from 17b0454 to 74e9ed3 Compare December 8, 2023 13:19
@gsproston-scottlogic gsproston-scottlogic marked this pull request as ready for review December 8, 2023 13:32
Copy link
Contributor

@pmarsh-scottlogic pmarsh-scottlogic left a comment

Choose a reason for hiding this comment

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

All looks good! Not too worried about the concerns. Re the double message, would it be good enough to, in addErrorMessage() quickly check if the last message is identical to the one you're about to add?

@gsproston-scottlogic
Copy link
Contributor Author

All looks good! Not too worried about the concerns. Re the double message, would it be good enough to, in addErrorMessage() quickly check if the last message is identical to the one you're about to add?

That didn't quite work as when the second render checked the messages array, it was still empty o.O but I think I found a way around this using useRef

Copy link
Contributor

@pmarsh-scottlogic pmarsh-scottlogic left a comment

Choose a reason for hiding this comment

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

beautiful!

In the future it might be good to disable stuff (like sending messages) if the backend is down, but that's a different ticket

@gsproston-scottlogic
Copy link
Contributor Author

gsproston-scottlogic commented Dec 8, 2023

beautiful!

In the future it might be good to disable stuff (like sending messages) if the backend is down, but that's a different ticket

Yeah agreed. As you say, best to make a new ticket as there might be other things that would be worth disabling if the backend is offline. I'll make a new ticket now... #640

Also, I changed how to show the error message yet again :') Now I'm just setting the whole messages array to be the error message if the backend isn't up. Should be fine as we're checking this when the frontend starts and there's no backend. So there won't be any existing chat history (can't retrieve it from the backend). This avoids using useRef which I felt was a little hacky.

Copy link
Contributor

@pmarsh-scottlogic pmarsh-scottlogic left a comment

Choose a reason for hiding this comment

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

that works too!

Another case is when the server goes down while the user is on the system. Edge case, but might be worth thinking about. Also something for another ticket.

Copy link
Contributor

@heatherlogan-scottlogic heatherlogan-scottlogic left a comment

Choose a reason for hiding this comment

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

Very nice! just the one comment from me

// perform backend health check
healthCheck().catch(() => {
// addErrorMessage('Failed to reach the server. Please try again later.');
setMessages([
Copy link
Contributor

Choose a reason for hiding this comment

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

could we do

setMessages((messages) => [
...messages,
{
message: 'Failed to reach the server. Please try again later.',
type: CHAT_MESSAGE_TYPE.ERROR_MSG,
},
]);

to stop any user input disappearing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes I did try that before, but that would give us two error messages in the chat:

image

For this though, we don't have to worry about user input disappearing. That's because this health check is run on mount, before the user has a chance to start chatting.

Copy link
Contributor

Choose a reason for hiding this comment

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

hm i was able to send messages before seeing the message, so my message would disappear. I see it's niche so happy to approve

@gsproston-scottlogic gsproston-scottlogic merged commit c354bc3 into dev Dec 12, 2023
@gsproston-scottlogic gsproston-scottlogic deleted the 622-backend-health-endpoint branch December 12, 2023 10:44
chriswilty pushed a commit that referenced this pull request Apr 8, 2024
* Backend health check endpoint

* Showing error if the backend isn't running

* Only showing the error once

* No longer using useRef to show one error message
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.

Backend health endpoint
3 participants