Bug 35277 - Pseudonymization should be done in a background job
Summary: Pseudonymization should be done in a background job
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens (kidclamp)
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-11-07 13:33 UTC by Nick Clemens (kidclamp)
Modified: 2024-02-09 08:57 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.03


Attachments
Benchmarking script (3.83 KB, application/x-perl)
2023-11-07 13:43 UTC, Nick Clemens (kidclamp)
Details
Bug 35277: Pseudonymize in a background job (6.92 KB, patch)
2023-11-07 13:48 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35277: Pseudonymize in a background job (6.99 KB, patch)
2023-11-07 16:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35277: Pseudonymize in a background job (6.54 KB, patch)
2023-11-07 16:57 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35277: Unit tests (4.18 KB, patch)
2023-11-07 16:57 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35277: Pseudonymize in a background job (6.61 KB, patch)
2023-11-07 17:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35277: Unit tests (4.25 KB, patch)
2023-11-07 17:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35277: (follow-up) Remove debugging module (853 bytes, patch)
2023-11-10 11:47 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35277: (QA follow-up) Fix POD (826 bytes, patch)
2023-12-01 11:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35277: Pseudonymize in a background job (6.69 KB, patch)
2024-01-12 10:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35277: Unit tests (4.34 KB, patch)
2024-01-12 10:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35277: (follow-up) Remove debugging module (948 bytes, patch)
2024-01-12 10:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35277: (QA follow-up) Fix POD (921 bytes, patch)
2024-01-12 10:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35277: (QA follow-up) POD corrections (1.14 KB, patch)
2024-01-12 10:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35277: (follow-up) Fix tests (8.57 KB, patch)
2024-01-16 14:27 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2023-11-07 13:33:03 UTC
Doing some testing, Pseudonymization has an impact on the rate of checkouts.

In large systems, with many attributes, we have seen significant impact.

To mitigate this, we should enqueue pseudonymization as a background task
Comment 1 Nick Clemens (kidclamp) 2023-11-07 13:43:29 UTC
Created attachment 158596 [details]
Benchmarking script

I used this to test the performance with and without pseudonymization enabled
Comment 2 Nick Clemens (kidclamp) 2023-11-07 13:48:47 UTC
Created attachment 158597 [details] [review]
Bug 35277: Pseudonymize in a background job

This patch adds a new background job for pseduonymization

To test:
 1 - Download the benchmarking script
 2 - Ensure your koha has bcrypt_settings in koha-conf.xml
     See bug 28911
 3 - perl perf_check_pseudo.pl
 4 - Note the slowdown after pseudonymization enabled
 5 - Apply patches, restart all
 6 - perl perf_check_pseudo.pl
 7 - Note improvement
 8 - Enable pseudonymization in sytem preferences
 9 - Perform some checkouts and returns
10 - Verify the background jobs complete successfully
11 - Verify the pseudonymized_transactions table is updated correctly
Comment 3 Martin Renvoize 2023-11-07 16:39:31 UTC
Created attachment 158606 [details] [review]
Bug 35277: Pseudonymize in a background job

This patch adds a new background job for pseduonymization

To test:
 1 - Download the benchmarking script
 2 - Ensure your koha has bcrypt_settings in koha-conf.xml
     See bug 28911
 3 - perl perf_check_pseudo.pl
 4 - Note the slowdown after pseudonymization enabled
 5 - Apply patches, restart all
 6 - perl perf_check_pseudo.pl
 7 - Note improvement
 8 - Enable pseudonymization in sytem preferences
 9 - Perform some checkouts and returns
10 - Verify the background jobs complete successfully
11 - Verify the pseudonymized_transactions table is updated correctly

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize 2023-11-07 16:45:30 UTC
This makes a significant improvement for me, the stats are interesting.

That tables all look correct to me after the patch and job has run too.

Without the patch I go from 30/s to 8/s for the checkout_in benchmark between the syspref on and off.. with the patch I go from 30/s to 25/s.. that's a marked improvement and I feel is well worthwhile.

Signing off
Comment 5 Nick Clemens (kidclamp) 2023-11-07 16:57:18 UTC
Created attachment 158607 [details] [review]
Bug 35277: Pseudonymize in a background job

This patch adds a new background job for pseduonymization

To test:
 1 - Download the benchmarking script
 2 - Ensure your koha has bcrypt_settings in koha-conf.xml
     See bug 28911
 3 - perl perf_check_pseudo.pl
 4 - Note the slowdown after pseudonymization enabled
 5 - Apply patches, restart all
 6 - perl perf_check_pseudo.pl
 7 - Note improvement
 8 - Enable pseudonymization in sytem preferences
 9 - Perform some checkouts and returns
10 - Verify the background jobs complete successfully
11 - Verify the pseudonymized_transactions table is updated correctly
Comment 6 Nick Clemens (kidclamp) 2023-11-07 16:57:21 UTC
Created attachment 158608 [details] [review]
Bug 35277: Unit tests

prove -v t/db_dependent/Koha/BackgroundJob/PseudonymizeStatistic.t
Comment 7 Martin Renvoize 2023-11-07 17:24:37 UTC
Created attachment 158610 [details] [review]
Bug 35277: Pseudonymize in a background job

This patch adds a new background job for pseduonymization

To test:
 1 - Download the benchmarking script
 2 - Ensure your koha has bcrypt_settings in koha-conf.xml
     See bug 28911
 3 - perl perf_check_pseudo.pl
 4 - Note the slowdown after pseudonymization enabled
 5 - Apply patches, restart all
 6 - perl perf_check_pseudo.pl
 7 - Note improvement
 8 - Enable pseudonymization in sytem preferences
 9 - Perform some checkouts and returns
10 - Verify the background jobs complete successfully
11 - Verify the pseudonymized_transactions table is updated correctly

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2023-11-07 17:24:40 UTC
Created attachment 158611 [details] [review]
Bug 35277: Unit tests

prove -v t/db_dependent/Koha/BackgroundJob/PseudonymizeStatistic.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Marcel de Rooy 2023-11-10 09:30:37 UTC
No Assignee.
Comment 10 Marcel de Rooy 2023-11-10 09:34:36 UTC
 FAIL   t/db_dependent/Koha/BackgroundJob/PseudonymizeStatistic.t
   FAIL   valid
                Can't locate Carp/Always.pm in @INC (you may need to install the Carp::Always module) (@INC contains: /usr/share/koha /usr/share/koha/lib /app/qatools /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.32.1 /usr/local/share/perl/5.32.1 /usr/lib/x86_64-linux-gnu/perl5/5.32 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl-base /usr/lib/x86_64-linux-gnu/perl/5.32 /usr/share/perl/5.32 /usr/local/lib/site_perl /var/lib/koha/myclone/plugins)
Comment 11 Nick Clemens (kidclamp) 2023-11-10 11:47:11 UTC
Created attachment 158802 [details] [review]
Bug 35277: (follow-up) Remove debugging module
Comment 12 Victor Grousset/tuxayo 2023-11-12 01:15:43 UTC
Partial QA: Code looks good to me. Though I don't know if there are pitfalls when moving a processing to a background job. That looks like a very simple and minimal case of background jobs usage.
Comment 13 Marcel de Rooy 2023-11-17 07:35:09 UTC
Sorry, string freeze.
Comment 14 Marcel de Rooy 2023-12-01 10:18:29 UTC
+use Modern::Perl;
+
+use Koha::PseudonymizedTransactions;
+
+use base 'Koha::BackgroundJob';
+
+=head1 NAME
+
+Koha::BackgroundJob::BatchCancelHold - Batch cancel holds



Copy and paste stuff? Please check the rest too.
Comment 15 Kyle M Hall 2023-12-01 11:17:52 UTC
Created attachment 159440 [details] [review]
Bug 35277: (QA follow-up) Fix POD
Comment 16 Marcel de Rooy 2024-01-12 09:49:32 UTC
Revisiting now
Comment 17 Marcel de Rooy 2024-01-12 10:03:54 UTC
Created attachment 160908 [details] [review]
Bug 35277: Pseudonymize in a background job

This patch adds a new background job for pseduonymization

To test:
 1 - Download the benchmarking script
 2 - Ensure your koha has bcrypt_settings in koha-conf.xml
     See bug 28911
 3 - perl perf_check_pseudo.pl
 4 - Note the slowdown after pseudonymization enabled
 5 - Apply patches, restart all
 6 - perl perf_check_pseudo.pl
 7 - Note improvement
 8 - Enable pseudonymization in sytem preferences
 9 - Perform some checkouts and returns
10 - Verify the background jobs complete successfully
11 - Verify the pseudonymized_transactions table is updated correctly

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2024-01-12 10:03:57 UTC
Created attachment 160909 [details] [review]
Bug 35277: Unit tests

prove -v t/db_dependent/Koha/BackgroundJob/PseudonymizeStatistic.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Marcel de Rooy 2024-01-12 10:03:59 UTC
Created attachment 160910 [details] [review]
Bug 35277: (follow-up) Remove debugging module

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Marcel de Rooy 2024-01-12 10:04:02 UTC
Created attachment 160911 [details] [review]
Bug 35277: (QA follow-up) Fix POD

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Marcel de Rooy 2024-01-12 10:04:05 UTC
Created attachment 160912 [details] [review]
Bug 35277: (QA follow-up) POD corrections

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Katrin Fischer 2024-01-16 11:09:12 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 23 Jonathan Druart 2024-01-16 12:23:04 UTC
It broke Jenkins:

koha_1       | #   Failed test 'No tests run for subtest "Config does not exist"'
koha_1       | #   at t/db_dependent/Koha/Pseudonymization.t line 73.
koha_1       |     # Looks like you planned 12 tests but ran 1.
koha_1       | 
koha_1       | #   Failed test 'Koha::Anonymized::Transactions tests'
koha_1       | #   at t/db_dependent/Koha/Pseudonymization.t line 118.
koha_1       | Can't call method "hashed_borrowernumber" on an undefined value at t/db_dependent/Koha/Pseudonymization.t line 104.
Comment 24 Nick Clemens (kidclamp) 2024-01-16 14:27:45 UTC
Created attachment 161065 [details] [review]
Bug 35277: (follow-up) Fix tests

This patch adjusts the tests to mock the Background to ensure it is called
and directly generate the pseudonymized transactions for testing
Comment 25 Fridolin Somers 2024-01-17 09:28:47 UTC
Great work. But more enhancement no ?
Since there is additional_work_needed I do not backport to 23.11.x for now.
Comment 26 Katrin Fischer 2024-01-17 10:30:03 UTC
(In reply to Fridolin Somers from comment #25)
> Great work. But more enhancement no ?
> Since there is additional_work_needed I do not backport to 23.11.x for now.

Good catch.
The additional_work_needed was for the last patch - failing tests. I am removing it now. 

This was posted as a performance issue, not sure how severe. Nick?
Comment 27 Nick Clemens (kidclamp) 2024-01-18 11:54:03 UTC
(In reply to Katrin Fischer from comment #26)
> (In reply to Fridolin Somers from comment #25)
> > Great work. But more enhancement no ?
> > Since there is additional_work_needed I do not backport to 23.11.x for now.
> 
> Good catch.
> The additional_work_needed was for the last patch - failing tests. I am
> removing it now. 
> 
> This was posted as a performance issue, not sure how severe. Nick?

Depending on the setup it could be a severe issue, even in minimal setups the delay was noticeable. I would suggest for backport to 23.11 at least
Comment 28 Fridolin Somers 2024-02-02 10:54:46 UTC
OK i give it a try
Comment 29 Fridolin Somers 2024-02-09 08:08:57 UTC
Works fine in 23.11, its great :D
Comment 30 Fridolin Somers 2024-02-09 08:09:34 UTC
Pushed to 23.11.x for 23.11.03
Comment 31 Fridolin Somers 2024-02-09 08:24:39 UTC
Ah one reflexion.
Circulation operations are in a DB transaction.
But when they generate a backgroundjob this action will be outside transaction right ?
So if transaction fails the backgroundjob will still exist.
I bet this is very rare we will do with it ;)
Comment 32 Jonathan Druart 2024-02-09 08:57:28 UTC
(In reply to Fridolin Somers from comment #31)
> Ah one reflexion.
> Circulation operations are in a DB transaction.
> But when they generate a backgroundjob this action will be outside
> transaction right ?
> So if transaction fails the backgroundjob will still exist.
> I bet this is very rare we will do with it ;)

See bug 35819.