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

[ShopifyVM] Add schema analyzer using blue to function-runner #343

Closed
wants to merge 45 commits into from

Conversation

mssalemi
Copy link
Contributor

@mssalemi mssalemi commented Aug 20, 2024

#gsd37875

Addresses:

Description

Current scaling implementation: https://github.com/Shopify/shopify/pull/518219/files#diff-bef9ccfcca30dfc391c6ad9d0a3e0739bf8a2ab27e659473894e4e62d43bad08

We currently scale our function run limits based on the directive scaleLimits with an argument rate on cartLines. We scale by the length of the field. We multiply the directive rate found on the fields definition, by the length of that fields (seem by either array length or string length) to determine how much to scale limits our default limits by.

This PR adds an analyzer that will determine the maximum scale factor from all fields with the scale limits directive on it. We chose to use this implementation because we want to ability to add directives for scale limits of different fields in the future. Currently we only scale on cart lines.

Note: Currently have a few .graphql and json files used for testing, I will remove these before merging, but will keep in this PR for now incase anyone wants to tophat.

Run Commands:

See Scaled Limits

cargo run -- -f './tests/fixtures/build/js_function.wasm' -i './tests/fixtures/input/js_function_input_large_willnotmerge.json' -s='./tests/fixtures/schema/schema.graphql' -q='./tests/fixtures/query/query.graphql'

We will traverse the query alongside the input, and determine the scale factor.

Example Query:

query {
  cart {
    lines {
      quantity
    }
  }
}

Notes on Visitor implementation:

Initialization: An instance of ScaleLimits is created, initializing the value_stack with the entire input JSON. The path_stack and rates are initialized as empty. The path stack, will keep track of the current field we are visiting. The value stack will keep track of what part of the json we are focused on. The rates hashmap will be used to track different rates (different fields with the directive).

Visiting cart Field: The visitor enters the cart field.

  • "cart" is pushed onto the path_stack.
  • The value_stack is updated to focus on the value of "cart" from the input JSON.

Visiting lines Field: The visitor moves to the lines field inside cart.

  • "lines" is pushed onto the path_stack.
  • The value_stack is updated to focus on the value of "lines".
  • The rate_for_field_definition function retrieves the scaling rate (0.005) for the lines field from the schema.
  • The scaling factor is calculated based on the length of the lines array.
  • The rates hashmap is updated with the path as the key and the calculated scaling increment as the value.

Final Output: After the traversal is complete, the into_output method of the Analyzer trait is called.

  • This method clamps the result between minimum and maximum scale factors (1.0 to 10.0).

After we call the analyzer, we then use that result when we display the output. I applied the same logic in showing red color for the usage when the run is over the limit, and regular text when it is within the limits. Limts are now calculated by multiplying the scaling rate determined by the analyzer, to the defaults.

Tophatting

Normal Cart:

Screenshot 2024-08-28 at 11 39 16 AM

With Scaled Limits:

Screenshot 2024-08-28 at 11 41 39 AM
cargo run -- -f './tests/fixtures/build/js_function.wasm' -i './tests/fixtures/input/js_function_input_large.json' -s='yolo-schema.graphql' -q='yolo-query.graphql'

These files no longest exist in this PR, but can copy them over from any the spin app.

When Query or Schema are not passed to function-runner

  • these are optional parameters for function runner
  • when they are not provided, we will use the defaults for determined the output of the limits
Screenshot 2024-08-28 at 12 15 00 PM

@mssalemi mssalemi force-pushed the ms.function-runner-schema-analyzer branch from 1e3cad1 to 09dce95 Compare August 21, 2024 15:45
@mssalemi mssalemi force-pushed the ms.function-runner-schema-analyzer branch from d486f94 to 7742cfb Compare August 22, 2024 20:42
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/scale_limits_analyzer.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@mssalemi mssalemi force-pushed the ms.function-runner-schema-analyzer branch from f2969a4 to a5d5782 Compare August 28, 2024 15:21
@@ -0,0 +1,7 @@
query {
Copy link
Contributor Author

@mssalemi mssalemi Aug 28, 2024

Choose a reason for hiding this comment

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

Left in for tophatting, will not merge. Same with other .graphql and json files added.

@mssalemi mssalemi changed the title [Draft] Add schema analyzer using blue to function-runner [ShopifyVM] Add schema analyzer using blue to function-runner Aug 28, 2024
@mssalemi mssalemi marked this pull request as ready for review August 28, 2024 16:21
Copy link
Contributor

@jeffcharles jeffcharles left a comment

Choose a reason for hiding this comment

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

Looking better!

src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@jeffcharles jeffcharles left a comment

Choose a reason for hiding this comment

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

Looks good to me! I'll defer to others on the logic for analyzing the schema though. Also I'll be out of office for the next two weeks.

src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
let cache =
bluejay_validator::executable::Cache::new(&executable_document, &schema_definition);

let input_str = to_json_string(input).unwrap_or_else(|_| "<invalid JSON>".to_string());
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of passing along <invalid JSON>, should we just return an error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can remove this error here, looking at code again, we check if the input is valid here. (Although we create a string from that buffer).

Given that, I think we can just propagate this error up one level, and when we catch it in the main.rs, we will just default to the 1.0.

@mssalemi mssalemi force-pushed the ms.function-runner-schema-analyzer branch from ad53e51 to c8ea84c Compare September 4, 2024 19:37
@mssalemi mssalemi marked this pull request as draft September 5, 2024 12:30
@mssalemi
Copy link
Contributor Author

mssalemi commented Sep 5, 2024

Splitting this up into 2 PRs so we ca isolate the analyzer and then the usage in function-runner.

Appreciate all the the reviews on the PR, i will re-tag on the new ones.

@jacobsteves
Copy link
Member

Closing as this was implemented in another PR.

@jacobsteves jacobsteves closed this Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants