Bug 36546 - Bundle API spec to speed up worker startup
Summary: Bundle API spec to speed up worker startup
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-04-08 14:36 UTC by Didier Gautheron
Modified: 2024-05-27 01:14 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This change adds a bundled version of the API specification during build time, which requires less processing, which in turn allows Koha to start up faster.
Version(s) released in:
24.05.00


Attachments
Flamegraph of debian/templates/plack.psgi (269.56 KB, image/png)
2024-04-08 14:46 UTC, Julian Maurice
Details
Bug 36546: Add a bundled version of the API spec (56.88 KB, patch)
2024-04-23 15:38 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36546: Fix js-yaml dependency in package.json (2.31 KB, patch)
2024-04-24 03:20 UTC, David Cook
Details | Diff | Splinter Review
Bug 36546: Add a bundled version of the API spec (56.93 KB, patch)
2024-04-24 09:36 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 36546: Fix js-yaml dependency in package.json (2.36 KB, patch)
2024-04-24 09:36 UTC, Paul Derscheid
Details | Diff | Splinter Review
[OPTIONAL] Bug 36546: Use JSON instead (1.31 KB, patch)
2024-04-25 14:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36546: Use JSON instead (1.35 KB, patch)
2024-04-25 19:15 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 36546: Use JSON instead (1.35 KB, patch)
2024-04-25 20:07 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 36546: Deploy swagger_bundle.json via make (1.93 KB, patch)
2024-04-29 23:46 UTC, David Cook
Details | Diff | Splinter Review
Bug 36546: Add a bundled version of the API spec (57.05 KB, patch)
2024-04-30 06:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36546: Fix js-yaml dependency in package.json (2.48 KB, patch)
2024-04-30 06:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36546: Use JSON instead (1.47 KB, patch)
2024-04-30 06:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36546: Deploy swagger_bundle.json via make (1.99 KB, patch)
2024-04-30 06:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36546: (QA follow-up) Add bundle spec to .gitignore (679 bytes, patch)
2024-04-30 18:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36546: (follow-up) Add fallback to unbundled spec (1.00 KB, patch)
2024-05-01 08:17 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Didier Gautheron 2024-04-08 14:36:33 UTC
From an strace it can take up to 6 sec on our servers.

Ex:

First stat:
07:48:46.941069 (+     0.000129) brk(0x5641224ad000) = 0x5641224ad000 <0.000018>
07:48:46.941340 (+     0.000015) stat("/home/koha/src/api/v1/swagger/swagger.yaml", {st_mode=S_IFREG|0644, st_size=30696, ...}) = 0 <0.000010>
...
last one
07:48:53.453980 (+     0.000030) lstat("/home/koha/src/api/v1/swagger/swagger.yaml", {st_mode=S_IFREG|0644, st_size=30696, ...}) = 0 <0.000015>
Comment 1 Julian Maurice 2024-04-08 14:46:38 UTC
Created attachment 164520 [details]
Flamegraph of debian/templates/plack.psgi

I also noticed that startup time was really long and ran debian/templates/plack.psgi with Devel::NYTProf to see what was happening.
And indeed the YAML parsing/validation is what takes the most time (95% of the startup is spent into Koha::REST::V1::startup
Here's the flamegraph to give an idea of the problem
Comment 2 Tomás Cohen Arazi 2024-04-23 13:14:15 UTC
Didier and I made some testing and bundling the spec reduces the startup time significantly (5x).
Comment 3 Tomás Cohen Arazi 2024-04-23 15:38:25 UTC
Created attachment 165412 [details] [review]
Bug 36546: Add a bundled version of the API spec

This patch makes the Koha::REST::V1 class use a bundled version of the
API spec.

For that to happen, this patch also adds:

* a `yarn api:bundle` task for generating the `swagger_bundle.yaml`
  file
* the bundle file to `.gitignore`
* @redocly/cli to the packages.json dependencies (yarn add @redocly/cli)
* `api:bundle` is added to the `yarn build` general command

NOTE: A new workflow is introduced by this patch as developers will need
to call the `yarn api:bundle` command on each API spec change they make.

To test:
1. Have KTD running
2. Verify the API works as usual
=> SUCCESS: The API just works
3. Apply this patch
4. Run:
   $ ktd --shell
  k$ yarn api:bundle
5. Restart plack:
  k$ koha-plack --restart kohadev ; tail -f /var/log/koha/kohadev/*.log
=> SUCCESS: The API loads correctly
6. Repeat 2
=> SUCCESS: Nothing changed
7. Sign off :-D
Comment 4 Tomás Cohen Arazi 2024-04-23 15:38:59 UTC
Putting this here for review/discussion.

Feedback is welcome!
Comment 5 David Cook 2024-04-24 01:59:34 UTC
I am so happy to see other people talking about this, because this has been driving me crazy for *years*. It's arguably my biggest pet peeve about Koha!

A few years ago, I got Jan Henning Thorsen to add "skip_validating_specification" to Mojolicious::Plugin::OpenAPI, and I've turned that on locally in Koha, but it hasn't made a noticeable difference to startup times, and when I use strace I notice something similar to Didier. The same syscalls over and over again:

newfstatat(AT_FDCWD, "/usr", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(AT_FDCWD, "/usr/share", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(AT_FDCWD, "/usr/share/koha", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(AT_FDCWD, "/usr/share/koha/api", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(AT_FDCWD, "/usr/share/koha/api/v1", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(AT_FDCWD, "/usr/share/koha/api/v1/swagger", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(AT_FDCWD, "/usr/share/koha/api/v1/swagger/swagger.yaml", {st_mode=S_IFREG|0644, st_size=44125, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(AT_FDCWD, "/usr/share/koha/api/v1/swagger/swagger.yaml", {st_mode=S_IFREG|0644, st_size=44125, ...}, 0) = 0

Jan was always skeptical about the validation being the problem, but he humoured me nonetheless. In my tests in 2021, skipping validation solved the problem, but using "skip_validating_specification" hasn't helped in 2024 (and earlier years).

Even back then, I figured the problem was with the dereferencing of the spec, but it seemed to only happen with the validation. 

I'd be interested to see a flamegraph generated with Tomas's patch. I'm going to give it a go now and see what strace says...
Comment 6 David Cook 2024-04-24 02:08:38 UTC Comment hidden (obsolete)
Comment 7 David Cook 2024-04-24 02:09:37 UTC Comment hidden (obsolete)
Comment 8 David Cook 2024-04-24 02:33:51 UTC Comment hidden (obsolete)
Comment 9 Tomás Cohen Arazi 2024-04-24 02:36:20 UTC Comment hidden (obsolete)
Comment 10 David Cook 2024-04-24 03:02:36 UTC Comment hidden (obsolete)
Comment 11 David Cook 2024-04-24 03:04:00 UTC Comment hidden (obsolete)
Comment 12 David Cook 2024-04-24 03:08:09 UTC Comment hidden (obsolete)
Comment 13 David Cook 2024-04-24 03:09:27 UTC Comment hidden (obsolete)
Comment 14 David Cook 2024-04-24 03:10:15 UTC
Comment on attachment 165412 [details] [review]
Bug 36546: Add a bundled version of the API spec

Review of attachment 165412 [details] [review]:
-----------------------------------------------------------------

::: Koha/REST/V1.pm
@@ +75,4 @@
>          $self->secrets([$secret_passphrase]);
>      }
>  
> +    my $spec_file = $self->home->rel_file("api/v1/swagger/swagger_bundle.yaml");

Perhaps we should test for "swagger_bundle.yaml" and fallback to "swagger.yaml" if it's unavailable?
Comment 15 David Cook 2024-04-24 03:20:01 UTC
Created attachment 165446 [details] [review]
Bug 36546: Fix js-yaml dependency in package.json
Comment 16 David Cook 2024-04-24 03:24:47 UTC
I think perhaps we should use swagger.yml for development, and then we could build swagger_bundle.yml during the build. 

In koha-testing-docker, Koha is starting so fast I can't catch the Swagger parsing with strace.

I'm excited to try this out on a test server with multiple instances and more realistic load than my dev machine.

Great work here, Tomas (and everyone else)!
Comment 17 Paul Derscheid 2024-04-24 09:26:17 UTC
Wow, just tested this. Awesome!
Comment 18 Julian Maurice 2024-04-24 09:34:55 UTC
(In reply to David Cook from comment #16)
> I think perhaps we should use swagger.yml for development, and then we could
> build swagger_bundle.yml during the build. 

I would say: use swagger_bundle.yml if it exists, fallback to swagger.yml if not. A slow startup is better than a failed one :)
And using swagger_bundle.yml while developing something unrelated to the API would also improve the code/restart/test cycle speed
Comment 19 Paul Derscheid 2024-04-24 09:36:48 UTC
Created attachment 165449 [details] [review]
Bug 36546: Add a bundled version of the API spec

This patch makes the Koha::REST::V1 class use a bundled version of the
API spec.

For that to happen, this patch also adds:

* a `yarn api:bundle` task for generating the `swagger_bundle.yaml`
  file
* the bundle file to `.gitignore`
* @redocly/cli to the packages.json dependencies (yarn add @redocly/cli)
* `api:bundle` is added to the `yarn build` general command

NOTE: A new workflow is introduced by this patch as developers will need
to call the `yarn api:bundle` command on each API spec change they make.

To test:
1. Have KTD running
2. Verify the API works as usual
=> SUCCESS: The API just works
3. Apply this patch
4. Run:
   $ ktd --shell
  k$ yarn api:bundle
5. Restart plack:
  k$ koha-plack --restart kohadev ; tail -f /var/log/koha/kohadev/*.log
=> SUCCESS: The API loads correctly
6. Repeat 2
=> SUCCESS: Nothing changed
7. Sign off :-D

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 20 Paul Derscheid 2024-04-24 09:36:51 UTC
Created attachment 165450 [details] [review]
Bug 36546: Fix js-yaml dependency in package.json

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 21 Paul Derscheid 2024-04-24 09:38:25 UTC
Still leaving this in discussion for additional opinions but the approach is sane, the gain is measurable and all tests pass (which is to be expected, but still..).

Would love for this to move forward.
Comment 22 Julian Maurice 2024-04-24 10:03:04 UTC
(In reply to Tomás Cohen Arazi from comment #2)
> Didier and I made some testing and bundling the spec reduces the startup
> time significantly (5x).

I see an improvement but not that significant.

I ran the following command:

  time KOHA_HOME=$(pwd) perl debian/templates/plack.psgi

On master, the average time is 3.5s
With the patch, it's 2.4s

The new flamegraph is not that much different, except that the `resolve` call takes less time relative to others

Still a nice improvement!
Comment 23 Paul Derscheid 2024-04-24 10:49:04 UTC
Would be interesting how JSON would perform compared to yaml as the spec is much simpler. Maybe worth a try?
Comment 24 Tomás Cohen Arazi 2024-04-25 14:09:49 UTC
Created attachment 165550 [details] [review]
[OPTIONAL] Bug 36546: Use JSON instead

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 25 Paul Derscheid 2024-04-25 19:14:01 UTC
Just tested the JSON patch as well.

There is another small gain:

(Used the same thing as Julian: `time KOHA_HOME=$(pwd) perl debian/templates/plack.psgi`)

Without the patches applied:
real	0m4.225s
user	0m4.117s
sys	0m0.101s

With the first 2 applied:
real	0m3.065s
user	0m2.983s
sys	0m0.074s

With the JSON patch (3rd) on top:
real	0m2.704s
user	0m2.611s
sys	0m0.086s

I think it wouldn't hurt to use JSON for the bundle.
Comment 26 Paul Derscheid 2024-04-25 19:15:37 UTC
Created attachment 165581 [details] [review]
Bug 36546: Use JSON instead

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 27 Paul Derscheid 2024-04-25 20:07:52 UTC
Created attachment 165589 [details] [review]
Bug 36546: Use JSON instead

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 28 David Cook 2024-04-29 23:31:53 UTC
Failing this for now, since it's not deployable. 

1. We need package.json to include yarn api:bundle in build:prod.
2. swagger_bundle.json will need to be added to the file_map in Makefile.PL as produced by build-resources.PL

I think we should also include a fallback to swagger.yml if swagger_bundled.json is missing, although technically it should be installed by "make install", so I suppose it's not the end of the world...

I can look at writing that missing patch...
Comment 29 David Cook 2024-04-29 23:46:18 UTC
Created attachment 165821 [details] [review]
Bug 36546: Deploy swagger_bundle.json via make

To test:
0. Apply patch
1a. perl Makefile.PL
1b. Choose "single" Installation mode
1c. Specify /opt/koha directory
2. make
2b. ls blib/API_CGI_DIR/v1/swagger/swagger_bundle.json
3. Ensure you're running as root or make koha user own /opt
4. make install
5. ls /opt/koha/api/v1/swagger/swagger_bundle.json
Comment 30 David Cook 2024-04-29 23:48:25 UTC
Actually yeah I think we do need the failover to swagger.json or else many users of koha-testing-docker will have broken Koha instances...
Comment 31 David Cook 2024-04-30 00:23:52 UTC
(In reply to David Cook from comment #30)
> Actually yeah I think we do need the failover to swagger.json or else many
> users of koha-testing-docker will have broken Koha instances...

Ah wait... my bad. ktd runs yarn build so you should have a api/v1/swagger/swagger_bundle.json

Then any core API changes will require a rebuild. Not the end of the world...
Comment 32 Martin Renvoize 2024-04-30 06:44:26 UTC
Created attachment 165828 [details] [review]
Bug 36546: Add a bundled version of the API spec

This patch makes the Koha::REST::V1 class use a bundled version of the
API spec.

For that to happen, this patch also adds:

* a `yarn api:bundle` task for generating the `swagger_bundle.yaml`
  file
* the bundle file to `.gitignore`
* @redocly/cli to the packages.json dependencies (yarn add @redocly/cli)
* `api:bundle` is added to the `yarn build` general command

NOTE: A new workflow is introduced by this patch as developers will need
to call the `yarn api:bundle` command on each API spec change they make.

To test:
1. Have KTD running
2. Verify the API works as usual
=> SUCCESS: The API just works
3. Apply this patch
4. Run:
   $ ktd --shell
  k$ yarn api:bundle
5. Restart plack:
  k$ koha-plack --restart kohadev ; tail -f /var/log/koha/kohadev/*.log
=> SUCCESS: The API loads correctly
6. Repeat 2
=> SUCCESS: Nothing changed
7. Sign off :-D

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 33 Martin Renvoize 2024-04-30 06:44:29 UTC
Created attachment 165829 [details] [review]
Bug 36546: Fix js-yaml dependency in package.json

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 34 Martin Renvoize 2024-04-30 06:44:32 UTC
Created attachment 165830 [details] [review]
Bug 36546: Use JSON instead

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 35 Martin Renvoize 2024-04-30 06:44:35 UTC
Created attachment 165831 [details] [review]
Bug 36546: Deploy swagger_bundle.json via make

To test:
0. Apply patch
1a. perl Makefile.PL
1b. Choose "single" Installation mode
1c. Specify /opt/koha directory
2. make
2b. ls blib/API_CGI_DIR/v1/swagger/swagger_bundle.json
3. Ensure you're running as root or make koha user own /opt
4. make install
5. ls /opt/koha/api/v1/swagger/swagger_bundle.json

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 36 Martin Renvoize 2024-04-30 06:47:37 UTC
This shows a pretty solid improvement for me here too.. I think the polishing that's gone in between the authors is great and I'm happy to QA.

I've added the Signoff lines from each of the testing parties and am going to take their comments and refinements as signoffs respectively.

QA scripts are happy, KTD starts up nicely with the patches applied (and even highlighted a bug that had been missed in main).

Passing QA
Comment 37 Katrin Fischer 2024-04-30 12:33:18 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 38 Katrin Fischer 2024-04-30 14:07:50 UTC
I suspect this to have broken API driven things like patron search:

Something went wrong when loading the table.
404: Not Found. 

HELP.
Comment 39 Katrin Fischer 2024-04-30 14:49:34 UTC
When I run yarn:build locally it first complains about an outdated module and then leaves me with an untracked file:

api/v1/swagger/swagger_bundle.json
Comment 40 Tomás Cohen Arazi 2024-04-30 18:08:28 UTC
Created attachment 165923 [details] [review]
Bug 36546: (QA follow-up) Add bundle spec to .gitignore

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 41 Tomás Cohen Arazi 2024-04-30 18:10:05 UTC
(In reply to Katrin Fischer from comment #38)
> I suspect this to have broken API driven things like patron search:
> 
> Something went wrong when loading the table.
> 404: Not Found. 
> 
> HELP.

This is related to bug 36722. I was able to reproduce the failure, but at some point it started working again. So probably a networking issue with json-schema.org

The right solution is on 36722. And should be done on packaging.
Comment 42 Tomás Cohen Arazi 2024-04-30 18:12:13 UTC
(In reply to Katrin Fischer from comment #39)
> When I run yarn:build locally it first complains about an outdated module

It 'lets you know' there's a newer version of the tool we use to bundle the spec. Not an error per-se.

> and then leaves me with an untracked file:
> api/v1/swagger/swagger_bundle.json

That's the expected output for the command. It highlights the 'Use JSON instead' patch missed to change the file name on the .gitignore file. Follow-up submitted for fixing it.
Comment 43 Tomás Cohen Arazi 2024-04-30 18:16:02 UTC
(In reply to David Cook from comment #28)
> Failing this for now, since it's not deployable. 
> 
> 1. We need package.json to include yarn api:bundle in build:prod.

I'm having hard time understanding how we lost that line, which was included on the first patch! I don't see any patch removing it by itself!
Comment 44 Martin Renvoize 2024-04-30 18:51:10 UTC
Darn, I should have caught the gitignore regression during QA, apologies
Comment 45 David Cook 2024-05-01 00:26:37 UTC
(In reply to Katrin Fischer from comment #39)
> When I run yarn:build locally it first complains about an outdated module
> and then leaves me with an untracked file:
> 
> api/v1/swagger/swagger_bundle.json

You'll need to "rm -rf node_modules" and re-run "yarn install" to fix the outdated module issue.
Comment 46 David Cook 2024-05-01 00:27:51 UTC
(In reply to Tomás Cohen Arazi from comment #43)
> (In reply to David Cook from comment #28)
> > Failing this for now, since it's not deployable. 
> > 
> > 1. We need package.json to include yarn api:bundle in build:prod.
> 
> I'm having hard time understanding how we lost that line, which was included
> on the first patch! I don't see any patch removing it by itself!

The first patch added it to "build" but not to "build:prod". So nothing was lost. Just missed!
Comment 47 David Cook 2024-05-01 00:31:13 UTC
(At some point, it could be interesting to look at resolving the API spec in a module which could be pre-loaded like in bug 36721, because that could deliver even further performance gains.)
Comment 48 Martin Renvoize 2024-05-01 08:17:54 UTC
Created attachment 165945 [details] [review]
Bug 36546: (follow-up) Add fallback to unbundled spec

Some developers are running without ktd still and as such may not want
to install the node modules required to bundle the specification.

This patch adds a fallback to the unbundled yaml files when the
bundle.json file isn't found.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 49 Katrin Fischer 2024-05-02 11:29:55 UTC
Remaining follow-up pushed to main:
 Bug 36546: (follow-up) Add fallback to unbundled spec
Comment 50 Katrin Fischer 2024-05-02 13:28:55 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 51 David Cook 2024-05-03 02:44:42 UTC
What are we thinking in terms of backporting this one?
Comment 52 Fridolin Somers 2024-05-24 12:22:53 UTC
(In reply to David Cook from comment #51)
> What are we thinking in terms of backporting this one?

I'm on it.
Looks like a change that may affect all API.
Some non-packaged, non-debian install may be surprised about this change.
For me this is not a change we should apply on stable branches.
Comment 53 David Cook 2024-05-27 01:14:29 UTC
(In reply to Fridolin Somers from comment #52)
> (In reply to David Cook from comment #51)
> > What are we thinking in terms of backporting this one?
> 
> I'm on it.
> Looks like a change that may affect all API.
> Some non-packaged, non-debian install may be surprised about this change.
> For me this is not a change we should apply on stable branches.

In theory, non-packaged non-debian installs should be OK so long as they update using "make". Even for a git/dev install that doesn't, the fallback to the unbundled spec should still work. 

Let me know if I can help in any way :)