Update Integration Tests #34

cassidycodes · 2024-10-16T20:33:10Z

The integration tests were returning false positives when running locally. This PR updates the integration tests to fail when thresholds are breached. Please see my self-review for an explanation.

cassidycodes · 2024-10-16T20:40:10Z

.github/workflows/integration.yml

        working-directory: ./k6/graphql-api
        run: |
-          bundle
-          bundle exec puma -t 0:1 -p 9291 &
+          ./boot-servers.sh &


Use boot-servers.sh to clean this up and to make local testing simpler.

cassidycodes · 2024-10-16T20:40:42Z

k6/boot-servers.sh

+  HIVE_ENABLED=$hive_enabled \
+  PORT=$port \
+  LOG_LEVEL=$LOG_LEVEL \
+  bundle exec puma -C puma.rb | log_with_prefix "$prefix" &


Start a puma server using puma.rb as the config. See notes below about how this is different from before.

cassidycodes · 2024-10-16T20:41:36Z

k6/graphql-api/puma.rb

+workers 2
+threads 2, 2


Previously this was booted with one worker and one thread. Two workers and two threads lets us run with little more throughput. We are not load testing puma, here. So we may as well boot up a few workers.

cassidycodes · 2024-10-16T20:42:54Z

k6/integration-test.js

+      vus: 5,
+      iterations: 1000,


Dialling down the number of VUs (virtual users) so that we don't slam Puma with throughput. But amping up the iterations so we get better data on p95 response times.

cassidycodes · 2024-10-16T20:43:15Z

k6/integration-test.js

+    "http_req_duration{hive:enabled}": ["p(95)<15"],
+    "http_req_duration{hive:disabled}": ["p(95)<15"],


Fail if either of these has a p95 > 15 ms.

cassidycodes · 2024-10-16T20:43:55Z

k6/integration-test.js

+  check(count, {
+    "usage-api starts with 0 operations": (count) => count === 0,
+  });


New setup with a check to make sure we reset the usage api counter before testing.

cassidycodes · 2024-10-16T20:44:09Z

k6/integration-test.js

+  check(res, {
+    "response body is GraphQL": (res) => res.body.includes("data"),
+  });
+  check(res, {
+    "response body is not a GraphQL error": (res) =>
+      !res.body.includes("errors"),
+  });


Two new checks to ensure we get good data back.

cassidycodes · 2024-10-16T20:45:41Z

k6/integration-test.js

+  check(count, {
+    "usage-api received 1000 operations": (count) => count === REQUEST_COUNT,
+  });


New check to ensure usage API received 1000 operations.

k6/integration-test.js

cassidycodes · 2024-10-17T15:29:13Z

k6/graphql-api/schema.rb

@@ -33,7 +33,8 @@ class Schema < GraphQL::Schema
    GraphQL::Hive,
    enabled: ENV["HIVE_ENABLED"] === "true",
    endpoint: "localhost",
-    debug: true,
+    debug: false,
+    buffer_size: 1,


Use a buffer_size of 1 because we have two puma processes running and we want every operation to get sent for the integration test.

cassidycodes · 2024-10-17T15:30:24Z

k6/integration-test.js

+  thresholds: {
+    "http_req_duration{hive:enabled}": [
+      {
+        threshold: "p(95)<25",
+        abortOnFail: true,
+      },
+    ],
+    "http_req_duration{hive:disabled}": [
+      {
+        threshold: "p(95)<25",
+        abortOnFail: true,
+      },
+    ],
+    checks: [
+      {
+        threshold: "rate===1",
+        abortOnFail: true,
+      },
+    ],


We do a non-zero exit code if any of these fail.

cassidycodes · 2024-10-17T17:56:14Z

k6/k6.js

-  if (
-    data.metrics["http_req_duration{hive:enabled}"] &&
-    data.metrics["http_req_duration{hive:disabled}"]
-  ) {
-    const withHive =
-      data.metrics["http_req_duration{hive:enabled}"].values["avg"];
-    const withoutHive =
-      data.metrics["http_req_duration{hive:disabled}"].values["avg"];
-    overheadPercentage = 100 - (withHive * 100.0) / withoutHive;
-  }


This was not accurate. On repeated tests I saw hive:enabled 1-15% faster than hive:disabled. Instead of this check, I added a p95 threshold for both tests of 25ms, which seems to work well on github actions runners.

cassidycodes · 2024-10-17T17:57:12Z

k6/integration-test.js

+    "http_req_duration{hive:enabled}": [
+      {
+        threshold: "p(95)<25",
+        abortOnFail: true,
+      },
+    ],
+    "http_req_duration{hive:disabled}": [
+      {
+        threshold: "p(95)<25",
+        abortOnFail: true,
+      },
+    ],


These are in lieu of the performance regression check that compared avg response time. 25 ms is set based on a few github actions runs to be sure these thresholds work on the runners provided.

cassidycodes · 2024-10-17T18:11:34Z

k6/integration-test.js

+  let count = 0;
+  for (let i = 0; i < 10; i++) {
+    const res = http.get("http://localhost:8888/count");
+    count = JSON.parse(res.body).count;
+    console.log(`📊 Total operations: ${count}`);
+    if (count === REQUEST_COUNT) {
+      break;
+    }
+    await sleep(1);
+  }


Perhaps overkill, but this is incase one of the threads hasn't finished reporting by the time teardown is run.

cassidycodes · 2024-10-18T13:31:39Z

k6/integration-test.js

+    "http_req_duration{hive:enabled}": [
+      {
+        // Set a threshold that works consistently on GitHub Actions runners
+        threshold: "p(95)<50",
+        abortOnFail: true,
+      },
+    ],
+    "http_req_duration{hive:disabled}": [
+      {
+        threshold: "p(95)<50",
+        abortOnFail: false,
+      },
+    ],


In a few tests I found that a P95 of 50 ms reliably passed on GitHub actions. The test will fail if hive:enabled crosses the threshold.

rperryng · 2024-10-19T13:59:09Z

.github/workflows/integration.yml

+        uses: ruby/setup-ruby@2a9a743e19810b9f3c38060637daf594dbd7b37f
+        with:
+          working-directory: ./k6/graphql-api
+          ruby-version: "3.3"


would you mind adding a .ruby-version file instead? so that all our CI / localdev tools willl just rely on a single source of truth for this?

It looks like setting working-directory will dictate where setup-ruby checks for the .ruby-version file, but I believe we should be able to just add a symlink at ./k6/graphql-api/.ruby-version to point to the root .ruby-version file

rperryng · 2024-10-19T14:01:00Z

.github/workflows/integration.yml

+      - name: Use Node
+        uses: actions/setup-node@v4
+        with:
+          node-version: 22


maybe same here? just with node-version-file instead?

README.md

rperryng · 2024-10-19T14:03:45Z

README.md

+| reporting.author                     | Author of the latest change                      | Author of the latest change                           |                                                                     |
+| reporting.commit                     | Git SHA or any identifier                        | git sha or any identifier                             |                                                                     |
+| reporting.service_name               | (Optional) Name of the service                   |                                                       |                                                                     |
+| reporting.service_url                | (Optional) URL of the service                    |                                                       |                                                                     |


Are you using something to generate this table? seems like it would be a bit of a chore to update in the future but nbd

I converted the former Ruby Hash to this table manually. I thought this was easier to read in the rendered markdown. I can revert this, though, if you prefer the Ruby Hash version.

rperryng · 2024-10-19T14:06:34Z

README.md

+```sh
+git clone https://github.com/your-username/your-repo-name.git
+cd your-repo-name
+```


Should we just link to the official GitHub docs for forking instructions?

i.e. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/fork-a-repo#forking-a-repository?

README.md

rperryng · 2024-10-19T14:10:30Z

lib/graphql-hive/version.rb

@@ -2,6 +2,6 @@

 module Graphql
  module Hive
-    VERSION = "0.4.3"
+    VERSION = "0.4.4"


I think you mentioned we didn't want to create a new release for this, so should we maintain the current version number?

Co-authored-by: Ryan Perry-Nguyen <[email protected]>

This reverts commit cb40842.

cassidycodes · 2024-10-28T17:51:04Z

Closing this PR as the k6 tests tend to be flaky and aren't the best way of testing either benchmarks in CI or integration.

cassidycodes added 5 commits October 16, 2024 16:24

Update Integration Tests

277cda6

tune VUs

6d6257e

update readme

5017ceb

add contributing section

ae29b2c

bump version

cb40842

cassidycodes commented Oct 16, 2024

View reviewed changes

cassidycodes force-pushed the fix-k6-tests branch 2 times, most recently from bdfc9c6 to 6893d38 Compare October 17, 2024 13:18

Repository owner deleted a comment from charlypoly Oct 17, 2024

cassidycodes force-pushed the fix-k6-tests branch from e986dd1 to 22cfcf5 Compare October 17, 2024 15:25

cassidycodes commented Oct 17, 2024

View reviewed changes

cassidycodes marked this pull request as ready for review October 17, 2024 15:46

cassidycodes commented Oct 17, 2024

View reviewed changes

cassidycodes self-assigned this Oct 17, 2024

cassidycodes requested a review from rperryng October 17, 2024 17:57

cassidycodes commented Oct 17, 2024

View reviewed changes

cassidycodes force-pushed the fix-k6-tests branch from 4a4e3f0 to 2faa8de Compare October 18, 2024 13:12

Repository owner deleted a comment from charlypoly Oct 18, 2024

cassidycodes force-pushed the fix-k6-tests branch 2 times, most recently from e837e3d to 4bc9d6b Compare October 18, 2024 13:21

Repository owner deleted a comment from charlypoly Oct 18, 2024

Update docs and use small buffer

17944a5

cassidycodes force-pushed the fix-k6-tests branch from 4bc9d6b to 17944a5 Compare October 18, 2024 13:24

Repository owner deleted a comment from charlypoly Oct 18, 2024

remove github comment

c6fe94d

cassidycodes force-pushed the fix-k6-tests branch from 3c67f2f to c6fe94d Compare October 18, 2024 13:28

Repository owner deleted a comment from charlypoly Oct 18, 2024

adjust threshold for github actions

0927ffc

cassidycodes commented Oct 18, 2024

View reviewed changes

cassidycodes requested a review from aryascripts October 18, 2024 14:19

rperryng reviewed Oct 19, 2024

View reviewed changes

cassidycodes and others added 3 commits October 21, 2024 09:41

Update README.md

e605242

Co-authored-by: Ryan Perry-Nguyen <[email protected]>

Update README.md

ceb192f

Co-authored-by: Ryan Perry-Nguyen <[email protected]>

Revert "bump version"

1e93cd6

This reverts commit cb40842.

cassidycodes force-pushed the fix-k6-tests branch from d62b72a to 8b79edb Compare October 22, 2024 18:34

cassidycodes added 2 commits October 22, 2024 14:36

use version files

7645e3a

add link to gh docs

4273f44

cassidycodes force-pushed the fix-k6-tests branch from 8b79edb to 4273f44 Compare October 22, 2024 18:36

use yarn wait-on

97fb54c

cassidycodes closed this Oct 28, 2024

This was referenced Oct 30, 2024

refactor(usage-reporter): split into classes #33

Closed

Benchmark Tests Give False Positives #40

Closed

cassidycodes deleted the fix-k6-tests branch December 11, 2024 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Integration Tests #34

Update Integration Tests #34

cassidycodes commented Oct 16, 2024

cassidycodes Oct 16, 2024

cassidycodes Oct 16, 2024

cassidycodes Oct 16, 2024

cassidycodes Oct 16, 2024

cassidycodes Oct 16, 2024

cassidycodes Oct 16, 2024

cassidycodes Oct 16, 2024

cassidycodes Oct 16, 2024

cassidycodes Oct 17, 2024

cassidycodes Oct 17, 2024

cassidycodes Oct 17, 2024

cassidycodes Oct 17, 2024

cassidycodes Oct 17, 2024

cassidycodes Oct 18, 2024

rperryng Oct 19, 2024

rperryng Oct 19, 2024

rperryng Oct 19, 2024

cassidycodes Oct 21, 2024

rperryng Oct 19, 2024

cassidycodes Oct 21, 2024

rperryng Oct 19, 2024

cassidycodes commented Oct 28, 2024

		"http_req_duration{hive:enabled}": ["p(95)<15"],
		"http_req_duration{hive:disabled}": ["p(95)<15"],

Update Integration Tests #34

Update Integration Tests #34

Conversation

cassidycodes commented Oct 16, 2024

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cassidycodes commented Oct 28, 2024