fishjam-dev / fishjam

General purpose media server. Supports WebRTC, HLS, RTSP, SIP
https://fishjam-dev.github.io/fishjam-docs/
Apache License 2.0
203 stars 16 forks source link

[RTC-454] Healthcheck endpoint #143

Closed sgfn closed 10 months ago

sgfn commented 10 months ago

About the topic: https://emmer.dev/blog/writing-meaningful-health-check-endpoints/ https://tyk.io/blog/api-health-checks-how-to-keep-apps-healthy/

There are several things we need to consider:

  1. Whether we want to perform some actual checks before responding (+ include some additional data in the response)
    1. We're effectively returning a hard-coded 200 every time, which seems more like a ping endpoint than a healthcheck endpoint...
    2. ...but:
      1. We don't have any microservices or databases we depend on
      2. There is little need for the distinction between live and ready states of Jellyfish:
        1. When it starts, it should function correctly from the get-go
        2. During its lifetime, we could designate it as not-ready whenever the load is too high to handle more traffic, but detecting that from within the app wouldn't be trivial
      3. We already have metrics reporting resource usage, amount of rooms etc. We could consider adding uptime, I guess (?)
  2. Whether issuing healthcheck requests should require authentication
    1. IMO right now it isn't necessary, but if we choose to include anything other than {"status": "UP"} in the response, we should use auth

Discussion on these topics is welcome 🙂

Acknowledging the stipulations set forth:

codecov[bot] commented 10 months ago

Codecov Report

Merging #143 (1f517b5) into main (2ca9b97) will decrease coverage by 0.21%. Report is 2 commits behind head on main. The diff coverage is 50.00%.

:exclamation: Current head 1f517b5 differs from pull request most recent head 471bc00. Consider uploading reports for the commit 471bc00 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #143 +/- ## ========================================== - Coverage 87.32% 87.12% -0.21% ========================================== Files 59 62 +3 Lines 1097 1103 +6 ========================================== + Hits 958 961 +3 - Misses 139 142 +3 ``` | [Files](https://app.codecov.io/gh/jellyfish-dev/jellyfish/pull/143?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jellyfish-dev) | Coverage Δ | | |---|---|---| | [lib/jellyfish\_web/api\_spec/health\_report.ex](https://app.codecov.io/gh/jellyfish-dev/jellyfish/pull/143?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jellyfish-dev#diff-bGliL2plbGx5ZmlzaF93ZWIvYXBpX3NwZWMvaGVhbHRoX3JlcG9ydC5leA==) | `100.00% <100.00%> (ø)` | | | [lib/jellyfish\_web/api\_spec/responses.ex](https://app.codecov.io/gh/jellyfish-dev/jellyfish/pull/143?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jellyfish-dev#diff-bGliL2plbGx5ZmlzaF93ZWIvYXBpX3NwZWMvcmVzcG9uc2VzLmV4) | `100.00% <ø> (ø)` | | | [...ellyfish\_web/controllers/healthcheck\_controller.ex](https://app.codecov.io/gh/jellyfish-dev/jellyfish/pull/143?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jellyfish-dev#diff-bGliL2plbGx5ZmlzaF93ZWIvY29udHJvbGxlcnMvaGVhbHRoY2hlY2tfY29udHJvbGxlci5leA==) | `50.00% <50.00%> (ø)` | | | [lib/jellyfish\_web/controllers/healthcheck\_json.ex](https://app.codecov.io/gh/jellyfish-dev/jellyfish/pull/143?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jellyfish-dev#diff-bGliL2plbGx5ZmlzaF93ZWIvY29udHJvbGxlcnMvaGVhbHRoY2hlY2tfanNvbi5leA==) | `0.00% <0.00%> (ø)` | | | [lib/jellyfish\_web/router.ex](https://app.codecov.io/gh/jellyfish-dev/jellyfish/pull/143?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jellyfish-dev#diff-bGliL2plbGx5ZmlzaF93ZWIvcm91dGVyLmV4) | `91.66% <0.00%> (-8.34%)` | :arrow_down: | ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/jellyfish-dev/jellyfish/pull/143?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jellyfish-dev). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jellyfish-dev) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/jellyfish-dev/jellyfish/pull/143?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jellyfish-dev). Last update [2ca9b97...471bc00](https://app.codecov.io/gh/jellyfish-dev/jellyfish/pull/143?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jellyfish-dev). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jellyfish-dev).
mickel8 commented 10 months ago

Unpinning myself, feel free to merge if @Rados13 and @roznawsk approve

roznawsk commented 10 months ago

I agree with Rados13 on the great research 👍🏽 . We can add information about distribution to the response.