Skip to content

Added /healthz JSON response for heartbeat data. #1940 #1984

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 3 commits into from
Aug 31, 2020

Conversation

JacobGo
Copy link
Contributor

@JacobGo JacobGo commented Aug 19, 2020

I added a JSON object with heartbeat information by passing the server's Heart to the new HealthHttpProvider. I wasn't quite sure what status code to use. I was considering 503 for expired health checks but settled on 200 with state in the response content.
I haven't written TypeScript before, perhaps a type for the result object would be better?
I made sure not to heartbeat for onRequest with the /healthz route.
Closes #1940

Side note: when can we expect a new release? 3.4.1 is two months old and still has a broken heart 🙃 (zombie connections fixed by ebef18d)

Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

This looks great! All my comments but one are just from the linter.

@code-asher
Copy link
Member

As for the new release, we should have one out this week.

@code-asher
Copy link
Member

One interesting note is that if you use a browser then it requests the service worker which triggers a heart beat. But since this is going to be used by things that aren't browsers I think it should be fine.

@nhooyr nhooyr changed the base branch from master to dev August 25, 2020 18:50
@nhooyr nhooyr closed this Aug 25, 2020
@nhooyr
Copy link
Contributor

nhooyr commented Aug 25, 2020

I'm so sorry about this! Deleted the dev branch was gonna switch this back to base on master but GitHub doesn't let you do that.... After you delete the PR base.

🤦

Please open a new PR with these changes! Once again, I'm so sorry!

@nhooyr nhooyr reopened this Aug 25, 2020
@nhooyr nhooyr changed the base branch from dev to master August 25, 2020 19:07
@nhooyr
Copy link
Contributor

nhooyr commented Aug 25, 2020

Nvm, got it!

@JacobGo
Copy link
Contributor Author

JacobGo commented Aug 31, 2020

Figured out how to run the linter 😅
Hopefully this will make it into 3.5.1! Love the project, I might try to tackle some more milestones, time permitting.

Copy link
Member

@kylecarbs kylecarbs left a comment

Choose a reason for hiding this comment

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

Looks good from my glance. Thanks @JacobGo!

@code-asher code-asher merged commit 75c8fde into coder:master Aug 31, 2020
@code-asher
Copy link
Member

Everything looks great. Thank you for the contribution!

}

public async handleRequest(route: Route, request: http.IncomingMessage): Promise<HttpResponse> {
if (!this.authenticated(request)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this endpoint work regardless of whether or not we're authenticated? @code-asher

Copy link
Member

Choose a reason for hiding this comment

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

Ah crap you're right.

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.

healthcheck path
4 participants