Enable client-side rate limiting on source-stripe #31512 #32284

keu · 2023-11-08T00:32:54Z

What

This adds client-side rate limiting that helps increase the number of workers and not hit the call rate limit when it is too big.

How

Describe the solution

🚨 User Impact 🚨

Are there any breaking changes? What is the end result perceived by the user?

For connector PRs, use this section to explain which type of semantic versioning bump occurs as a result of the changes. Refer to our Semantic Versioning for Connectors guidelines for more information. Breaking changes to connectors must be documented by an Airbyte engineer (PR author, or reviewer for community PRs) by using the Breaking Change Release Playbook.

If there are breaking changes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.

Pre-merge Actions

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

Community member? Grant edit access to maintainers (instructions)
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Connector version is set to 0.0.1
- Dockerfile has version 0.0.1
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- docs/integrations/<source or destination>/<name>.md including changelog with an entry for the initial version. See changelog example
- docs/integrations/README.md

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Unit & integration tests added

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.

Connector Generator

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:generateScaffolds then checking in your changes
Documentation which references the generator is updated as needed

Updating the Python CDK

Airbyter

Before merging:

Pull Request description explains what problem it is solving
Code change is unit tested
Build and my-py check pass
Smoke test the change on at least one affected connector
- On Github: Run this workflow, passing --use-local-cdk --name=source-<connector> as options
- Locally: airbyte-ci connectors --use-local-cdk --name=source-<connector> test
PR is reviewed and approved

After merging:

Publish the CDK
- The CDK does not follow proper semantic versioning. Choose minor if this the change has significant user impact or is a breaking change. Choose patch otherwise.
- Write a thoughtful changelog message so we know what was updated.
Merge the platform PR that was auto-created for updating the Connector Builder's CDK version
- This step is optional if the change does not affect the connector builder or declarative connectors.

vercel · 2023-11-08T00:33:00Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 16, 2023 1:50am

CLAassistant · 2023-11-08T00:33:00Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ keu
❌ eugene-kulak
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

github-actions · 2023-11-08T00:33:16Z

Before Merging a Connector Pull Request

Wow! What a great pull request you have here! 🎉

To merge this PR, ensure the following has been done/considered for each connector added or updated:

PR name follows PR naming conventions
Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan.
Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
You've updated the connector's metadata.yaml file any other relevant changes, including a breakingChanges entry for major version bumps. See metadata.yaml docs
Secrets in the connector's spec are annotated with airbyte_secret
All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
Migration guide updated in docs/integrations/<source or destination>/<name>-migrations.md with an entry for the new version, if the version is a breaking change. See migration guide example
If set, you've ensured the icon is present in the platform-internal repo. (Docs)

If the checklist is complete, but the CI check is failing,

Check for hidden checklists in your PR description
Toggle the github label checklist-action-run on/off to re-run the checklist CI.

…all_rate

maxi297

Can we add some test to ensure that we get the behavior we want?

airbyte-integrations/connectors/source-stripe/source_stripe/source.py

keu · 2023-11-08T21:29:26Z

blocked by this https://github.com/airbytehq/airbyte/pull/31696/files?diff=unified&w=0
I'm waiting for it to update this PR with logic for concurrent streams.

…all_rate

it showed better performance and almost never hit the API limit in local testing

maxi297

Thanks for adding the tests! We just need to do the items from the connector checklist and we should be good to go. There is this blocked situation with the other PR that I don't fully get though so if you can expand on that just to be sure we're aligned

girarda · 2023-11-14T17:15:50Z

airbyte-integrations/connectors/source-stripe/source_stripe/spec.yaml

@@ -76,3 +76,9 @@ connectionSpecification:
        The number of API calls per second that you allow connector to make. This value can not be bigger than real
        API call rate limit (https://stripe.com/docs/rate-limits). If not specified the default maximum is 25 and 100
        calls per second for test and production tokens respectively.
+    call_rate_policy:


is this field only for testing purposes? This looks like an implementation detail to me. a simpler option for the user would be to configure the rate limit in terms of requests per seconds

I have added this just for your convenience to test with different policies :)

keu · 2023-11-14T17:25:26Z

Thanks for adding the tests! We just need to do the items from the connector checklist and we should be good to go. There is this blocked situation with the other PR that I don't fully get though so if you can expand on that just to be sure we're aligned

Got it, I want to run a few more tests to make sure which policy to enable by default. Also @girarda will help me to run tests with live account to make sure we don't miss anything. And I'm done.

…all_rate_concurrent

girarda

I ran a test locally with a live account and 15 workers and the moving window strategy.

The acquire_call CPU usage was ~2% which isn't outrageous to me

girarda · 2023-11-14T18:56:22Z

airbyte-integrations/connectors/source-stripe/setup.py

@@ -5,7 +5,7 @@

 from setuptools import find_packages, setup

-MAIN_REQUIREMENTS = ["airbyte-cdk==0.52.8", "stripe==2.56.0", "pendulum==2.1.2"]
+MAIN_REQUIREMENTS = ["airbyte-cdk==0.53.2", "stripe==2.56.0", "pendulum==2.1.2"]


note: we'll need to bump this to the latest version with the higher retry count

girarda · 2023-11-14T18:57:02Z

airbyte-integrations/connectors/source-stripe/source_stripe/source.py

+                ),
+            ]
+        else:
+            policies = []


the default should use a policy

girarda · 2023-11-14T20:15:14Z

airbyte-integrations/connectors/source-stripe/source_stripe/source.py

-_MAX_CONCURRENCY = 3
+logger = logging.getLogger("airbyte")
+
+_MAX_CONCURRENCY = 20


the limit in the spec should be aligned with this limit

…all_rate

girarda · 2023-11-15T15:38:21Z

airbyte-integrations/connectors/source-stripe/source_stripe/spec.yaml

@@ -62,9 +62,17 @@ connectionSpecification:
      title: Number of concurrent workers
      minimum: 1
      maximum: 3


maximum should be 20

girarda · 2023-11-15T15:38:33Z

airbyte-integrations/connectors/source-stripe/source_stripe/spec.yaml

@@ -62,9 +62,17 @@ connectionSpecification:
      title: Number of concurrent workers
      minimum: 1
      maximum: 3
-      default: 2
+      default: 20


let's set the default to 10 and go from there

girarda · 2023-11-15T15:39:08Z

airbyte-integrations/connectors/source-stripe/source_stripe/spec.yaml

@@ -62,9 +62,17 @@ connectionSpecification:
      title: Number of concurrent workers
      minimum: 1
      maximum: 3
-      default: 2
+      default: 20
      examples: [1, 2, 3]
      description: >-
        The number of worker thread to use for the sync. The bigger the value is, the faster the sync will be.


Suggested change

The number of worker thread to use for the sync. The bigger the value is, the faster the sync will be.

The number of worker thread to use for the sync.

this statement isn't true

girarda · 2023-11-15T15:41:40Z

airbyte-integrations/connectors/source-stripe/source_stripe/source.py

+        else:
+            call_limit = max_call_rate
+
+        if config.get("call_rate_policy") == "fixed_window":


call_rate_policy isn't in the pec so this will never be true

I think something went wrong with merge, so this was an old code, updated

…all_rate

…o keu/source-stripe/call_rate

girarda

!

girarda · 2023-11-16T01:14:03Z

docs/integrations/sources/stripe.md

@@ -216,6 +216,7 @@ Each record is marked with `is_deleted` flag when the appropriate event happens

 | Version | Date       | Pull Request                                              | Subject                                                                                                                                              |
 |:--------|:-----------|:----------------------------------------------------------|:-----------------------------------------------------------------------------------------------------------------------------------------------------|
+| 4.5.4   | 2023-11-15 | [32284](https://github.com/airbytehq/airbyte/pull/32284/) | Enable client-side rate limiting                                                                                                                     |


nit: let's update the date before merging

keu · 2023-11-16T01:51:39Z

@maxi297 😇

maxi297

Check list has been updated. I'm good with the implementation

add call_rate_limit parameter

cbe519b

octavia-squidington-iii added area/connectors Connector related issues connectors/source/stripe labels Nov 8, 2023

octavia-squidington-iv requested a review from a team November 8, 2023 00:34

customize call rate limit for files API

8be4462

keu linked an issue Nov 8, 2023 that may be closed by this pull request

Enable client-side rate limiting on source-stripe #31512

Closed

4 tasks

keu requested review from yevhenii-ldv and removed request for a team November 8, 2023 00:50

Merge remote-tracking branch 'origin/master' into keu/source-stripe/c…

5b95449

…all_rate

vercel bot deployed to Preview November 8, 2023 01:08 View deployment

octavia-squidington-iv requested a review from a team November 8, 2023 01:08

keu self-assigned this Nov 8, 2023

add file_links call rate checks

c39d5ef

girarda requested a review from maxi297 November 8, 2023 19:31

maxi297 requested changes Nov 8, 2023

View reviewed changes

airbyte-integrations/connectors/source-stripe/source_stripe/source.py Outdated Show resolved Hide resolved

airbyte-integrations/connectors/source-stripe/source_stripe/source.py Show resolved Hide resolved

eugene-kulak added 2 commits November 14, 2023 02:48

unit tests

c503ed9

Merge remote-tracking branch 'origin/master' into keu/source-stripe/c…

6f0608e

…all_rate

keu force-pushed the keu/source-stripe/call_rate branch from 7d50264 to 6f0608e Compare November 14, 2023 00:52

keu requested a review from maxi297 November 14, 2023 00:53

vercel bot deployed to Preview November 14, 2023 00:56 View deployment

change call rate policy to use MovingWindow

4dfafc4

it showed better performance and almost never hit the API limit in local testing

maxi297 reviewed Nov 14, 2023

View reviewed changes

add call_ratee_policy parameter (for testing)

830c89d

girarda reviewed Nov 14, 2023

View reviewed changes

increase maximum_attempts_to_acquire to never hit it

aa3d3ed

Merge remote-tracking branch 'origin/master' into keu/source-stripe/c…

2bba7f7

…all_rate_concurrent

vercel bot deployed to Preview November 14, 2023 17:35 View deployment

girarda reviewed Nov 14, 2023

View reviewed changes

eugene-kulak added 4 commits November 14, 2023 22:32

Merge remote-tracking branch 'origin/master' into keu/source-stripe/c…

404e41e

…all_rate

update CDK version

f85b6d7

update version and changelog

48504b1

Merge remote-tracking branch 'origin/master' into keu/source-stripe/c…

b22c7be

…all_rate

octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Nov 15, 2023

vercel bot deployed to Preview November 15, 2023 14:31 View deployment

girarda requested a review from maxi297 November 15, 2023 15:33

girarda reviewed Nov 15, 2023

View reviewed changes

eugene-kulak added 3 commits November 16, 2023 00:42

fix unit tests after merge

efb11dd

remove fixed window policy

7a440f4

Merge remote-tracking branch 'origin/master' into keu/source-stripe/c…

cafa299

…all_rate

vercel bot deployed to Preview November 15, 2023 22:48 View deployment

eugene-kulak and others added 4 commits November 16, 2023 00:49

fix default value for num_workers

e0db465

Automated Commit - Formatting Changes

109d6ab

fix default number of workers

5ad5879

Merge remote-tracking branch 'origin/keu/source-stripe/call_rate' int…

5836114

…o keu/source-stripe/call_rate

girarda approved these changes Nov 16, 2023

View reviewed changes

stripe: update date in changelog

98ee96f

vercel bot deployed to Preview November 16, 2023 01:50 View deployment

maxi297 approved these changes Nov 16, 2023

View reviewed changes

keu merged commit a38eca1 into master Nov 16, 2023
24 of 26 checks passed

keu deleted the keu/source-stripe/call_rate branch November 16, 2023 15:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable client-side rate limiting on source-stripe #31512 #32284

Enable client-side rate limiting on source-stripe #31512 #32284

keu commented Nov 8, 2023 •

edited

Loading

vercel bot commented Nov 8, 2023 •

edited

Loading

CLAassistant commented Nov 8, 2023 •

edited

Loading

github-actions bot commented Nov 8, 2023 •

edited by keu

Loading

maxi297 left a comment

keu commented Nov 8, 2023

maxi297 left a comment

girarda Nov 14, 2023

keu Nov 14, 2023

keu commented Nov 14, 2023

girarda left a comment •

edited

Loading

girarda Nov 14, 2023

girarda Nov 14, 2023

girarda Nov 14, 2023

girarda Nov 15, 2023

girarda Nov 15, 2023

girarda Nov 15, 2023

girarda Nov 15, 2023

girarda Nov 15, 2023

keu Nov 15, 2023

girarda left a comment

girarda Nov 16, 2023

keu Nov 16, 2023

keu commented Nov 16, 2023 •

edited

Loading

maxi297 left a comment

	The number of worker thread to use for the sync. The bigger the value is, the faster the sync will be.
	The number of worker thread to use for the sync.

Enable client-side rate limiting on source-stripe #31512 #32284

Enable client-side rate limiting on source-stripe #31512 #32284

Conversation

keu commented Nov 8, 2023 • edited Loading

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Actions

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Airbyter

vercel bot commented Nov 8, 2023 • edited Loading

CLAassistant commented Nov 8, 2023 • edited Loading

github-actions bot commented Nov 8, 2023 • edited by keu Loading

Before Merging a Connector Pull Request

maxi297 left a comment

Choose a reason for hiding this comment

keu commented Nov 8, 2023

maxi297 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keu commented Nov 14, 2023

girarda left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

girarda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keu commented Nov 16, 2023 • edited Loading

maxi297 left a comment

Choose a reason for hiding this comment

keu commented Nov 8, 2023 •

edited

Loading

vercel bot commented Nov 8, 2023 •

edited

Loading

CLAassistant commented Nov 8, 2023 •

edited

Loading

github-actions bot commented Nov 8, 2023 •

edited by keu

Loading

girarda left a comment •

edited

Loading

keu commented Nov 16, 2023 •

edited

Loading