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" #613

Merged
merged 9 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/18592 by adding the PPE charge endpoint to JS client.

The idempotency key creation was taken from an actor by the store team.
Issue to add docs URL #614

@github-actions github-actions bot added t-c&c Team covering store and finance matters. tested Temporary label used only programatically for some analytics. labels Dec 3, 2024
@B4nan B4nan changed the title Feat(client): Add PPE charge to JS client feat(client): Add PPE charge to JS client Dec 3, 2024
@B4nan B4nan changed the title feat(client): Add PPE charge to JS client feat: Add PPE charge to JS client Dec 3, 2024
@Jkuzz Jkuzz requested review from B4nan and barjin December 4, 2024 09:39
@Jkuzz Jkuzz marked this pull request as ready for review December 4, 2024 09:39
@B4nan B4nan changed the title feat: Add PPE charge to JS client feat: Add charge method to the run client for "pay per result" Dec 4, 2024
@Jkuzz Jkuzz changed the title feat: Add charge method to the run client for "pay per result" feat: Add charge method to the run client for "pay per event" Dec 4, 2024
Copy link
Member

@B4nan B4nan left a comment

Choose a reason for hiding this comment

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

you can ignore the failing docs build, its not detecting the links inside parens correctly

Comment on lines 145 to 146
* TODO: docs url
* https://github.com/apify/apify-client-js/issues/614
Copy link
Member

Choose a reason for hiding this comment

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

can we add the URL here right ahead? even if it doesn't exist yet, feels better than publishing with a TODO

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 don't have the URL. I'll ask Mat'o what's the status on the endpoint docs 👍

Copy link
Member

Choose a reason for hiding this comment

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

I am rather sure we can guess it before it exists, its just another endpoint, the URLs for them are not random.

/**
* https://docs.apify.com/api/v2#/reference/actor-runs/charge-run/charge-run
*/
async charge(options: RunChargeOptions): Promise<ApifyResponse<Record<string, never>>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity - what's the expected return type of this method? I see that most of the RunClient methods return ActorRun object (which reflects the changes made by the call).

Is this return type (ApifyResponse<Record<string, never>>) a temporary thing before we figure out the actual API response shape, or is this the final thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We discussed this in person. The endpoint uses a memorized version of the run from the database for performance reasons, as the run's pricing model cannot change during the run. Therefore we cannot return the modified run object and the only important output is the response status code.

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 problem copied over from the Python client review

}));

const count = options.count ?? 1;
const idempotencyKey = options.idempotencyKey ?? `${this.id}-${options.eventName}-${Date.now()}`;
Copy link
Member

Choose a reason for hiding this comment

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

Same note here as in the Python client, the idempotency key has to be a unique string. This way, if someone charged for two occurences of the same event in the same millisecond, only one of the charges would go through.

apify/apify-client-python#304 (review)

@Jkuzz Jkuzz requested a review from fnesveda December 5, 2024 10:51
@Jkuzz Jkuzz merged commit 3d9c64d into master Dec 5, 2024
6 of 7 checks passed
@Jkuzz Jkuzz deleted the feat/ppe/actor-charge-client branch December 5, 2024 15:17
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. tested Temporary label used only programatically for some analytics.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants