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

Consider updated fields for Jinja2 based computed attributes #5307

Merged
merged 1 commit into from
Dec 23, 2024

Conversation

ogenstad
Copy link
Contributor

@ogenstad ogenstad commented Dec 23, 2024

Changes in this PR

  • The computed attribute mutation didn't manage the update in a transaction so I've wrapped the save() method within a new transaction
  • Changed the node mutated event to include a "fields" key in the payload (due to this I also had to modify the webhook as it gained an extra level of "indentation", we'll probably refactor the webhook completely in a not to distant future regardless)
  • Use these updated fields as part of the lookup to find computed attributes in the schema. The goal is to only run the computed attribute calculation if a relevant attribute/relationship has been updated.

Note in this iteration I've only isolated the change to when we are actually running the Computed Attributes, it will save us from querying the backend with the SDK when we know that it isn't strictly required.

I view this as an initial step to improve the performance a bit. The next iteration of this would be to modify the event automations so that the triggers themselves have these fields built in. I think it would be nice to refactor the automations in general as we'll end up with potentially quite a few additional automations when this is in place. Most of the code for this was already in place since before we started using it as automations in Prefect but the filtering was ignored as we always sent in an empty list for "updated_fields".

Also I really dislike that I did with the "update_fields" parameter and the use of ujson, we have this with the webhooks too. Would like a cleaner solution.

Final note: To improve things in a similar way for the Transforms I think we really have to spend time to rewrite and enhance the GraphQLQueryAnalyzer.

@github-actions github-actions bot added the group/backend Issue related to the backend (API Server, Git Agent) label Dec 23, 2024
@ogenstad ogenstad force-pushed the pog-computed-attribute-jinja-updated-fields branch from 8047f39 to 57327c1 Compare December 23, 2024 14:31
Copy link

codspeed-hq bot commented Dec 23, 2024

CodSpeed Performance Report

Merging #5307 will not alter performance

Comparing pog-computed-attribute-jinja-updated-fields (4942eb7) with release-1.1 (530fb03)

Summary

✅ 10 untouched benchmarks

@ogenstad ogenstad force-pushed the pog-computed-attribute-jinja-updated-fields branch 2 times, most recently from 6ded313 to ac64cdc Compare December 23, 2024 15:15
@ogenstad ogenstad marked this pull request as ready for review December 23, 2024 16:19
@ogenstad ogenstad requested a review from a team December 23, 2024 16:19
backend/infrahub/events/node_action.py Outdated Show resolved Hide resolved

graphql_payload = await target_node.to_graphql(db=context.db, filter_sensitive=True)
graphql_payload = await target_node.to_graphql(db=dbt, filter_sensitive=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if this matters here, but I have encountered problems where changes are not written to the database until the transaction is closed and that prevents us from getting the latest changes.
so maybe this to_graphql should be outside of the transaction

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok! I moved this part out of the transaction.

@ogenstad ogenstad force-pushed the pog-computed-attribute-jinja-updated-fields branch from ac64cdc to b98eb81 Compare December 23, 2024 20:22
@ogenstad ogenstad force-pushed the pog-computed-attribute-jinja-updated-fields branch from b98eb81 to 4942eb7 Compare December 23, 2024 20:26
@ogenstad ogenstad merged commit 9b2562d into release-1.1 Dec 23, 2024
34 checks passed
@ogenstad ogenstad deleted the pog-computed-attribute-jinja-updated-fields branch December 23, 2024 21:36
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