test(load): jwt loadtests with complex jwt-role-claim-key#5013
test(load): jwt loadtests with complex jwt-role-claim-key#5013taimoorzaeem wants to merge 1 commit into
Conversation
Using slightly complex `jwt-role-claim-key` so performance impact can be seen when logic or syntax related to this config changes. Signed-off-by: Taimoor Zaeem <taimoorzaeem@gmail.com>
| export PGRST_DB_TX_END="rollback-allow-override" | ||
| export PGRST_LOG_LEVEL="crit" | ||
| export PGRST_JWT_SECRET="reallyreallyreallyreallyverysafe" | ||
| export PGRST_JWT_ROLE_CLAIM_KEY=".postgrest.roles[?(@ ^== \"postgrest\")]" |
There was a problem hiding this comment.
In #4984, this will be replaced by $.postgrest.roles[?search(@, '^postgrest')]. We can then compare the load test results.
There was a problem hiding this comment.
We can then compare the load test results.
Not really, I think. To compare loadtest results for #4984, the loadtest in question must be supported by both the old and the new version. It's always the loadtest code from the head branch that runs against the older branch.
We'd need to be a bit smarter than that, I'm afraid.
There was a problem hiding this comment.
To compare loadtest results for #4984, the loadtest in question must be supported by both the old and the new version. It's always the loadtest code from the head branch that runs against the older branch.
Wait, I don't understand. Once this gets merged, we'll have this on main branch.
I'll then update it #4984 so the PR branch will compare to main i.e comparing .postgrest.roles[?(@ ^== \"postgrest\")] with $.postgrest.roles[?search(@, '^postgrest')]. What am I missing?
There was a problem hiding this comment.
The loadtest consists of two components:
- All the nix-based tooling. This is always taken from the head branch, because this is the script you're running with
postgrest-loadtest-againstand such. - The
postgrestexecutable, which is built via nix on each tested branch.
This means that the configuration that PostgREST is started with comes from the head branch, not the main branch. In #4984, you will run both executables with $.postgrest.roles[?search(@, '^postgrest')] - and that will just fail for the main branch. For example see 4f9bc89, which removed some of the old fallback configuration that we had to keep in place to be able to compare loadtests against older branches, which still required some configuration options.
With how things are currently set up, it would be easiest to do a manual loadtest locally, where you run postgrest-loadtest (not postgrest-loadtest-against) on both branches, where each branch has the respective configuration option set, and then run the reporting script manually.
There was a problem hiding this comment.
This means that the configuration that PostgREST is started with comes from the head branch, not the main branch.
Oh I see now. This explains it. Thanks for the clarification.
There was a problem hiding this comment.
This means that the configuration that PostgREST is started with comes from the head branch, not the main branch. In #4984, you will run both executables with $.postgrest.roles[?search(@, '^postgrest')] - and that will just fail for the main branch
@wolfgangwalther Why did we did that? Was it to be able to run the loadtests on older versions?
It is confusing and IIRC @robx was also confused before somewhere, so I'm wondering if we can switch to running the Nix tooling from head and main branch?
There was a problem hiding this comment.
Why did we did that? Was it to be able to run the loadtests on older versions?
That was one of the reasons, yes: We needed to do it this way, otherwise there was no way to run the loadtests against older commits which did not have the loadtests introduced, yet.
But that's only one reason. There are other very good reasons to do it like this: Once you start using loadtest scripts themselves from older commits, you will create a maintenance headache: Suppose you fix a bug in these scripts or make some other significant change to them. You can't get any meaningful results from running these against an older commit anymore, because you will be stuck on that older commit.
You might even end up comparing test runs with different versions of certain tools, for example vegeta itself etc.
I believe it creates a lot more problems and pitfalls to do it that way. The case we're having right now should really be the exception anyway, so I don't see a need to change anything here.
3 participants
Using a slightly complex
jwt-role-claim-keyso performance impact can be seen when logic or syntax related to this config changes.