This gives the import statements more space, essentially, it collectively
removes 90+ unneeded lines in the imports statements.
Signed-off-by: Taimoor Zaeem <taimoorzaeem@gmail.com>
src/ now contains all source code - in subdirectories, according to the
.cabal component they belong to. This will allow us to put vendored
libraries in the same place - and later split our own code into multiple
components/libraries as well.
Runs the doctests much faster, which is potentially useful in
combination with postgrest-watch for local development.
This implies that doctests run on compiled code, not in a GHCi session,
which has some implications:
- Only exported functions can be tested.
- Imports need to be made explicit in doctests themselves.
On the flipside, this would allow us to potentially include doctest
results in code coverage, I believe.
This change is a requirement to vendor hasql, which otherwise breaks the
existing doctests: hasql contains a .hsc file, which *needs* to be
compiled - not interpreted - to make the tests work.
Instead of generating them at loadtest runtime, we generate these
dynamic targets once via Nix. Because we're using libfaketime, we're not
bound to doing it at the same time as running the tests.
This allows us to run each loadtest right after each other, with minimal
other stuff going on inbetween. It's also a tiny bit more reproducible,
because the loadtests against both branches run with exactly the same
set of targets, where they did not before.
Instead of building, running, building, running, ... we now build all
executables once ahead of time and then run all loadtests right after
each other. This can sometimes reduce noise when load on the GHA runner
varies over time.
Since this requires us to move building into the loadtest-against
script, it also allows to go back to have the regular postgrest-loadtest
command default to building with cabal for faster local iteration.
I'm not sure whether anyone is actually using them, but they never
worked really well, I believe. We should add something based on an
existing pre-commit hook manager, not that brittle custom thing we have.
Allows me to remove postgrest-with-git in the next commit instead of
duplicating it.
This is a heavily discouraged way of using these tools - they will
become outdated almost immediately and would need to be reinstalled on
every change to main.
This list is chronically outdated and hard to maintain with the two
column layout. Simplify and reduce, not important to keep up-to-date to
bring the point across.
We don't need to restrict us to run every request once. Instead we can
run as many as possible within 60s by removing the `-lazy` flag.
This also allows us to generate fewer targets, because vegeta will do
the repetition for us.
This test is not really functional (anymore?). It depends on some tokens
expiring - but they don't, really. This is because:
1) Before merging the RSA and HS variants earlier in this patch series,
they ran roughly 15s and 34s respectively in CI.
2) Since PostgREST takes a clock skew of up to 30 seconds into account,
there would be *at most* 4s of expiring tokens in a test run - and
*only* in the HS case, not for RSA.
While we didn't have much problem in this test, it's surely not a
problem either to make the test more reproducible with a fixed base
timestamp. This gives us a tiny bit less of a diff on the next change.
This test is not very interesting, really: It's hard to imagine to be
able to produce a regression that would slow down the no-cache case, but
would *not* show when a cache is used.
Let's use the same dependency when creating key material and when
creating tokens, no need to carry multiple different dependencies and
converting keys between them.
Instead of passing a path to the output target file, just pass the
directory in which to create the file. This allows the script to create
multiple files later.
When we changed the test structure to use libfaketime, we moved away
from generating targets as part of the chained command running
with-pg/with-pgrst. This function was left-over from that, the
genTargets script does not need to relay its remaining arguments
arguments anymore.
Instead of creating separate test suites for the key type, the PostgREST
instance now accepts both keys via a JWKSet and the targets are
generated 50/50 for both.
The results are still reported seperately by using a different URL,
which shows up as separate rows in the results.
The script currently depends on results from "head" and "main" to be
present to calculate the change ratio. Fallback nicely in case these are
not available, which allows using the same to report URL-by-URL stats
locally.
BREAKING CHANGE
Breaks the string comparison operators implemented in #3813. Those can
be replaced with regex searches using JSON Path `search()` function.
Signed-off-by: Taimoor Zaeem <taimoorzaeem@gmail.com>
Instead of starting a separate process, which would make the pid we
store the one from `faketime` instead of `postgrest`, we can also just
start libfaketime directly via LD_PRELOAD.
There is no need to store the out-links to the various build artifacts
in the current working directory while running postgrest-with-pg, which
is used by postgrest-loadtest. Instead reference the nix store paths
directly.
Resolves#5028
This also requires enabling shared_preload_libraries=pg_stat_statements.
Really we only need compute_query_id=on, but pg_stat_statements is
useful for other tests and it already enables compute_query_id (since
it's `auto` by default).
This reports the percentage change between the current head branch and
the main branch, which is exactly the number we'll want to make our
decisions on "success or fail" on.
CI failures will initially be reported for regressions of 5% or more on
an individual number.
Because we separate loadtest results per URL now, we can move the error
tests into the regular mixed bag of loadtests - we will be able to tell
from the misspelled URLs when we hit a regression in that area.
We should be able to do similar things for JWT tests, but we'll need
more infrastructure here.
We previously used "rate", i.e. number of requests per second, as the
primary metric to judge loadtest results. However, this has always been
varying from run to run quite a bit, especially in CI where other jobs
possibly run on the same VM host.
The run-to-run variance has massively increased after splitting the
results up per request. Example run in CI with rate on the PR
introducing this change (on which we would expect no change at all):
| rate [1/s] | main | head | Δ |
|:-----------------------------------|-------:|-------:|-----:|
| / | 9.4 | 9.5 | 1% |
| /actors | 870.4 | 1023.0 | 18% |
| /actors?actor=eq.1 | 188.5 | 198.6 | 5% |
| /actors?actor=eq.1&columns=name | 197.3 | 167.1 | -15% |
| /actors?select=*,roles(*,films(*)) | 153.9 | 144.9 | -6% |
| /films?columns=id,title | 157.9 | 182.6 | 16% |
| /films?columns=id,title,year,... | 87.0 | 87.1 | 0% |
| /roles | 204.5 | 267.3 | 31% |
| /rpc/call_me | 231.3 | 208.8 | -10% |
| /rpc/call_me?name=John | 212.2 | 201.7 | -5% |
From the data we can easily tell that the very reason that rate as a
paramter has only worked, so far, because the data was *heavily*
dominated by the requests on the root endpoint for OpenAPI. The longer
duration makes the request much less vulnerable for concurrent activity.
For all other requests its essentially not possible to judge the effect
of a PR this way.
One way to counter this would be to massively increase the time the
loadtest runs. More samples will result in a smoother average. However,
that's not practical for usability of CI. In the original PR #1812 I
already evaluated using the *minimum latency* as the most reliable
criteriumi, but this has never really caught on. The theory behind this
is: The variation in timings between requests is happening because of
concurrent activity, priority chosen by the scheduler, availability of
resources and such - all factors *outside* our control, and *irrelevant*
to the Haskell code we're writing.
Using the minimum latency is an estimation of how fast the code can run
*in the best case*. This might not be a number relevant for production,
but it's much more directly related to the code we write.
Here's to show how variation becomes *much* smaller with minimum latency
as the parameter:
| min latency [μs] | main | head | Δ |
|:-----------------------------------|---------:|-------:|-----:|
| / | 1275.3 | 1263.6 | -1% |
| /actors | 10.0 | 9.9 | -1% |
| /actors?actor=eq.1 | 50.7 | 48.3 | -5% |
| /actors?actor=eq.1&columns=name | 54.1 | 54.0 | 0% |
| /actors?select=*,roles(*,films(*)) | 63.2 | 61.9 | -2% |
| /films?columns=id,title | 51.1 | 50.7 | -1% |
| /films?columns=id,title,year,... | 121.9 | 121.8 | 0% |
| /roles | 42.9 | 42.6 | -1% |
| /rpc/call_me | 45.6 | 45.4 | 0% |
| /rpc/call_me?name=John | 44.4 | 44.2 | 0% |
Since we're separating results per request now, we can only sensibly
focus on *one* parameter - otherwise this would get really clunky
UI-wise. Especially for automated CI failures, minimum latency is the
logical choice.
This commit starts using minimum latency, i.e. P0, but any percentile
should be an improvement over the status quo. A later commit will change
to a different P-value.
Different requests hit different code paths and perform very
differently. By looking at each request type separately, we should be
able to get a much better idea of what kind of change in performance
we're looking at and where the root cause might be.
It will hopefully also allow us to migrate some of the other test-cases
into the main loadtest.
Ultimately, we only look at the `rate` column, so we can just as well
remove all other columns.
This makes the next step, when we split results by request type, much
less noisy.
Instead of taking wild guesses at the runtime of the target generation
itself, we're just making sure to reset the system time to a fixed value
when we ultimately start PostgREST. This allows us to create the right
JWT expiry values ahead of time.
This was required for v9 and earlier, but these don't build with the
current nix invocation anymore anyway. Even loadtesting against v10 does
not work, because `--version` is used in one of the wait scripts and
this was only added in v11.2.
So no need to pretend we'd support comparing against older versions.
Disabling the autovacuum daemon should also help reproducibility in
theory, although I don't know of any cases where we hit a problem with
that.
VACUUM changes the order of rows that PostgreSQL returns for some table
without explicit ordering, thus doing the latter to make it consistently
reproducible.
After ANALYZE estimates are 100% exact for the moment, so some requests
which returned 206 Partial Response now return 200 instead. The fact
that PostgREST returns 206 on an unfiltered endpoint can probably be
considered a bug.
Proves the failure on https://github.com/PostgREST/postgrest/issues/4622.
This doesn't require additional test infra, only nginx. Taking advantage
of the `stream {}` context which is also compatible with unix socket
besides TCP.
Taimoor Zaeem