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

Add throttling configuration #66

Merged
merged 2 commits into from
Sep 1, 2024
Merged

Conversation

KiKoS0
Copy link
Collaborator

@KiKoS0 KiKoS0 commented Aug 31, 2024

Summary

Add throttling configuration to the InngestFunctionConfigBuilder.

A test is also included, but it may be challenging to verify if the function is throttled since it returns a status of Running. If the test fails due to timing issues, it might not be worth keeping.

Checklist

  • Update documentation
  • Added unit/integration tests

Related

@KiKoS0 KiKoS0 requested a review from albertchae August 31, 2024 13:41
@KiKoS0 KiKoS0 self-assigned this Aug 31, 2024
Copy link

linear bot commented Aug 31, 2024

@KiKoS0 KiKoS0 force-pushed the riadh/inn-3324-add-throttling-config branch 5 times, most recently from b0c450e to dd75b01 Compare September 1, 2024 00:46
Copy link
Collaborator

@albertchae albertchae 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, main question is about calling it throttle vs rateLimit

fun throttle(
limit: Int,
period: Duration,
burst: Int? = null,
Copy link
Collaborator

Choose a reason for hiding this comment

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

would this default value of null overwrite the default of 1 in the data class? I guess it doesn't technically matter if it doesn't get serialized to the Inngest server and if they default to 1 there

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I should have mentioned it in the description but rateLimit and throttle are different configs, the former being lossy as in if the limit is hit function runs are skipped: https://github.com/inngest/inngest-js/blob/809b4efec259a608ce77a004d98fbc2f36d2bc3a/packages/inngest/src/components/InngestFunction.ts#L365-L420

I believe throttle is missing completely from the spec. We do have a ticket for rateLimit as well: https://linear.app/inngest/issue/INN-3332/add-ratelimit-configuration

Copy link
Collaborator Author

@KiKoS0 KiKoS0 Sep 1, 2024

Choose a reason for hiding this comment

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

would this default value of null overwrite the default of 1 in the data class? I guess it doesn't technically matter if it doesn't get serialized to the Inngest server and if they default to 1 there

Good catch, honestly i have no idea so it's better to avoid the confusion and initialize it to null. The JS SDK skips it when it's null so yeah it's up to the server.

Request
POST /fn/register HTTP/1.1
host: 127.0.0.1:8288
connection: keep-alive
Content-Type: application/json
User-Agent: inngest-js:v3.22.5
x-inngest-sdk: inngest-js:v3.22.5
x-inngest-framework: nextjs
x-inngest-expected-server-kind: dev
Server-Timing: handler
Authorization: Bearer undefined
accept: */*
accept-language: *
sec-fetch-mode: cors
accept-encoding: gzip, deflate
content-length: 730

{"url":"http://localhost:3000/api/inngest","deployType":"ping","framework":"nextjs","appName":"new-app","functions":[{"id":"new-app-hello-world","name":"hello-world","triggers":[{"event":"hello-world"}],"steps":{"step":{"id":"step","name":"step","runtime":{"type":"http","url":"http://localhost:3000/api/inngest?fnId=new-app-hello-world&stepId=step"}}},"throttle":{"limit":1,"period":"1s"}},{"id":"new-app-failing-function","name":"failing-function","triggers":[{"event":"fn-fail"}],"steps":{"step":{"id":"step","name":"step","runtime":{"type":"http","url":"http://localhost:3000/api/inngest?fnId=new-app-failing-function&stepId=step"},"retries":{"attempts":0}}}}],"sdk":"js:v3.22.5","v":"0.1","capabilities":{"trust_probe":"v1"}}
Response
HTTP/1.1 200 OK
Content-Type: application/json; charset=utf-8
Vary: Origin
X-Inngest-Server-Kind: dev
Date: Sun, 01 Sep 2024 15:20:05 GMT
Content-Length: 1273

{
  "version": "0.29.6-2e911abc6",
  "authed": false,
  "startOpts": {
    "dir": "",
    "urls": [
      "http://127.0.0.1:8080/api/inngest"
    ],
    "autodiscover": true,
    "poll": true,
    "poll_interval": 5,
    "tick": 150000000,
    "retry_interval": 0
  },
  "functions": [
    {
      "id": "6e55e21b-b344-56bd-8fdf-233a81768e3a",
      "fv": 0,
      "name": "hello-world",
      "slug": "new-app-hello-world",
      "triggers": [
        {
          "event": "hello-world"
        }
      ],
      "throttle": {
        "burst": 1,
        "limit": 1,
        "period": "1s"
      },
      "steps": [
        {
          "id": "step",
          "name": "step",
          "uri": "http://localhost:3000/api/inngest?fnId=new-app-hello-world\u0026stepId=step"
        }
      ]
    },
    {
      "id": "9001ee96-8157-5588-b51e-381438392f9e",
      "fv": 0,
      "name": "failing-function",
      "slug": "new-app-failing-function",
      "triggers": [
        {
          "event": "fn-fail"
        }
      ],
      "steps": [
        {
          "id": "step",
          "name": "step",
          "uri": "http://localhost:3000/api/inngest?fnId=new-app-failing-function\u0026stepId=step",
          "retries": 0
        }
      ]
    }
  ],
  "handlers": null
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

cool thanks for checking! totally missed we have another ticket for rateLimit

.id("ThrottledFunction")
.name("Throttled Function")
.triggerEvent("test/throttled")
.throttle(1, Duration.ofSeconds(10), 1, "throttled");
Copy link
Collaborator

Choose a reason for hiding this comment

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

hm seeing it called here makes me wonder if we should use named arguments somehow since having 4 arguments especially with 2 of them as Ints could be confusing, but it looks like batchEvents has 3 arguments and this is following the same pattern, so fine to leave as is.

I guess we would get named arguments for free anyway in Kotlin but in Java would have to expose the data class or some kind of builder to achieve it.


Thread.sleep(5000);

// Without throttling, both events would have been completed by now
Copy link
Collaborator

Choose a reason for hiding this comment

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

definitely could see this being flaky, so let's keep an eye on it

@KiKoS0 KiKoS0 force-pushed the riadh/inn-3324-add-throttling-config branch from d679f63 to 8bb8d5a Compare September 1, 2024 15:37
A test is included, but it may be challenging to verify if the function
is throttled since it returns a status of Running. If the test fails due
to timing issues, it might not be worth keeping.
@KiKoS0 KiKoS0 force-pushed the riadh/inn-3324-add-throttling-config branch from 8bb8d5a to a583176 Compare September 1, 2024 15:55
@KiKoS0 KiKoS0 merged commit ba822f3 into main Sep 1, 2024
9 checks passed
@KiKoS0 KiKoS0 deleted the riadh/inn-3324-add-throttling-config branch September 1, 2024 17:35
albertchae added a commit that referenced this pull request Sep 11, 2024
Very similar to throttle implementation in #66
@albertchae albertchae mentioned this pull request Sep 11, 2024
2 tasks
albertchae added a commit that referenced this pull request Sep 11, 2024
Very similar to throttle implementation in #66
albertchae added a commit that referenced this pull request Sep 12, 2024
Very similar to throttle implementation in #66
albertchae added a commit that referenced this pull request Sep 13, 2024
Very similar to throttle implementation in #66

The test timings need to account for GCRA buckets https://www.inngest.com/docs/guides/rate-limiting#how-rate-limiting-works
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.

2 participants