Wait for Plausible active state #3153

ngken0995 · 2023-10-05T20:11:30Z

Fixes

Description

Implement wait in justfile. Wait will have host as localhost:50288 because it points to plausible localhost. There is a loop going through to check the activity of plausible. We can check the health of plausible API(https://plausible.io/docs/stats-api#breakdown-custom-event-by-custom-props).

Testing Instructions

just down -v
just frontend/up
just frontend/init

Checklist

My pull request has a descriptive title (not a vague title likeUpdate index.md).
My pull request targets the default branch of the repository (main) or a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
[N/A] I added or updated tests for the changes I made (if applicable).
[N/A] I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.
[N/A] I ran the DAG documentation generator (if applicable).

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

AetherUnbound

This works great! Added one note, otherwise this is good to merge IMO 🙂

AetherUnbound · 2023-10-07T00:30:59Z

frontend/justfile

@@ -59,7 +59,7 @@ up *flags:

 # Wait for all profile services to be up
 wait-up: up
-    echo "🚧 TODO"
+    just wait


I think it'd be fine to move the contents of the wait recipe here and remove the @wait recipe below entirely!

I think this is implemented this way for consistency with the API justfile which has both @wait and wait-up. I can get behind this, as this can make working in a Dockerised context locally easier in the future.

sarayourfriend · 2023-10-08T22:20:41Z

frontend/justfile

+@health host="localhost:50288":
+    -curl -s -o /dev/null -w '%{http_code}' 'http://{{ host }}/api/health'


Can you change the name of this to health-plausible so that it is clear which service it is checking (specifically, that it isn't checking for the "frontend" service, which doesn't exist).

dhruvkb

Overall the changes look good to me, and you're going in the right direction. Thanks for the contribution @ngken0995! But I some suggestions.

dhruvkb · 2023-10-09T06:09:07Z

frontend/justfile

+    # parent directory and so must be prefixed with `api/`.
+    just ../_loop \
+    '"$(just frontend/health {{ host }})" != "200"' \
+    "Waiting for the frontend to be healthy..."


Along the same lines as @sarayourfriend's feedback, this should be updated as follows.

Suggested change

"Waiting for the frontend to be healthy..."

"Waiting for Plausible to be healthy..."

dhruvkb · 2023-10-09T06:11:48Z

frontend/justfile

+# Wait for the service to be healthy
+@wait host="localhost:50288":
+    # The just command on the second line is executed in the context of the
+    # parent directory and so must be prefixed with `api/`.


Suggested change

# parent directory and so must be prefixed with `api/`.

# parent directory and so must be prefixed with `frontend/`.

dhruvkb · 2023-10-09T06:12:23Z

frontend/justfile

@@ -59,7 +59,7 @@ up *flags:

 # Wait for all profile services to be up
 wait-up: up
-    echo "🚧 TODO"
+    just wait


I think this is implemented this way for consistency with the API justfile which has both @wait and wait-up. I can get behind this, as this can make working in a Dockerised context locally easier in the future.

dhruvkb · 2023-10-09T06:17:49Z

frontend/justfile

+    # The just command on the second line is executed in the context of the
+    # parent directory and so must be prefixed with `api/`.
+    just ../_loop \
+    '"$(just frontend/health {{ host }})" != "200"' \


I think we should also check that the response contains 3 "ok"s.

{ "clickhouse": "ok", "postgres": "ok", "sites_cache": "ok" }

I haven't seen what the status code and response is when Plausible is not healthy so there is a possibility that it returns some "not ok" but with a HTTP 200 response.

@dhruvkb I found two ways to parse the JSON object. First option, we can use a jq which is a JSON parsing and transformation tool for linux and determine if the response contains 3 "ok"s. Second options, we can create python to parse through the JSON object and determine if the response contains 3 "ok"s. Let me know if there is better solution. What is the best option to take?

I don't want to add a dependency on jq. Using Python might be fine 👍.

Agreed that not adding jq as a dependency is good. We've avoided it in other instances as well, opting instead for Python then too.

I think we don't need something as complex as Python or jq or even JSON parsing. Looking at the plausible code for the healthcheck endpoint, we can just as easily do this and it would catch the unhealthy cases:

curl -s 'http://localhost:50288/api/health' --fail | grep -vq 'error'

-vq inverts the match and suppresses grep output (stops it from printing matched lines, in this case all the ok that don't match error, inverted) so we just get the exit code. --fail in curl causes it to fail if the response code is 500, bypassing grep entirely.

@sarayourfriend your solves only require curl and grep which I would prefer.

In the loop for wait, require a condition to stop the loop. I'm not sure what is the output or return for curl -s 'http://localhost:50288/api/health' --fail | grep -vq 'error'. I change the grep to have a invert match count of the word error. curl -s 'http://localhost:50288/api/health' --fail | grep -v -c 'error' which will return 1 when error doesn't exist and return 0 when it does.

@dhruvkb Please take a look at the suggested resolution to use grep and invert matching.

Sorry for not responding here. You'd need to use the exit code for my original suggestion, which you can get with $?. Your approach looks more conventional though, I always have to look up what $? is 😅

I tried 'http://localhost:50288/api/health' --fail | grep -vq 'error' | echo $? and getting this error:

--rpc-eval : RPC failed with reason :nodedown error: Recipe `init` failed on line 66 with exit code 1

Also, I'm not sure how I can pass a exit code into a command substitution.

It seems like the curl -s 'http://localhost:50288/api/health' --fail | grep -v -c 'error' is working as expected. Let me know if there could be any edge case.

What are the other API status result from clickhouse, postgres and sites_cache beside "ok" and "error"?

ngken0995 · 2023-10-10T02:54:57Z

@dhruvkb and @AetherUnbound I created a python file to check the JSON object response for the three oks. I believe it can be improved because I had to add a try-except. Please let me know what I can do to making a sustainable code to check the health of plausible,

dhruvkb

The script is good, and using try-except is not bad practice! But we can use it more effectively by returning a non-zero exit code when the status is not "ok".

sarayourfriend · 2023-10-10T04:47:09Z

frontend/plausible_health_check.py

@@ -0,0 +1,22 @@
+import sys
+
+import requests


requests isn't standard library and not everyone will have it installed in their global python installation (I don't, for example). urlopen is a better choice here, but less convenient, if we stick with a python script instead of a shell-based approach.

+1 to Dhruv's comment that try/except is just fine here, totally appropriate usage.

openverse-bot · 2023-10-13T00:00:20Z

Based on the medium urgency of this PR, the following reviewers are being gently reminded to review this PR:

@obulat
@dhruvkb
@stacimc
This reminder is being automatically generated due to the urgency configuration.

Excluding weekend¹ days, this PR was ready for review 5 day(s) ago. PRs labelled with medium urgency are expected to be reviewed within 4 weekday(s)².

@ngken0995, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings.

Specifically, Saturday and Sunday. ↩
For the purpose of these reminders we treat Monday - Friday as weekdays. Please note that the operation that generates these reminders runs at midnight UTC on Monday - Friday. This means that depending on your timezone, you may be pinged outside of the expected range. ↩

dhruvkb

LGTM, verified that this works. Thanks for the contribution @ngken0995! Will merge it in once CI has passed.

dhruvkb · 2023-10-13T05:28:32Z

frontend/justfile

+@plausible-health host="localhost:50288":
+    -curl -s 'http://localhost:50288/api/health' --fail | grep -v -c 'error'


Verified that this returns 0 for unhealthy and 1 for healthy.

frontend/justfile

check for plausible active state

cb90c72

ngken0995 requested a review from a team as a code owner October 5, 2023 20:11

ngken0995 requested review from AetherUnbound and stacimc October 5, 2023 20:11

openverse-bot added 🟨 priority: medium Not blocking but should be addressed soon 🛠 goal: fix Bug fix 🤖 aspect: dx Concerns developers' experience with the codebase 🧱 stack: mgmt Related to repo management and automations labels Oct 5, 2023

AetherUnbound approved these changes Oct 7, 2023

View reviewed changes

sarayourfriend reviewed Oct 8, 2023

View reviewed changes

dhruvkb reviewed Oct 9, 2023

View reviewed changes

check plausible health

767cefa

ngken0995 requested a review from a team as a code owner October 10, 2023 02:52

ngken0995 requested a review from obulat October 10, 2023 02:52

dhruvkb reviewed Oct 10, 2023

View reviewed changes

sarayourfriend reviewed Oct 10, 2023

View reviewed changes

ngken0995 added 2 commits October 10, 2023 11:13

use curl and grep to check for error

2a117b8

revise grammar

8079ce5

ngken0995 requested a review from dhruvkb October 12, 2023 03:29

dhruvkb approved these changes Oct 13, 2023

View reviewed changes

dhruvkb and others added 2 commits October 13, 2023 05:30

Correct grammar in waiting message

0f5eb2f

Merge branch 'main' into 2489-wait-plausible-service

aa10454

dhruvkb changed the title ~~Check for plausible active state~~ Wait for Plausible active state Oct 13, 2023

dhruvkb merged commit aa3a7f5 into WordPress:main Oct 13, 2023
42 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wait for Plausible active state #3153

Wait for Plausible active state #3153

ngken0995 commented Oct 5, 2023

AetherUnbound left a comment

AetherUnbound Oct 7, 2023

dhruvkb Oct 9, 2023

sarayourfriend Oct 8, 2023

dhruvkb left a comment

dhruvkb Oct 9, 2023

dhruvkb Oct 9, 2023

dhruvkb Oct 9, 2023

dhruvkb Oct 9, 2023

ngken0995 Oct 9, 2023 •

edited

Loading

dhruvkb Oct 9, 2023

sarayourfriend Oct 10, 2023

sarayourfriend Oct 10, 2023

ngken0995 Oct 10, 2023 •

edited

Loading

ngken0995 Oct 12, 2023

sarayourfriend Oct 12, 2023

ngken0995 Oct 12, 2023

ngken0995 commented Oct 10, 2023

dhruvkb left a comment •

edited

Loading

sarayourfriend Oct 10, 2023

openverse-bot commented Oct 13, 2023

dhruvkb left a comment •

edited

Loading

dhruvkb Oct 13, 2023

		@health host="localhost:50288":
		-curl -s -o /dev/null -w '%{http_code}' 'http://{{ host }}/api/health'

	"Waiting for the frontend to be healthy..."
	"Waiting for Plausible to be healthy..."

	# parent directory and so must be prefixed with `api/`.
	# parent directory and so must be prefixed with `frontend/`.

		@plausible-health host="localhost:50288":
		-curl -s 'http://localhost:50288/api/health' --fail \| grep -v -c 'error'

Wait for Plausible active state #3153

Wait for Plausible active state #3153

Conversation

ngken0995 commented Oct 5, 2023

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

AetherUnbound 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

dhruvkb 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

ngken0995 Oct 9, 2023 • edited Loading

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

ngken0995 Oct 10, 2023 • edited Loading

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

ngken0995 commented Oct 10, 2023

dhruvkb left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openverse-bot commented Oct 13, 2023

Footnotes

dhruvkb left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngken0995 Oct 9, 2023 •

edited

Loading

ngken0995 Oct 10, 2023 •

edited

Loading

dhruvkb left a comment •

edited

Loading

dhruvkb left a comment •

edited

Loading