Add healthcheck endpoint #177

kmakiela · 2020-06-22T17:16:58Z

Added healthcheck controller
Added tests
Created documentation for this endpoint
Updated Sparrow

lib/mongoose_push_web/controllers/healthcheck_controller.ex

janciesla8818

Thanks for the PR, looks good to me.

michalwski

Thanks for implementing the health check. I have 2 points to discuss:

Documentation of the /healthcheck. I think it's important to say about the endpoint and explain that it's not recommended to do it frequently due to extra work added to the worker processes.
Is it possible, does it make sense to add spec of this endpoint to our OpenAPI spec?
Open question. Is status 200 a good fit for a situation when all connections are dead? Software like load balancers decides if a service is a health based on the HTTP status code.

michalwski · 2020-06-26T10:23:50Z

test/unit/healthcheck_test.exs

+  end
+
+  test "Successful connection to services" do
+    {_, _, body} = API.get("/healthcheck")


I think it'd worth to check the status of the response as well. If I understand it correctly this is 200.

3. Open question. Is status 200 a good fit for a situation when all connections are dead? Software like load balancers decides if a service is a health based on the HTTP status code.

I agree, I put 503 status response in that case. However, I'm not completely sure if this "service unavailable" would be about APNS and FCM or the MPush itself

Maybe this can be used as a guideline: https://tools.ietf.org/id/draft-inadarei-api-health-check-01.html :)

That's good reading. Thanks for that @rslota. @kmakiela could you take this document into account?

* Added healthcheck controller * Added tests * Created documentation for this endpoint * Updated Sparrow dependency

NelsonVides

Just an article check and some small questions 🙂

mix.exs

test/support/api.ex

README.md

Co-authored-by: Nelson Vides <nelson.vides@erlang-solutions.com>

aleklisi · 2020-07-01T08:41:37Z

README.md

+  "connection_status":
+    [
+      "connected",
+      "connected"


So if I get 100 workers does it mean that this list is 100 "connected" strings?
How about:

"connection_status": { "connected": 2, "disconnected": 0 }

At first I thought we wanted to possibly give more information about the workers but since we stick with the connection status only I agree it's cleaner that way. However, the whole response is going to be restructured to match the draft RFC mentioned earlier

michalwski

Thanks for making the response compatible with the proposed RFC and adding documentation. I had few additional, minor comments. Other than that it looks good.

michalwski · 2020-07-01T12:47:55Z

lib/mongoose_push_web/controllers/healthcheck_controller.ex

+    case is_everything_disconnected do
+      true ->
+        503
+


I think this empty line is redundant and make the code less readable.

Suggested change

michalwski · 2020-07-01T12:50:50Z

lib/mongoose_push_web/controllers/healthcheck_controller.ex

+    case pool_info[:connection_status][:connected] do
+      0 ->
+        true
+
+      _ ->
+        false
+    end


How about just this:

Suggested change

case pool_info[:connection_status][:connected] do

0 ->

true

_ ->

false

end

not (pool_info[:connection_status][:connected] > 0)

or could it be even pool_info[:connection_status][:connected] == 0?

michalwski · 2020-07-01T12:51:50Z

lib/mongoose_push_web/controllers/healthcheck_controller.ex

+      case status do
+        503 ->
+          "fail"
+


Suggested change

michalwski · 2020-07-01T12:53:34Z

lib/mongoose_push_web/controllers/healthcheck_controller.ex

+      case is_pool_disconnected?(pool_info) do
+        true ->
+          "fail"
+


Suggested change

kmakiela · 2020-07-01T13:18:56Z

Thanks for making the response compatible with the proposed RFC and adding documentation. I had few additional, minor comments. Other than that it looks good.

These additional lines are all added and required by mix.format.

NelsonVides · 2020-07-01T13:37:42Z

Thanks for making the response compatible with the proposed RFC and adding documentation. I had few additional, minor comments. Other than that it looks good.

These additional lines are all added and required by mix.format.

Yeah, often mix.format forced me to do unexpected stuff 🤷‍♂️

leszke · 2020-07-01T15:27:21Z

lib/mongoose_push_web/controllers/healthcheck_controller.ex

+
+    connections =
+      stats
+      |> Enum.map(&extract_connection_info_from_pool/1)


The coding style remark is that it is considered a bad practise to use pipe only once. Please follow the style guide in future.

Good catch, there were more pipes earlier that got removed

leszke · 2020-07-01T15:42:47Z

lib/mongoose_push_web/controllers/healthcheck_controller.ex

+
+    {_workers, connection_status} =
+      workers
+      |> Enum.map_reduce(%{connected: 0, disconnected: 0}, fn worker_info, acc ->


And here as well :)

README.md

lib/mongoose_push_web/controllers/healthcheck_controller.ex

NelsonVides

Excellent 😄

kmakiela added the wip label Jun 22, 2020

kmakiela force-pushed the km/healthcheck branch from 3baacb2 to 1097640 Compare June 22, 2020 17:18

rslota reviewed Jun 22, 2020

View reviewed changes

lib/mongoose_push_web/controllers/healthcheck_controller.ex Outdated Show resolved Hide resolved

kmakiela force-pushed the km/healthcheck branch from db16ba3 to d03ce97 Compare June 23, 2020 16:50

michalwski marked this pull request as draft June 24, 2020 08:29

kmakiela force-pushed the km/healthcheck branch from d03ce97 to 98c3322 Compare June 25, 2020 11:02

kmakiela requested review from NelsonVides, janciesla8818 and michalwski June 25, 2020 11:04

kmakiela marked this pull request as ready for review June 25, 2020 11:04

kmakiela changed the title ~~Add healtheck endpoint~~ Add healthcheck endpoint Jun 25, 2020

kmakiela force-pushed the km/healthcheck branch from 98c3322 to e1a6bce Compare June 25, 2020 11:09

kmakiela removed the wip label Jun 25, 2020

janciesla8818 requested a review from leszke June 26, 2020 08:54

janciesla8818 approved these changes Jun 26, 2020

View reviewed changes

michalwski reviewed Jun 26, 2020

View reviewed changes

kmakiela force-pushed the km/healthcheck branch 2 times, most recently from 9577b3b to de603a3 Compare June 29, 2020 13:45

Add healthcheck endpoint

597d1f1

* Added healthcheck controller * Added tests * Created documentation for this endpoint * Updated Sparrow dependency

kmakiela force-pushed the km/healthcheck branch from de603a3 to 597d1f1 Compare June 29, 2020 13:57

Merge branch 'master' into km/healthcheck

2ea327d

NelsonVides reviewed Jun 30, 2020

View reviewed changes

mix.exs Outdated Show resolved Hide resolved

test/support/api.ex Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

kmakiela and others added 2 commits June 30, 2020 14:02

Update README.md

e6efb7b

Co-authored-by: Nelson Vides <nelson.vides@erlang-solutions.com>

Merge branch 'master' into km/healthcheck

7a03800

aleklisi reviewed Jul 1, 2020

View reviewed changes

Change response structure to match the suggestions of the RFC

a713302

michalwski reviewed Jul 1, 2020

View reviewed changes

Update tests and documentation

3756d88

kmakiela force-pushed the km/healthcheck branch from 7101c40 to 3756d88 Compare July 1, 2020 12:56

leszke reviewed Jul 1, 2020

View reviewed changes

kmakiela added 2 commits July 1, 2020 18:45

review comments and enabling :warn status

37d801f

App version extracted from spec

61bbd1d

kmakiela force-pushed the km/healthcheck branch from d69fa2a to 61bbd1d Compare July 1, 2020 16:45

NelsonVides reviewed Jul 2, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

lib/mongoose_push_web/controllers/healthcheck_controller.ex Outdated Show resolved Hide resolved

kmakiela force-pushed the km/healthcheck branch from 173503a to 9ab6717 Compare July 2, 2020 09:40

NelsonVides approved these changes Jul 2, 2020

View reviewed changes

Updated to match the newest version of the RFC

db2f290

kmakiela force-pushed the km/healthcheck branch from 9ab6717 to db2f290 Compare July 2, 2020 09:43

NelsonVides merged commit ee74ff3 into master Jul 2, 2020

NelsonVides deleted the km/healthcheck branch July 2, 2020 09:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add healthcheck endpoint #177

Add healthcheck endpoint #177

kmakiela commented Jun 22, 2020 •

edited

Loading

janciesla8818 left a comment

michalwski left a comment

michalwski Jun 26, 2020

kmakiela Jun 29, 2020

rslota Jun 30, 2020

michalwski Jul 1, 2020

NelsonVides left a comment

aleklisi Jul 1, 2020 •

edited

Loading

kmakiela Jul 1, 2020

michalwski left a comment

michalwski Jul 1, 2020

michalwski Jul 1, 2020

leszke Jul 1, 2020

michalwski Jul 1, 2020

michalwski Jul 1, 2020

kmakiela commented Jul 1, 2020

NelsonVides commented Jul 1, 2020

leszke Jul 1, 2020

kmakiela Jul 1, 2020

leszke Jul 1, 2020

NelsonVides left a comment

Add healthcheck endpoint #177

Add healthcheck endpoint #177

Conversation

kmakiela commented Jun 22, 2020 • edited Loading

janciesla8818 left a comment

Choose a reason for hiding this comment

michalwski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NelsonVides left a comment

Choose a reason for hiding this comment

aleklisi Jul 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalwski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmakiela commented Jul 1, 2020

NelsonVides commented Jul 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NelsonVides left a comment

Choose a reason for hiding this comment

kmakiela commented Jun 22, 2020 •

edited

Loading

aleklisi Jul 1, 2020 •

edited

Loading