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

Webhook health check endpoint #272

Merged
merged 1 commit into from
Jan 30, 2017
Merged

Conversation

mironov
Copy link
Contributor

@mironov mironov commented Jan 29, 2017

Docker-ized environments usually expose an HTTP endpoint for health checks.

For example, in order to use a bot built with this lib in a kubernetes cluster, we have to give it an HTTP endpoint that responds 200 OK.

Refs:
https://github.com/kubernetes/contrib/tree/master/ingress/controllers/gce#health-checks
https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-probes/#defining-a-liveness-http-request

Copy link
Collaborator

@GochoMugo GochoMugo left a comment

Choose a reason for hiding this comment

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

I actually love this PR! Please fix the little details, we merge this in!

@@ -133,8 +133,13 @@ class TelegramBotWebHook {
debug('WebHook request URL: %s', req.url);
debug('WebHook request headers: %j', req.headers);

// If there isn't token on URL
if (!this._regex.test(req.url)) {
if (/\/healthz/.test(req.url)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we make this configurable, like with this._regexp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Done.

// If there isn't token on URL
if (!this._regex.test(req.url)) {
if (/\/healthz/.test(req.url)) {
// If this is a helth check
Copy link
Collaborator

Choose a reason for hiding this comment

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

uuuuuh! Typo! 😸

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooops, thanks! 🙈

* @param {Boolean} [options.https=false] Use https
* @return {Promise}
*/
sendWebHook,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if we named it sendWebHookRequest, so it makes more sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, fixed!

debug('WebHook health check passed');
res.statusCode = 200;
res.end('OK');
} else if (!this._regex.test(req.url)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should test for the token first, then the health endpoint, considering that we do not want to waste too much CPU with each and every update we receive on the webhook! What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, changed the order.

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request head (health_endpoint@998c652).


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 24a3f6d...998c652. Read the comment docs.

@GochoMugo GochoMugo merged commit 3263c6c into yagop:master Jan 30, 2017
GochoMugo added a commit that referenced this pull request Jan 30, 2017
@GochoMugo
Copy link
Collaborator

Thanks @mironov for your time and effort in this PR! Awesome! 🎉

GochoMugo added a commit that referenced this pull request Feb 10, 2017
Added:

1. Add constructor options:
  * (#243) `options.polling.params` (by @GochoMugo, requested-by @sidelux)
1. Add methods:
  * (#74) *TelegramBot#removeReplyListener()* (by @githugger)
1. (#283) Add proper error handling (by @GochoMugo)
1. (#272) Add health-check endpoint (by @mironov)
  * `options.webHook.healthEndpoint`
1. (#152) Add test for TelegramBot#sendDocument() using 'fileOpts'
   param (by @evolun)
1. Document `options.webHook.host` (by @GochoMugo)
1. (#264) Add Bot API version to README (by @kamikazechaser)
1. Add examples:
  - (#271) WebHook on Heroku (by @TheBeastOfCaerbannog)
  - (#274) WebHook on Zeit Now (by @ferrari)

Changed:

1. (#147) Use *String#indexOf()*, instead of *RegExp#test()*, to
   find token in webhook request (by @AVVS)

Fixed:

* Fix bug:
  - (#275, #280) fix es6 syntax error on Node.js v4.x (by @CrazyAbdul)
  - (#276) promise.warning from `request-promise` (by @GochoMugo,
    reported-by @preco21)
  - (#281) fix handling error during polling (by @GochoMugo,
    reported-by @dimawebmaker)
  - (#284) fix error during deletion of already-set webhook, during
    polling (by @GochoMugo, reported-by @dcparga)
1. Fix links in documentation (by @Ni2c2k)
@GochoMugo
Copy link
Collaborator

Shipped in v0.27.0.

N-Agency-member pushed a commit to N-Agency-member/TelegramBot-API-Node.js- that referenced this pull request Sep 3, 2024
passion-27 added a commit to passion-27/node-telegram-bot that referenced this pull request Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants