-
Notifications
You must be signed in to change notification settings - Fork 13
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
feat: Add charge method to the run client for "pay per event" #304
Conversation
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.
The link to the docs is there, so I believe we can close #305, correct?
Co-authored-by: Vlada Dusek <[email protected]>
Co-authored-by: Vlada Dusek <[email protected]>
@vdusek the URL is a best-effort guess of what it will be when it's added to docs based on apify/apify-client-js#613 (comment) When the docs are created, I added a request to check of these links. Let's leave those issues open until then so we don't forget https://github.com/apify/apify-core/issues/18591#issuecomment-2516852353 |
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.
LGTM
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.
One note about the idempotency key
url=self._url('charge'), | ||
method='POST', | ||
headers={ | ||
'idempotency-key': idempotency_key or f'{self.resource_id}-{event_name}-{int(time.time() * 1000)}', |
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.
What if someone tries to charge for two occurences of one event twice in the same millisecond? Then the idempotency key will be the same and one charge will get ignored. Some unique key is needed here, I think a random string would be enough.
There's a crypto_random_object_id
implementation in Crawlee for Python, perhaps we could move it to apify-shared-python
and use it here.
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.
I took the idempotency key generation from https://github.com/apify-store/contact-info/blob/master/code/src/charging_manager.ts but it could be specific to their actors 🤔 I can move crypto_random_object_id
👍
Resolves https://github.com/apify/apify-core/issues/18593 by adding the charge endpoint to the run client.
Issue to add docs URL #305