Skip to content

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

Merged
code-asher merged 3 commits into
coder:masterfrom
JacobGo:master
Aug 31, 2020
Merged

Added /healthz JSON response for heartbeat data. #1940#1984
code-asher merged 3 commits into
coder:masterfrom
JacobGo:master

Conversation

@JacobGo

@JacobGo JacobGo commented Aug 19, 2020

Copy link
Copy Markdown
Contributor

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)

@code-asher code-asher left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/node/app/health.ts Outdated
Comment thread src/node/app/health.ts Outdated
Comment thread src/node/http.ts Outdated
Comment thread src/node/app/health.ts Outdated
Comment thread src/node/app/health.ts
Comment thread src/node/app/health.ts Outdated
Comment thread src/node/entry.ts Outdated
@code-asher

Copy link
Copy Markdown
Member

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

@code-asher

Copy link
Copy Markdown
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

nhooyr commented Aug 25, 2020

Copy link
Copy Markdown
Contributor

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

nhooyr commented Aug 25, 2020

Copy link
Copy Markdown
Contributor

Nvm, got it!

@JacobGo

JacobGo commented Aug 31, 2020

Copy link
Copy Markdown
Contributor Author

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.

@kylecarbs kylecarbs left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

Everything looks great. Thank you for the contribution!

Comment thread src/node/app/health.ts
}

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

Copy link
Copy Markdown
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
Copy Markdown
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