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

Bug: CheckTrialOrIntroDiscountEligibility too slow #1893

Closed
1 task done
jesus-mg-ios opened this issue Sep 8, 2022 · 21 comments
Closed
1 task done

Bug: CheckTrialOrIntroDiscountEligibility too slow #1893

jesus-mg-ios opened this issue Sep 8, 2022 · 21 comments
Labels
bug status: waiting-for-reply Issues that are waiting for a reply

Comments

@jesus-mg-ios
Copy link

Describe the bug
Add any other context about the problem here.

We are using checkTrialOrIntroDiscountEligibility in iOS with swift  after get the product, and we notice that this call is too slow (even 45s - 1 min). 

Purchases.shared.checkTrialOrIntroDiscountEligibility(productIdentifiers: ["productIdentifier"]) { result in
....
}
  1. Environment
    1. Platform: iOS
    2. SDK version: 4.10
    3. StoreKit 2 (enabled with useStoreKit2IfEnabled) (Y/N): N
    4. OS version: 15.5
    5. Xcode version: 13.4.1
    6. How widespread is the issue. Percentage of devices affected: Unknown/Random
      Additional context

Environment: Real device & simulator. iOS 15.5
SDK Version: "4.10.0"
Code that is too slow

Purchases.shared.checkTrialOrIntroDiscountEligibility(productIdentifiers: ["productIdentifier"]) { result in
....
}
@RCGitBot
Copy link
Contributor

RCGitBot commented Sep 8, 2022

👀 SDKONCALL-120 We've just linked this issue to our internal tracker and notified the team. Thank you for reporting, we're checking this out!

@aboedo
Copy link
Member

aboedo commented Sep 8, 2022

Thanks for reporting!
I get the feeling that the problem is actually coming from StoreKit 2.

Currently we use StoreKit 2 for this particular API regardless of whether usesStoreKit2IfAvailable is true, because with StoreKit 1, intro eligibility calculation is sadly a "best guess" approach, since it's not technically possible to get an accurate value for all edge cases with StoreKit 1.

Could you share the debug logs for your device?
We might try allowing you to disable SK2 for this particular feature, would you be open to trying that out using a branch from our SDK?

@aboedo
Copy link
Member

aboedo commented Sep 8, 2022

@jesus-mg-ios would you mind trying out the branch intro-eligibility-checker-sk1 from this PR to see if you get better results? #1894

It just disables SK2 for this particular operation.

Let me know if you need any instructions for using the SDK from that branch

@jesus-mg-ios
Copy link
Author

Thanks, I'm gonna try it!

@stale
Copy link

stale bot commented Sep 15, 2022

This issue has been automatically marked as stale due to inactivity. It will be closed if no further activity occurs. Please reach out if you have additional information to help us investigate further!

@stale stale bot added the status: waiting-for-reply Issues that are waiting for a reply label Sep 15, 2022
@jesus-mg-ios
Copy link
Author

Hi @aboedo, the change was amazingly faster.

We're gonna share with you the logs before and
after the change.

RevenueCatLogsBeforeFixBranch_4_11_0 copy.txt

RevenueCatLogsAfterFixBranch_4_12_0 copy.txt

@stale stale bot removed the status: waiting-for-reply Issues that are waiting for a reply label Sep 20, 2022
@aboedo
Copy link
Member

aboedo commented Sep 20, 2022

@jesus-mg-ios thanks for getting back to us.

This confirms that the issue is StoreKit 2's underlying API being very slow for this...

Which is frankly unfortunate because it's the only one that's technically guaranteed to be perfectly accurate (with StoreKit 1, 100% accuracy for intro eligibility is technically not possible 🤕). It will still be correct for the vast majority of use cases, though.

We're going to think about the best way to provide flexibility for this API.

cc @NachoSoto

@stale
Copy link

stale bot commented Sep 27, 2022

This issue has been automatically marked as stale due to inactivity. It will be closed if no further activity occurs. Please reach out if you have additional information to help us investigate further!

@stale stale bot added the status: waiting-for-reply Issues that are waiting for a reply label Sep 27, 2022
@aboedo aboedo removed the status: waiting-for-reply Issues that are waiting for a reply label Sep 29, 2022
@jesus-mg-ios
Copy link
Author

jesus-mg-ios commented Oct 5, 2022

@aboedo we're experimenting delays also in the simulator (with no appleid configured and not Apple ID signed in).
Is it expected?

cc: @unxavi

@timfraedrich
Copy link

We are also experiencing this issue, might it be possible to request and cache the eligibility properties together with the offerings on app start? From my understanding the values should not change except if a product is purchased at which point RevenueCat should know about it and could invalidate the cache. I may be missing something here though.

@tcwalther
Copy link

@timfraedrich I just came here because of the same issue - being able to cache offerings and eligibility would be great. Afaics this would require making RevenueCat.Package conform to Codable.

@NachoSoto
Copy link
Contributor

Afaics this would require making RevenueCat.Package conform to Codable.

Caching eligibility totally makes sense. But this approach wouldn't be trivial / possible. Package has StoreProducts, which have underlying StoreKit products we can't fully serialize.

@jesus-mg-ios
Copy link
Author

jesus-mg-ios commented Oct 14, 2022

And it is possible to use a configuration parameter to disable (opt), the case that @aboedo reverted? I think that solve our problem for now. @NachoSoto . At this moment we have not require a 100% of accuracy in this.

@tcwalther
Copy link

@NachoSoto afaics one can encode at least an SK2Product using jsonRepresentation, but I can't see how to decode it again.

I think in general though, being able to cache as much as possible of the pricing would be highly desirable. In our app, for example, we just added a lock screen widget that on the free version just sends a user to a paywall. That could very well happen from a cold start, and it would be great if we could display prices instantly.

@aboedo
Copy link
Member

aboedo commented Oct 14, 2022

Hey folks, thanks for the feedback!
We're discussing best approaches here. Caching is possible, although like all caching things it's not trivial and we'd want to get it right. So we'll add it, but I don't have an ETA for that yet and it might take us a bit longer to get to it.

In the meantime, we're also discussing the possibility of reusing the useSK2IfAvailable flag that's available when configuring the SDK to control the implementation of this method too. That would provide a low-risk solution that's easy to add on our side. We'll keep you posted!

NachoSoto added a commit that referenced this issue Oct 14, 2022
… enabled

This used to always default to the SK2 implementation if it was available because it was much more reliable.
However, as we've learned (#1893), it can be significantly slow.

Until we implement caching for it ([CSDK-493]), this for now allows users to disable it if it's too slow.

For users that don't specify `useSk2IfAvailable`, SK2 is now the new default, so this class will continue to use the "better" implementation for them too.
NachoSoto added a commit that referenced this issue Oct 14, 2022
… enabled (#1984)

This is similar to #1894, but as a permanent change.

This used to always default to the SK2 implementation if it was
available because it was much more reliable.
However, as we've learned (#1893), it can be significantly slow.

Until we implement caching for it ([CSDK-493]), this for now allows
users to disable it if it's too slow.

For users that don't specify `useSk2IfAvailable`, SK2 is now the new
default, so this class will continue to use the "better" implementation
for them too.

[CSDK-493]:
https://revenuecats.atlassian.net/browse/CSDK-493?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
@stale
Copy link

stale bot commented Oct 21, 2022

This issue has been automatically marked as stale due to inactivity. It will be closed if no further activity occurs. Please reach out if you have additional information to help us investigate further!

@stale stale bot added the status: waiting-for-reply Issues that are waiting for a reply label Oct 21, 2022
@aboedo
Copy link
Member

aboedo commented Oct 21, 2022

Update here: we've updated checkTrialOrIntroDiscountEligibility so that it uses our StoreKit 1 implementation if useSK2IfEnabled = false.
This will go out very soon in release 4.13.3, PR is here #1988

@stale stale bot removed the status: waiting-for-reply Issues that are waiting for a reply label Oct 21, 2022
@NachoSoto
Copy link
Contributor

NachoSoto commented Oct 24, 2022

I also filed CSDK-493 to add a caching mechanism to TrialOrIntroPriceEligibilityChecker.

@stale
Copy link

stale bot commented Nov 1, 2022

This issue has been automatically marked as stale due to inactivity. It will be closed if no further activity occurs. Please reach out if you have additional information to help us investigate further!

@stale stale bot added the status: waiting-for-reply Issues that are waiting for a reply label Nov 1, 2022
NachoSoto added a commit that referenced this issue Nov 4, 2022
…ons`

This changes the default back to `StoreKit 1`. We decided to do this for the following reasons:
- Purchasing with `PromotionalOffer`s does not work with StoreKit 2 due to an Apple bug (see #2020 (comment))
- `checkTrialOrIntroDiscountEligibility` is significantly slower with StoreKit 2 (#1893). We're adding optimizations to help with that (#2007), but the underlying logic will still be slow.
- A rare race-condition where `StoreKit 2` does not have transactions after a purchase ([TRIAGE-82]). We have some workarounds (#1945), but it's still being investigated.

_ Note: This effectively reverts 0ee540a. That commit made it easier to only change the default in one place which is why this PR is basically just one line._
NachoSoto added a commit that referenced this issue Nov 4, 2022
…ons` (#2022)

This changes the default back to `StoreKit 1`. We decided to do this for
the following reasons:
- Purchasing with `PromotionalOffer`s does not work with StoreKit 2 due
to an Apple bug (see
#2020 (comment))
- `checkTrialOrIntroDiscountEligibility` is significantly slower with
`StoreKit 2` (#1893). We're adding optimizations to help with that
(#2007), but the underlying logic will still be slow.
- A rare race-condition where `StoreKit 2` does not have transactions
after a purchase ([TRIAGE-82]). We have some workarounds (#1945), but
it's still being investigated.

_Note: This effectively reverts 0ee540a. That commit made it easier to
only change the default in one place which is why this PR is basically
just one line._

[TRIAGE-82]:
https://revenuecats.atlassian.net/browse/TRIAGE-82?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
@stale stale bot closed this as completed Nov 9, 2022
@aboedo
Copy link
Member

aboedo commented Nov 9, 2022

Final update here: the implementation of checkTrialOrIntroDiscountEligibility will now respect the preference for useStoreKit2IfAvailable.
Our latest release uses StoreKit 1 by default, which is the speedy implementation. So this should no longer be a problem by default, and if you're setting useStoreKit2IfAvailable to true, changing the value to false should solve it.

@github-actions
Copy link

This issue has been automatically locked due to no recent activity after it was closed. Please open a new issue for related reports.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug status: waiting-for-reply Issues that are waiting for a reply
Projects
None yet
Development

No branches or pull requests

6 participants