Skip to content

Conversation

janbuchar
Copy link
Contributor

@github-actions github-actions bot added this to the 125th sprint - Tooling team milestone Oct 7, 2025
@github-actions github-actions bot added the t-tooling Issues with this label are in the ownership of the tooling team. label Oct 7, 2025
@B4nan
Copy link
Member

B4nan commented Oct 7, 2025

Let's call this perf optimization, that's the main point of why we do it, right?

@B4nan B4nan changed the title refactor: Use Apify-provided environment variables to obtain PPE pricing information perf: Use Apify-provided environment variables to obtain PPE pricing information Oct 7, 2025
@janbuchar
Copy link
Contributor Author

E2E test is failing - I probably broke the charge limit logic

Copy link
Member

@metalwarrior665 metalwarrior665 left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot added the tested Temporary label used only programatically for some analytics. label Oct 8, 2025
@janbuchar
Copy link
Contributor Author

https://github.com/apify/apify-sdk-js/actions/runs/18348147670 e2e test run - should be OK now

@janbuchar janbuchar requested review from B4nan and barjin October 8, 2025 14:46
Copy link
Contributor

@barjin barjin left a comment

Choose a reason for hiding this comment

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

lgtm, thank you! 👍

I have a few ideas about the code quality, but neither is strictly necessary

this.chargingState[eventName] = {
chargeCount,
totalChargedAmount:
chargeCount * (this.pricingInfo[eventName]?.price ?? 0),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you have to call the new load* methods in this specific order, as loadChargedEventCounts reads variables initialized by loadPricingInfo (temporal coupling)

title: eventPricing.eventTitle,
};
if (
this.chargingState === undefined ||
Copy link
Contributor

@barjin barjin Oct 8, 2025

Choose a reason for hiding this comment

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

how about removing the load* method calls from the constructor, running them only once in the init method (at one call site) and conditionally loading the config with something like

async function getPricingInfo() {
   if (
        configuration.get('actorPricingInfo') &&
        configuration.get('chargedEventCounts')
    ) return { pricingInfo: JSON.parse(), ...

    const run = await this.apifyClient.run(this.actorRunId).get();
    ...
}

This would allow us to load the pricing info + events at one point in init like

...
const { pricingInfo ... } = await getPricingInfo();
this.loadPricingInfo( pricingInfo );
...

which is IMO cleaner than the current approach, which is conditional based on the current object state

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Load Actor pricing and charged events from env vars
4 participants