Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't render M2M fields on nested serializers, and other depth=1 performance improvements #3702

Merged
merged 9 commits into from
May 9, 2023

Conversation

glennmatthews
Copy link
Contributor

@glennmatthews glennmatthews commented May 4, 2023

Closes: #DNE

What's Changed

  • On nested serializers, exclude ManyToMany fields, Tag fields, and computed fields/relationships, and custom fields, in order to improve performance and reduce the number of SQL queries needed.
    • Added an is_nested property to the BaseModelSerializer class to facilitate implementing this sort of logic.
  • Add basic caching to TreeModel.display because this was calculated and re-calculated quite frequently when listing objects with related Locations.
  • Add some missing select_related entries on DeviceViewSet and PrefixViewSet that I noticed while profiling with DjDT.

Before (GET /api/dcim/locations/?depth=1):

Screenshot 2023-05-04 at 5 25 31 PM

After:

Screenshot 2023-05-04 at 5 25 46 PM

TODO

  • Document this clearly in the REST API documentation!
  • Explanation of Change(s)
  • Added change log fragment(s) (for more information see the documentation)
  • Attached Screenshots, Payload Example
  • Unit, Integration Tests
  • Documentation Updates (when adding/changing features)
  • n/a Example Plugin Updates (when adding/changing features)
  • Outline Remaining Work, Constraints from Design

@itdependsnetworks
Copy link
Contributor

Can you confirm my understanding:

  • depth 0 gets ManyToMany fields, Tag fields, computed fields/relationships, and custom fields
  • depth 1 and above fields can never get ManyToMany fields, Tag fields, computed fields/relationships, and custom fields

@glennmatthews
Copy link
Contributor Author

Can you confirm my understanding:

* depth 0 gets ManyToMany fields, Tag fields, computed fields/relationships, and custom fields

* depth 1 and above fields can never get ManyToMany fields, Tag fields, computed fields/relationships, and custom fields

Just to be explicitly clear:

  • at depth 0, the top-level object will include FK as well as M2M/Tag related objects by URL only, i.e.:
        {
            "id": "9bc9abcb-8ac0-4ad4-bba5-15d22a5d0299",
            "url": "http://localhost:8080/api/dcim/locations/9bc9abcb-8ac0-4ad4-bba5-15d22a5d0299/",
            "display": "AMER → USA → BLV",
            "created": "2023-04-28T14:47:10.351659Z",
            "last_updated": "2023-05-04T17:53:35.983034Z",
            "name": "BLV",
            "slug": "amer-usa-blv",
...
            "status": "http://localhost:8080/api/extras/statuses/cd3f3acc-56e7-47da-8711-bea391edd635/",
            "parent": "http://localhost:8080/api/dcim/locations/6428666a-c839-41b2-ae0e-048f7902011f/",
            "tags": [
                "http://localhost:8080/api/extras/tags/6d7f17bb-b050-4fd5-a910-3c22cf6e8cf8/"
            ]
        },
  • at depth 1, the top-level object will include FK, M2M and tag related objects as nested objects. The nested objects will include their own FK related objects by URL but will NOT (as changed by this PR) include M2M, Tags, Relationships, Computed Fields, even by URL:
        {
            "id": "9bc9abcb-8ac0-4ad4-bba5-15d22a5d0299",
            "url": "http://localhost:8080/api/dcim/locations/9bc9abcb-8ac0-4ad4-bba5-15d22a5d0299/",
...
            "location_type": {
                "id": "2c4b28ad-0bcd-43a0-98db-fc4a9cb1348d",
                "url": "http://localhost:8080/api/dcim/location-types/2c4b28ad-0bcd-43a0-98db-fc4a9cb1348d/",
                "display": "region → country → site",
                "notes_url": "http://localhost:8080/api/dcim/location-types/2c4b28ad-0bcd-43a0-98db-fc4a9cb1348d/notes/",
                "tree_depth": null,
                "created": "2023-04-28T14:46:29.483447Z",
                "last_updated": "2023-04-28T14:47:38.556059Z",
                "name": "site",
                "slug": "site",
                "description": "",
                "nestable": false,
                "parent": "http://localhost:8080/api/dcim/location-types/7240674f-984d-46dc-9d8d-4ffad964b44f/"
            },  <----<< NOTE: no `tags` on the `location_type` nested object
            "tenant": {
                "id": "523cbc1b-7c83-411e-b1e5-db07741caac4",
                "url": "http://localhost:8080/api/tenancy/tenants/523cbc1b-7c83-411e-b1e5-db07741caac4/",
                "display": "IT Facility AMER",
                "notes_url": "http://localhost:8080/api/tenancy/tenants/523cbc1b-7c83-411e-b1e5-db07741caac4/notes/",
                "created": "2023-04-28T14:46:48.494676Z",
                "last_updated": "2023-04-28T14:47:19.730230Z",
                "name": "IT Facility AMER",
                "description": "IT Facility for AMER region",
                "comments": "",
                "tenant_group": null
            },   <---<< NOTE: no `tags` on the `tenant` nested object
            "tags": [
                {
                    "id": "6d7f17bb-b050-4fd5-a910-3c22cf6e8cf8",
                    "url": "http://localhost:8080/api/extras/tags/6d7f17bb-b050-4fd5-a910-3c22cf6e8cf8/",
                    "display": "Remote",
                    "notes_url": "http://localhost:8080/api/extras/tags/6d7f17bb-b050-4fd5-a910-3c22cf6e8cf8/notes/",
                    "name": "Remote",
                    "slug": "remote",
                    "created": "2023-05-04T17:52:56.242959Z",
                    "last_updated": "2023-05-04T17:52:56.243007Z",
                    "color": "ff9800",
                    "description": ""
                }  <----<< NOTE: no `content_types` on the `tags` related objects
            ]
        },

How exactly we handle Relationships on the base object at depth 0 vs depth 1 is I think something we're still sorting out.

except self.DoesNotExist:
# Expected to occur at times during bulk-delete operations
pass
finally:
display_str += self.name
cache.set(cache_key, display_str, 5)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to cache the display name on the model and update the cache for the instance and all descendants any time the parent or name fields change? If the parent and name fields don't change often would that be more performant?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably would be more performant in the common case (as these typically wouldn't change often), but might be a bit more complex to code in. I can look into it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cascading cache updates might be something we want to look into for dynamic group reversibility performance but for now, I think this is a valuable add.

If we like the pattern for using Redis for caching it makes it easier to do invalidation but I could also see a JSONField "cache" entry on objects for DB-backed caching:

>>> location._cache
{
    "display": ["Grad-grandparent Location", "Grandparent Location", "Parent Location", "Location"]
}

@gsnider2195
Copy link
Contributor

The nested objects will include their own FK related objects by URL but will NOT (as changed by this PR) include M2M, Tags, Relationships, Computed Fields, even by URL

Could this be opt-in behavior? I could see this causing confusion when the top level object includes the m2m fields but they're missing from the nested objects.

@glennmatthews
Copy link
Contributor Author

The nested objects will include their own FK related objects by URL but will NOT (as changed by this PR) include M2M, Tags, Relationships, Computed Fields, even by URL

Could this be opt-in behavior? I could see this causing confusion when the top level object includes the m2m fields but they're missing from the nested objects.

That's definitely a valid concern. The issue is that looking up these related objects by default makes depth=1 quite a bit slower than our nested-serializer approach was in v1.x.

Possibly an alternate approach would be, rather than having things behave differently based on their depth/nestedness, explicitly making all M2M fields (including tags) opt-in fields that aren't included in the API at all by default? Happy to discuss further.

@gsnider2195
Copy link
Contributor

The nested objects will include their own FK related objects by URL but will NOT (as changed by this PR) include M2M, Tags, Relationships, Computed Fields, even by URL

Could this be opt-in behavior? I could see this causing confusion when the top level object includes the m2m fields but they're missing from the nested objects.

That's definitely a valid concern. The issue is that looking up these related objects by default makes depth=1 quite a bit slower than our nested-serializer approach was in v1.x.

Possibly an alternate approach would be, rather than having things behave differently based on their depth/nestedness, explicitly making all M2M fields (including tags) opt-in fields that aren't included in the API at all by default? Happy to discuss further.

The absence of the key should be sufficient information to know that the serializer isn't including that key in the data instead of "tags": [] if tags were actually empty on the nested object.

@glennmatthews glennmatthews marked this pull request as ready for review May 5, 2023 19:19
@glennmatthews
Copy link
Contributor Author

Need a fix from #3696 here it looks like.

@bryanculver bryanculver added type: housekeeping Changes to the application which do not directly impact the end user type: feature Introduction of new or enhanced functionality to the application impact: breaking change This change or feature will remove or replace existing functionality; needs to be an X.0.0 release and removed type: housekeeping Changes to the application which do not directly impact the end user labels May 8, 2023
@bryanculver bryanculver added this to the v2.0.0 milestone May 8, 2023
@bryanculver bryanculver removed the impact: breaking change This change or feature will remove or replace existing functionality; needs to be an X.0.0 release label May 8, 2023
@bryanculver
Copy link
Member

Need a fix from #3696 here it looks like.

Does this need to be rebased now that #3696 is merged?

@glennmatthews
Copy link
Contributor Author

Need a fix from #3696 here it looks like.

Does this need to be rebased now that #3696 is merged?

I fixed it differently by explicitly adding the display and other fields back to the base class get_field_names in ad8d68f

Base automatically changed from next-ui to next May 9, 2023 02:52
@glennmatthews glennmatthews force-pushed the u/glennmatthews-improve-depth-1-performance branch from ee112b9 to 8743917 Compare May 9, 2023 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature Introduction of new or enhanced functionality to the application
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants