-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Enable client-side rate limiting on source-stripe #31512 #32284
Changes from 10 commits
cbe519b
8be4462
5b95449
c39d5ef
c503ed9
6f0608e
4dfafc4
830c89d
aa3d3ed
2bba7f7
404e41e
f85b6d7
48504b1
b22c7be
efb11dd
7a440f4
cafa299
e0db465
109d6ab
5ad5879
5836114
98ee96f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,9 @@ | |
# Copyright (c) 2023 Airbyte, Inc., all rights reserved. | ||
# | ||
|
||
import logging | ||
import os | ||
from datetime import datetime, timedelta | ||
from typing import Any, List, Mapping, MutableMapping, Optional, Tuple | ||
|
||
import pendulum | ||
|
@@ -13,6 +15,14 @@ | |
from airbyte_cdk.sources import AbstractSource | ||
from airbyte_cdk.sources.message.repository import InMemoryMessageRepository | ||
from airbyte_cdk.sources.streams import Stream | ||
from airbyte_cdk.sources.streams.call_rate import ( | ||
AbstractAPIBudget, | ||
FixedWindowCallRatePolicy, | ||
HttpAPIBudget, | ||
HttpRequestMatcher, | ||
MovingWindowCallRatePolicy, | ||
Rate, | ||
) | ||
from airbyte_cdk.sources.streams.concurrent.adapters import StreamFacade | ||
from airbyte_cdk.sources.streams.concurrent.cursor import NoopCursor | ||
from airbyte_cdk.sources.streams.http.auth import TokenAuthenticator | ||
|
@@ -33,9 +43,12 @@ | |
UpdatedCursorIncrementalStripeStream, | ||
) | ||
|
||
_MAX_CONCURRENCY = 3 | ||
logger = logging.getLogger("airbyte") | ||
|
||
_MAX_CONCURRENCY = 20 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the limit in the spec should be aligned with this limit |
||
_CACHE_DISABLED = os.environ.get("CACHE_DISABLED") | ||
USE_CACHE = not _CACHE_DISABLED | ||
STRIPE_TEST_ACCOUNT_PREFIX = "sk_test_" | ||
|
||
|
||
class SourceStripe(AbstractSource): | ||
|
@@ -114,6 +127,73 @@ def customers(**args): | |
**args, | ||
) | ||
|
||
@staticmethod | ||
def is_test_account(config: Mapping[str, Any]) -> bool: | ||
"""Check if configuration uses Stripe test account (https://stripe.com/docs/keys#obtain-api-keys) | ||
|
||
:param config: | ||
:return: True if configured to use a test account, False - otherwise | ||
""" | ||
|
||
return str(config["client_secret"]).startswith(STRIPE_TEST_ACCOUNT_PREFIX) | ||
|
||
def get_api_call_budget(self, config: Mapping[str, Any]) -> AbstractAPIBudget: | ||
"""Get API call budget which connector is allowed to use. | ||
|
||
:param config: | ||
:return: | ||
""" | ||
|
||
max_call_rate = 25 if self.is_test_account(config) else 100 | ||
if config.get("call_rate_limit"): | ||
call_limit = config["call_rate_limit"] | ||
if call_limit > max_call_rate: | ||
keu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
logger.warning( | ||
"call_rate_limit is larger than maximum allowed %s, fallback to default %s.", | ||
max_call_rate, | ||
max_call_rate, | ||
) | ||
call_limit = max_call_rate | ||
else: | ||
call_limit = max_call_rate | ||
|
||
if config.get("call_rate_policy") == "fixed_window": | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. call_rate_policy isn't in the pec so this will never be true There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think something went wrong with merge, so this was an old code, updated |
||
policies = [ | ||
FixedWindowCallRatePolicy( | ||
next_reset_ts=datetime.now(), | ||
period=timedelta(seconds=1), | ||
call_limit=20, | ||
matchers=[ | ||
HttpRequestMatcher(url="https://api.stripe.com/v1/files"), | ||
HttpRequestMatcher(url="https://api.stripe.com/v1/file_links"), | ||
], | ||
), | ||
FixedWindowCallRatePolicy( | ||
next_reset_ts=datetime.now(), | ||
period=timedelta(seconds=1), | ||
call_limit=call_limit, | ||
matchers=[], | ||
), | ||
] | ||
elif config.get("call_rate_policy") == "moving_window": | ||
policies = [ | ||
MovingWindowCallRatePolicy( | ||
rates=[Rate(limit=20, interval=timedelta(seconds=1))], | ||
matchers=[ | ||
HttpRequestMatcher(url="https://api.stripe.com/v1/files"), | ||
HttpRequestMatcher(url="https://api.stripe.com/v1/file_links"), | ||
], | ||
), | ||
MovingWindowCallRatePolicy( | ||
rates=[Rate(limit=call_limit, interval=timedelta(seconds=1))], | ||
matchers=[], | ||
), | ||
] | ||
else: | ||
policies = [] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the default should use a policy |
||
|
||
return HttpAPIBudget(policies=policies, maximum_attempts_to_acquire=10000) | ||
|
||
def streams(self, config: Mapping[str, Any]) -> List[Stream]: | ||
config = self.validate_and_fill_with_defaults(config) | ||
authenticator = TokenAuthenticator(config["client_secret"]) | ||
|
@@ -122,6 +202,7 @@ def streams(self, config: Mapping[str, Any]) -> List[Stream]: | |
"account_id": config["account_id"], | ||
"start_date": config["start_date"], | ||
"slice_range": config["slice_range"], | ||
"api_budget": self.get_api_call_budget(config), | ||
} | ||
incremental_args = {**args, "lookback_window_days": config["lookback_window_days"]} | ||
subscriptions = IncrementalStripeStream( | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -66,5 +66,19 @@ connectionSpecification: | |||||
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. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this statement isn't true |
||||||
Be careful as rate limiting is not implemented. | ||||||
The performance upper boundary depends on call_rate_limit setting and type of account. | ||||||
order: 5 | ||||||
call_rate_limit: | ||||||
type: integer | ||||||
title: Max number of API calls per second | ||||||
examples: [25, 100] | ||||||
description: >- | ||||||
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: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have added this just for your convenience to test with different policies :) |
||||||
type: string | ||||||
title: Call rate policy to use | ||||||
examples: [fixed_window, moving_window, none] | ||||||
description: >- | ||||||
The call rate algorithm to use for tracking and limiting calls. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: we'll need to bump this to the latest version with the higher retry count