-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix: inaccurate Server-Timing durations #4544
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
789777d to
3a6efed
Compare
c546d39 to
b5d164b
Compare
The transaction duration was notably off, doing: ``` curl localhost:3000/rpc/sleep?seconds=5 -i ``` Shows `46.1` for the `transaction;dur`, with this fix we obtain `5007.3`. Fixes PostgREST#4522
b5d164b to
b2029af
Compare
|
There seems to be a slight loss of performance on the JWT loadtests and I'm very puzzled by it, do we use the Specially the Note: don'get confused by |
Makes no sense since |
|
Ah, I see it now, we use postgrest/src/PostgREST/AppState.hs Lines 419 to 421 in b2029af
This is when we show: The jwt loadtests consider this startup time for an estimate: postgrest/nix/tools/generate_targets.py Lines 134 to 135 in b2029af
|
|
This likely means that the perf loss when enabling server-timing is higher now: #3410 (comment) (used to be ~6.5%) |
|
Btw, with this change the initial load is modified/corrected too: oldnewEdit: will add a test for this. I can also confirm the recent gains on #4396 (comment) are maintained (the |
321ef10 to
68e5d78
Compare
I've tried fiddling with that but no dice 😕 .. I'm going to have to rework those loadtests before merging here. |
871f4f4 to
d1cb8dd
Compare
Also print last unauthorized body
9c82e24 to
095ea0a
Compare
Looks like on the
By increasing the
By increasing to 100s (2bb7ab0), we don't get errors (https://github.com/PostgREST/postgrest/actions/runs/20248902699/job/58136016682) But clearly this is no longer about the startup time, the increase is too much and doesn't make sense (manually testing it it cannot be more than 1s). The new |
|
I compared the loadtest on both branches with a profiled build, we can do this now (after the work on #4219): And there's no meaningful difference: mainMon Dec 15 18:12 2025 Time and Allocation Profiling Report (Final) headMon Dec 15 17:58 2025 Time and Allocation Profiling Report (Final) |
c434832 to
2998b97
Compare
2998b97 to
d0c6944
Compare
Turns out the above didn't really consider the build time when the cache is invalidated (happens here because postgrest.cabal is modified). The cached builds take 2 or 3 seconds but when building HEAD, it takes 90 seconds as seen below: (From https://github.com/PostgREST/postgrest/actions/runs/20254022788/job/58152032057?pr=4544) Will correct that separate from this PR. |
The transaction duration was notably off, doing:
Shows
46.1for thetransaction;dur, with this fix we obtain5007.3.Fixes #4522, fixes #4551