Skip to content

allow disabling profile logic in NodeGetListQuery #5971

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

Open
wants to merge 8 commits into
base: stable
Choose a base branch
from

Conversation

ajtmccarty
Copy link
Contributor

@ajtmccarty ajtmccarty commented Mar 7, 2025

adds a new env var INFRAHUB_EXPERIMENTAL_NO_PROFILES that removes the profile-related logic from NodeGetListQuery and NodeManager.get_many. this results in a substantially simpler NodeGetListQuery cypher query and an improvement in performance. my local testing showed perhaps a 10% improvement in performance when running a single query with and without the profiles logic, but I am not completely certain that there is actually a performance improvement. perhaps running one of the large generators the team has been working on is the best way to determine if this update actually improves performance

this new env var also removes any profile-related mutations from GraphQL schema. this caused toast errors on the front end, so removed it

if a user has profiles linked to objects and then turns activates this setting, queries for objects linked to existing profiles will no longer be correct. I'm not sure if there are more guardrails that we should put in place for this

@github-actions github-actions bot added the group/backend Issue related to the backend (API Server, Git Agent) label Mar 7, 2025
Copy link

codspeed-hq bot commented Mar 7, 2025

CodSpeed Performance Report

Merging #5971 will not alter performance

Comparing ajtm-03072025-node-get-list-wo-profiles (cd1959c) with stable (4b05693)

Summary

✅ 10 untouched benchmarks

Copy link
Collaborator

@dgarros dgarros left a comment

Choose a reason for hiding this comment

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

Would be good to validate if the frontend is able to work correctly when the profiles are disabled before merging this PR
if it doesn't work we might need to keep the GraphQL queries related to profiles for now

@fatih-acar
Copy link
Contributor

I get a rough 50% improvement on NodeGetList when running the following query:

query {
  NetworkPhysicalInterfaceL3(name__value: "Ethernet1/1/1", order: {disable: true}) {
    edges {
      node {
        id
      }
    }
  }
}

@fatih-acar
Copy link
Contributor

Not really the goal of this PR, but could we also do the same for NodeListGetInfoQuery? That would fully remove profiles overhead in the graphql query path.

@ajtmccarty ajtmccarty force-pushed the ajtm-03072025-node-get-list-wo-profiles branch from 8163b25 to cd1959c Compare March 11, 2025 00:02
@ajtmccarty
Copy link
Contributor Author

  • removed the code the turned off profile-related mutations if the new NO_PROFILES setting was active
  • updated NodeManager.get_many/NodeListGetInfoQuery to remove profile-related logic, depending on the new env var as well

@ajtmccarty ajtmccarty marked this pull request as ready for review March 11, 2025 00:06
@fatih-acar
Copy link
Contributor

After more thorough testing, I confirm what you've seen: we get a 10 to 20% improvement.
Still need to test it using the test framework with the generator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
group/backend Issue related to the backend (API Server, Git Agent)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants