coronasafe / care

Care is a Digital Public Good enabling TeleICU & Decentralised Administration of Healthcare Capacity across States.
https://careapi.ohc.network/swagger
MIT License
224 stars 258 forks source link

Fix N+1 queries in `/api/v1/consultationbed/` #1339

Open sainak opened 1 year ago

sainak commented 1 year ago
Sentry Issue: CARE-KD
Transaction Name /api/v1/consultationbed/
Parent Span django.view - consultationbed-list
Repeating Spans (0) db - SELECT facility_assetlocation.id, facility_assetlocation.external_id, facility_assetlocation.created_date, facility_assetlocation.modified_date, facility_assetlocation.deleted, facility_assetlocation.name, facility_assetlocation.description, facility_assetlocation.location_type, facility_assetlocation.facility_id FROM facility_assetlocation WHERE facility_assetlocation.id = %s
db - SELECT facility_facility.id, facility_facility.external_id, facility_facility.created_date, facility_facility.modified_date, facility_facility.deleted, facility_facility.name, facility_facility.is_active, facility_facility.verified, facility_facility.facility_type, facility_facility.kasp_empanelled, facility_facility.features, facility_facility.longitude, facility_facility.latitude, facility_facility.pincode, facility_facility.address, "facility...
db - SELECT (1) AS a FROM facility_consultationbed WHERE (facility_consultationbed.deleted = %s AND facility_consultationbed.bed_id = %s AND facility_consultationbed.end_date IS NULL) LIMIT 1
nikhilagastya commented 1 year ago

Hey @sainak can you assign me this issue and brief me a little on this !

GeekGawd commented 1 year ago

@sainak @nikhilagastya

This is what I have gotten from my reasearch...

First query comes from ConsultationBedSerializer -> BedSerializer (bed_object) -> AssetLocationSerializer (location_object)

Second query comes from ConsultationBedSerializer -> ExternalIdSerializerField(consultation) -> queryset (attr) -> PatientConsultation.objects.all()

Third query is always called because of the property in Bed Model in ./facility/models/bed.py @property def is_occupied(self) -> bool: return ConsultationBed.objects.filter(bed=self, end_date__isnull=True).exists()

I evaluated it by making a custom middleware and wrapping the request/response in that

   class SQLLoggingMiddleware:
    def __init__(self, get_response):
        self.get_response = get_response

    def __call__(self, request):
        response = self.get_response(request)
        sql_queries = connection.queries
        for query in sql_queries:
            logger.info(f"SQL Query: {query['sql']} - Time: {query['time']}")
        reset_queries()
        return response

Did some backtracking and logging got where they came from. I don't really see any optimisation can be done here, as they are coming from Serializer fields. Should we try passing the queryset from the view to Serializer?

vigneshhari commented 1 year ago

You can use the django extentions module and use runserver_plus --print-sql to print all SQL queries

GeekGawd commented 1 year ago

@vigneshhari actually didn't know that! Thank You! My current solution also does the same thing but will keep it mind for the future.

Though I wanted to say that I don't see much scope in improving the queries, could you help me in that regard.

vigneshhari commented 1 year ago

you could prefetch some data and then use signals to cache the property data to ensure that everything works with a single query

vigneshhari commented 1 year ago

@GeekGawd if you are working on the issue, lemme know, I'll assign it to you

GeekGawd commented 1 year ago

@vigneshhari Aaahhhhhh... makes sense, Thank You! Yep will do, could you please assign the issue to me.

dhruv-goyal-10 commented 5 months ago

@sainak @vigneshhari I want to work on this issue. Can you please assign this issue to me?