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

feat: Add charge method to the run client for "pay per event" #304

Merged
merged 12 commits into from
Dec 5, 2024

Conversation

Jkuzz
Copy link
Contributor

@Jkuzz Jkuzz commented Dec 3, 2024

Resolves https://github.com/apify/apify-core/issues/18593 by adding the charge endpoint to the run client.

Issue to add docs URL #305

@github-actions github-actions bot added the t-c&c Team covering store and finance matters. label Dec 3, 2024
@Jkuzz Jkuzz changed the title Feat(client): Add PPE charge to python client feat(client): Add PPE charge to python client Dec 3, 2024
@Jkuzz Jkuzz requested review from vdusek and fnesveda December 4, 2024 09:34
@Jkuzz Jkuzz marked this pull request as ready for review December 4, 2024 09:34
Copy link
Contributor

@vdusek vdusek left a 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?

src/apify_client/clients/resource_clients/run.py Outdated Show resolved Hide resolved
src/apify_client/clients/resource_clients/run.py Outdated Show resolved Hide resolved
@Jkuzz
Copy link
Contributor Author

Jkuzz commented Dec 4, 2024

@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

Copy link
Contributor

@vdusek vdusek left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@fnesveda fnesveda left a 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)}',
Copy link
Member

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.

Copy link
Contributor Author

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 👍

@B4nan B4nan changed the title feat(client): Add PPE charge to python client feat: Add charge method to the run client for "pay per event" Dec 5, 2024
@Jkuzz Jkuzz requested a review from fnesveda December 5, 2024 12:15
@Jkuzz Jkuzz merged commit 3bd6bbb into master Dec 5, 2024
28 checks passed
@Jkuzz Jkuzz deleted the feat/ppe/actor-charge-client branch December 5, 2024 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t-c&c Team covering store and finance matters.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants