feat(schema): Support "key" in throttle schema and the scope of "action" #757

kola-er · 2024-03-21T15:01:33Z

feat(schema): Support "key" in throttle schema and the scope of "action"

standielpls · 2024-03-21T19:04:06Z

packages/schema/docs/build/schema.md

@@ -2080,14 +2080,24 @@ Key | Required | Type | Description
 --- | -------- | ---- | -----------
 `window` | **yes** | `integer` | The timeframe, in seconds, within which the system tracks the number of invocations for an action. The number of invocations begins at zero at the start of each window.
 `limit` | **yes** | `integer` | The maximum number of invocations for an action, allowed within the timeframe window.
-`scope` | no | `array`[`string` in (`'user'`, `'auth'`, `'account'`)] | The granularity to throttle by. You can set the scope to one or more of the following: 'user' - Throttles based on user ids.  'auth' - Throttles based on auth ids. 'account' - Throttles based on account ids for all users under a single account. By default, throttling is scoped to the account.
+`key` | no | `string` | The key to throttle with in combination with the scope. This should be unique to the operation. While actions of different integrations with the same key and scope will never share the same limit, actions of the same integration with the same key and scope will do when "action" is not in the scope. User data provided for the input fields can be used in the key with the use of the curly braces referencing. For example, to access the user data provided for the input field "test_field", use `{{bundle.inputData.test_field}}`. Note that a required input field should be referenced to get user data always.


Is it worth mentioning what happens if the key is not provided?

That may be helpful, although implied with "in combination with the scope".

Perhaps even more helpful would be to explain key and scope together in the description of the schema instead of here individually in the props.

We could explain that throttling will be shared by all actions with the same key and that the values referenced by scope are mixed into it, and that scope defaults to ['account'].

Pseudo code may also help here:

const usedKey = (scope || ['account']) .map(s => values[s]) .concat(key, app) .join('-');

Same likely applies to LockObjectSchema.

Updated the descriptions by moving the implication of same key, window, and scope to the schema description.

standielpls · 2024-03-21T19:41:30Z

packages/schema/lib/schemas/ThrottleObjectSchema.js

@@ -19,11 +19,17 @@ module.exports = makeSchema({
        'The maximum number of invocations for an action, allowed within the timeframe window.',
      type: 'integer',
    },
+    key: {
+      description:
+        'The key to throttle with in combination with the scope. This should be unique to the operation. While actions of different integrations with the same key and scope will never share the same limit, actions of the same integration with the same key and scope will do when "action" is not in the scope. User data provided for the input fields can be used in the key with the use of the curly braces referencing. For example, to access the user data provided for the input field "test_field", use `{{bundle.inputData.test_field}}`. Note that a required input field should be referenced to get user data always.',


Can the user not provide a key?

What happens in the scenario where a user uses the same key in two actions, but has different values?

For example:

update_row: { ... throttle: { key: 'row_level_action', limit: 50, window: 60, scope: ['account'] } } delete_row: { ... throttle: { key: 'row_level_action', limit: 1000, window: 60, scope: ['account'] } }

Good question!

Looking at https://gitlab.com/zapier/zapier/-/blob/fb1bdfa5dbb019ddac4a599e2d303c13cd3a712d/web/backend/zapier/useful/throttle_tools.py#L133-135

window is mixed into the key, so if those don't match they won't affect each other. That should be called out in the above description (comment).

limit is applied per action, so if in your example window would be identical, then within that window update_row would throttle at 50, but delete_row would happily continue up to 1000. 🤔

Can the user not provide a key?

Yes, the key is not required.

What happens in the scenario where a user uses the same key in two actions, but has different values?

I didn't observe that the window is mixed into the key as @FokkeZB pointed out. Responding to the example, using the new information, both actions would share the same final key having the same key, scope excluded of "action", and window. With both using different limits, the action to first run an invocation within each 60s timeframe would determine the same limit that would be applied to both actions' invocations until the 60s timeframe is completed. So both limits of 50 and 1000 could be used but not both in the same 60s timeframe.

To prevent this uncertainty/complication, perhaps we mix the limit in the final key?

I see - I think there's 3 ways to go about this then:

Remove window from key as Fokke suggested

Add limit to the key as Kola suggested

Add a validation assertion to this schema: if a throttle key is defined, the limit and key must be equal and when the user runs zapier validate they will see a validation error.

1 and 2 carry the risk of unintentional behaviour, where a user may report that they thought their action will throttle at X/Y but is actually doing A/B. But we can be very loud about documenting this behaviour to try and mitigate this.

While 3 carries more work and effort, I think this will lead to implementing the original intent behind adding a throttle key.

I'm fine with any - if 1/2, then I suggest adding a clear message and maybe a snippet of the above example
@FokkeZB @kola-er wdyt?

That seems reasonable, thanks for discussing this!

I am happy to move forward with adding the limit value to the final key

Okay, will include it in the backend code and update the docs here too. Thanks @standielpls

@standielpls Updated the description to note that all the configurations are combined to form the final key in the last commit.

This makes sense. Thanks all.

Also see https://gitlab.com/zapier/zapier/-/merge_requests/56351#note_1830368072

FokkeZB

Approving, but left a comment to improve docs, And @standielpls raised an excellent point that also should be documented or perhaps considered as change (dropping window from the key?)

FokkeZB · 2024-03-22T09:49:34Z

packages/schema/docs/build/schema.md

@@ -2080,14 +2080,24 @@ Key | Required | Type | Description
 --- | -------- | ---- | -----------
 `window` | **yes** | `integer` | The timeframe, in seconds, within which the system tracks the number of invocations for an action. The number of invocations begins at zero at the start of each window.
 `limit` | **yes** | `integer` | The maximum number of invocations for an action, allowed within the timeframe window.
-`scope` | no | `array`[`string` in (`'user'`, `'auth'`, `'account'`)] | The granularity to throttle by. You can set the scope to one or more of the following: 'user' - Throttles based on user ids.  'auth' - Throttles based on auth ids. 'account' - Throttles based on account ids for all users under a single account. By default, throttling is scoped to the account.
+`key` | no | `string` | The key to throttle with in combination with the scope. This should be unique to the operation. While actions of different integrations with the same key and scope will never share the same limit, actions of the same integration with the same key and scope will do when "action" is not in the scope. User data provided for the input fields can be used in the key with the use of the curly braces referencing. For example, to access the user data provided for the input field "test_field", use `{{bundle.inputData.test_field}}`. Note that a required input field should be referenced to get user data always.


That may be helpful, although implied with "in combination with the scope".

Perhaps even more helpful would be to explain key and scope together in the description of the schema instead of here individually in the props.

We could explain that throttling will be shared by all actions with the same key and that the values referenced by scope are mixed into it, and that scope defaults to ['account'].

Pseudo code may also help here:

const usedKey = (scope || ['account']) .map(s => values[s]) .concat(key, app) .join('-');

Same likely applies to LockObjectSchema.

FokkeZB · 2024-03-22T09:56:33Z

packages/schema/lib/schemas/ThrottleObjectSchema.js

@@ -19,11 +19,17 @@ module.exports = makeSchema({
        'The maximum number of invocations for an action, allowed within the timeframe window.',
      type: 'integer',
    },
+    key: {
+      description:
+        'The key to throttle with in combination with the scope. This should be unique to the operation. While actions of different integrations with the same key and scope will never share the same limit, actions of the same integration with the same key and scope will do when "action" is not in the scope. User data provided for the input fields can be used in the key with the use of the curly braces referencing. For example, to access the user data provided for the input field "test_field", use `{{bundle.inputData.test_field}}`. Note that a required input field should be referenced to get user data always.',


Good question!

Looking at https://gitlab.com/zapier/zapier/-/blob/fb1bdfa5dbb019ddac4a599e2d303c13cd3a712d/web/backend/zapier/useful/throttle_tools.py#L133-135

window is mixed into the key, so if those don't match they won't affect each other. That should be called out in the above description (comment).

limit is applied per action, so if in your example window would be identical, then within that window update_row would throttle at 50, but delete_row would happily continue up to 1000. 🤔

IQQ-814 Add support for throttling across actions and "key" field

eb58f66

kola-er requested a review from standielpls March 21, 2024 15:01

kola-er requested review from eliangcs and rnegron as code owners March 21, 2024 15:01

standielpls reviewed Mar 21, 2024

View reviewed changes

FokkeZB approved these changes Mar 22, 2024

View reviewed changes

Add more explanation for edge cases for the throttle schema

fb9b377

kola-er mentioned this pull request Mar 25, 2024

docs(cli): Update docs on throttle config object #760

Merged

standielpls previously approved these changes Mar 25, 2024

View reviewed changes

Noted that limit is mixed into the final key for throttling

49df28b

kola-er dismissed standielpls’s stale review via 49df28b March 25, 2024 14:40

standielpls approved these changes Mar 25, 2024

View reviewed changes

kola-er merged commit f003657 into main Mar 25, 2024
13 checks passed

kola-er deleted the IQQ-814 branch March 25, 2024 15:43

eliangcs mentioned this pull request Mar 26, 2024

PDE-4798 chore: release 15.6.0 #758

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(schema): Support "key" in throttle schema and the scope of "action" #757

feat(schema): Support "key" in throttle schema and the scope of "action" #757

kola-er commented Mar 21, 2024

standielpls Mar 21, 2024

FokkeZB Mar 22, 2024

kola-er Mar 22, 2024

standielpls Mar 21, 2024

standielpls Mar 21, 2024

FokkeZB Mar 22, 2024 •

edited

Loading

kola-er Mar 22, 2024 •

edited

Loading

standielpls Mar 22, 2024

standielpls Mar 25, 2024

standielpls Mar 25, 2024

kola-er Mar 25, 2024

kola-er Mar 25, 2024

FokkeZB Mar 25, 2024

FokkeZB left a comment

FokkeZB Mar 22, 2024

FokkeZB Mar 22, 2024 •

edited

Loading

feat(schema): Support "key" in throttle schema and the scope of "action" #757

feat(schema): Support "key" in throttle schema and the scope of "action" #757

Conversation

kola-er commented Mar 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FokkeZB Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

kola-er Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FokkeZB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FokkeZB Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

FokkeZB Mar 22, 2024 •

edited

Loading

kola-er Mar 22, 2024 •

edited

Loading

FokkeZB Mar 22, 2024 •

edited

Loading