Pull all addons in one DB request #5289

dhruvkb · 2024-12-18T11:43:34Z

Fixes

Description

This PR prevents audio list requests with ?peaks=true from making N+1 queries by fetching all AudioAddon models from the DB in a single call.

Testing Instructions

With DEBUG logging enabled, make API calls to the /v1/audio/ endpoint with ?peaks=true. You should see only 2 total calls to the DB, where the second call is for audio addons:

SELECT
  "api_audioaddon"."created_on",
  "api_audioaddon"."updated_on",
  "api_audioaddon"."audio_identifier",
  "api_audioaddon"."waveform_peaks"
FROM "api_audioaddon"
WHERE "api_audioaddon"."audio_identifier" IN (
  '134d47ed3cd448f1b34e2671a428b463'::uuid,
  -- more UUIDs
  '2708c8f651254cbd992373e55359f1a8'::uuid
);

Checklist

My pull request has a descriptive title (not a vague title likeUpdate index.md).
My pull request targets the default branch of the repository (main) or a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added or updated tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.
I ran the DAG documentation generator (ov just catalog/generate-docs for catalog
PRs) or the media properties generator (ov just catalog/generate-docs media-props
for the catalog or ov just api/generate-docs for the API) where applicable.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

dhruvkb · 2024-12-18T12:15:22Z

Drafting so that I can update the tests.

obulat · 2024-12-18T13:21:18Z

@dhruvkb, how do we enable debug mode? I tried setting DJANGO_LOG_LEVEL to "DEBUG", but can't see the queries.

dhruvkb · 2024-12-18T17:03:47Z

@obulat can you try again with DJANGO_DATABASE_LOGGING configured to true?

openverse/api/conf/settings/logging.py

Line 24 in a048c41

    
           DJANGO_DATABASE_LOGGING = config("DJANGO_DATABASE_LOGGING", cast=bool, default=False)

AetherUnbound

I'm not able to see the DB queries, even after adding DJANGO_DATABASE_LOGGING=true to the api/.env file 🤔 is there a different way I should be doing that? It's under ov just logs web, right?

AetherUnbound · 2024-12-18T22:40:00Z

api/test/unit/models/test_audio.py

-
-@pytest.mark.django_db
-@mock.patch("api.models.audio.AudioAddOn.objects.get")
-def test_audio_waveform_sent_when_present(get_mock, audio_fixture):


Should there be new tests for the modified functionality of get_db_results with the include_addons parameter?

Good call, I'll add some tests for that asap.

dhruvkb · 2024-12-19T06:59:38Z

@AetherUnbound can you try this and see if you can see the DB logs?

krysal

I can see the DB queries in logs when these variables are set this way:

DJANGO_LOG_LEVEL=DEBUG
DJANGO_DATABASE_LOGGING=True

And indeed, there are no more N+1 queries when peaks are requested 😄 I support @AetherUnbound comment tho. It would be nice to have some tests on this functionality.

dhruvkb · 2024-12-19T18:55:31Z

@AetherUnbound and @krysal, do you think the test cases introduced in 51e1c3d are sufficient to cover that the addons are fetched and that at most 1 extra query is introduced for peaks?

obulat

The added tests fail on main 👍

obulat · 2024-12-20T09:33:50Z

api/api/views/media_views.py

@@ -147,6 +159,20 @@ def _validate_source(self, source):
                detail=f"Invalid source '{source}'. Valid sources are: {valid_string}.",
            )

+    def include_addons(self, serializer):


Are addons described anywhere in the codebase docs? Do we only have waveforms as addons now, or is there something else, too?
I would like for this comment to include something like "addons, like waveforms for audio". A description of why something is an addon and not in the main model (if it doesn't exist anywhere now) , would be very useful. Not necessarily in this PR, though :)

The use of the addon model is specific to the audio media type and that too only for the waveforms. The AudioAddOn model does not have any docstrings, so the conversation around the PR that added this model WordPress/openverse-api#510 is our only reference.

The PR describes that the reason the waveforms are in an AudioAddOn model is because the data in any columns that are not in the upstream DB would be gone after a data refresh.

AetherUnbound · 2024-12-22T00:30:22Z

I can see the DB queries in logs when these variables are set this way:
DJANGO_LOG_LEVEL=DEBUG
DJANGO_DATABASE_LOGGING=True
And indeed, there are no more N+1 queries when peaks are requested 😄 I support @AetherUnbound comment tho. It would be nice to have some tests on this functionality.

Ahh, I was probably missing the DJANGO_LOG_LEVEL=DEBUG too! My bad!

Pull all addons in one DB request

2880954

dhruvkb requested a review from a team as a code owner December 18, 2024 11:43

dhruvkb requested review from krysal and obulat and removed request for a team December 18, 2024 11:43

openverse-bot added 🧱 stack: api Related to the Django API 🟧 priority: high Stalls work on the project or its dependents 🛠 goal: fix Bug fix 💻 aspect: code Concerns the software code in the repository labels Dec 18, 2024

dhruvkb marked this pull request as draft December 18, 2024 12:15

dhruvkb added 2 commits December 18, 2024 21:30

Use .get to prevent KeyErrors

4ce3907

Update tests

35b39e5

dhruvkb marked this pull request as ready for review December 18, 2024 17:46

AetherUnbound reviewed Dec 19, 2024

View reviewed changes

krysal reviewed Dec 19, 2024

View reviewed changes

dhruvkb added 2 commits December 19, 2024 22:53

Allow media types to handle their own criteria for fetching addons

e67edbc

Test that the list view makes 1 or 2 DB calls based on addons

51e1c3d

obulat approved these changes Dec 20, 2024

View reviewed changes

dhruvkb merged commit 4a85e14 into main Dec 20, 2024
52 checks passed

dhruvkb deleted the fix_n_plus_one branch December 20, 2024 09:55

obulat mentioned this pull request Dec 22, 2024

Add instructions for debug database logging in the API to the docs #5299

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pull all addons in one DB request #5289

Pull all addons in one DB request #5289

dhruvkb commented Dec 18, 2024

dhruvkb commented Dec 18, 2024

obulat commented Dec 18, 2024

dhruvkb commented Dec 18, 2024

AetherUnbound left a comment

AetherUnbound Dec 18, 2024

dhruvkb Dec 19, 2024

dhruvkb commented Dec 19, 2024

krysal left a comment •

edited

Loading

dhruvkb commented Dec 19, 2024

obulat left a comment

obulat Dec 20, 2024

dhruvkb Dec 20, 2024

AetherUnbound commented Dec 22, 2024

Pull all addons in one DB request #5289

Pull all addons in one DB request #5289

Conversation

dhruvkb commented Dec 18, 2024

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

dhruvkb commented Dec 18, 2024

obulat commented Dec 18, 2024

dhruvkb commented Dec 18, 2024

AetherUnbound left a comment

Choose a reason for hiding this comment

AetherUnbound Dec 18, 2024

Choose a reason for hiding this comment

dhruvkb Dec 19, 2024

Choose a reason for hiding this comment

dhruvkb commented Dec 19, 2024

krysal left a comment • edited Loading

Choose a reason for hiding this comment

dhruvkb commented Dec 19, 2024

obulat left a comment

Choose a reason for hiding this comment

obulat Dec 20, 2024

Choose a reason for hiding this comment

dhruvkb Dec 20, 2024

Choose a reason for hiding this comment

AetherUnbound commented Dec 22, 2024

krysal left a comment •

edited

Loading