-
Notifications
You must be signed in to change notification settings - Fork 316
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
Add missing logs to ProductsFetcherSK2 #1780
Conversation
return cachedProducts | ||
} | ||
|
||
Logger.debug( | ||
Strings.offering.no_cached_requests_and_products_starting_skproduct_request(identifiers: identifiers) |
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.
this one doesn't perfectly translate, since we don't cache requests for sk2....maybe we should?
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.
maybe we should?
It's not trivial with async
, I think that's why we don't do it.
The "no cached requests" part of the message isn't a big deal, but the latter part of the message would be weird here:
starting
SKProducts
request for
Since that's SK1
. So maybe it makes sense to add a new string?
805b8a6
to
31b7005
Compare
…gLevel` is disabled I realized in #1780 that we might be doing unnecessary work creating messages that are never logged. This is a small optimization to prevent that.
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.
Thanks for doing this!
@@ -29,10 +29,17 @@ actor ProductsFetcherSK2 { | |||
func products(identifiers: Set<String>) async throws -> Set<SK2StoreProduct> { | |||
do { | |||
if let cachedProducts = await self.cachedProducts(withIdentifiers: identifiers) { | |||
let cachedIdentifiers = Set(cachedProducts.map { $0.productIdentifier }) | |||
Logger.debug(Strings.offering.products_already_cached(identifiers: cachedIdentifiers)) |
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.
👍🏻
let cachedIdentifiers = Set(cachedProducts.map { $0.productIdentifier }) | ||
Logger.debug(Strings.offering.products_already_cached(identifiers: cachedIdentifiers)) |
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 was thinking that creating this Set
was unnecessary if debug logs are disabled.
I just submitted #1781 to improve this.
To avoid creating this Set
unnecessarily I would inline this:
let cachedIdentifiers = Set(cachedProducts.map { $0.productIdentifier }) | |
Logger.debug(Strings.offering.products_already_cached(identifiers: cachedIdentifiers)) | |
Logger.debug(Strings.offering.products_already_cached(identifiers: | |
Set(cachedProducts.map { $0.productIdentifier }) | |
)) |
return cachedProducts | ||
} | ||
|
||
Logger.debug( | ||
Strings.offering.no_cached_requests_and_products_starting_skproduct_request(identifiers: identifiers) |
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.
maybe we should?
It's not trivial with async
, I think that's why we don't do it.
The "no cached requests" part of the message isn't a big deal, but the latter part of the message would be weird here:
starting
SKProducts
request for
Since that's SK1
. So maybe it makes sense to add a new string?
let storeKitProducts = try await StoreKit.Product.products(for: identifiers) | ||
Logger.rcSuccess(Strings.storeKit.skproductsrequest_received_response) |
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.
Ditto here. Alternatively we could change these strings to be agnostic of the types? So something like "product request received response", etc.
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.
well we do have a completion handler up the call chain in ProductsManager.sk2Products
:
func sk2Products(withIdentifiers identifiers: Set<String>, completion: @escaping (Result<Set<SK2StoreProduct>, Error>) -> Void)
...so we could theoretically hold a map of identifiers -> handlers there like ProductsFetcherSK1
does 🤷
@@ -119,9 +121,11 @@ private extension ProductsManager { | |||
productsFetcherSK1.products(withIdentifiers: removedProductIdentifiers, completion: { result in | |||
switch result { | |||
case.success: | |||
Logger.debug(Strings.storeKit.skproductsrequest_finished) | |||
// this one is duplicate, though other in ProductsFetcherSK1 is rcSuccess |
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.
options i see are:
*. remove duplication -- need to look into hierarchy of logging, when would rcSuccess happen vs debug?
- ensure sk2 has the same duplication
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 don't think we've looked into these logs very much in detail, so this refactor is great 👍🏻
rcSuccess
is just a success LogIntent
, but still LogLevel.debug
. I think that's good for these success cases.
@NachoSoto comments should be addressed, except for a couple open conversations... curious what you think about:
|
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 think this is good, looks like the only 2 things left:
- Don't mention
SKProduct
in SK2 - Consistent debug / success / error calls.
|
||
case .skproductsrequest_timed_out(let afterTimeInSeconds): | ||
return "SKProductsRequest took longer than \(afterTimeInSeconds) seconds, " + | ||
"cancelling request and returning an empty set. This seems to be an App Store quirk. " + | ||
"If this is happening to you consistently, you might want to try using a new Sandbox account. " + | ||
"More information: https://rev.cat/skproductsrequest-hangs" | ||
|
||
case .skproductsrequest_finished: | ||
case .store_product_request_did_finish: | ||
return "SKProductsRequest did finish" |
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.
This can change as well:
return "SKProductsRequest did finish" | |
return "Store product request did finish" |
|
||
case .no_cached_requests_and_products_starting_skproduct_request(let identifiers): | ||
return "No existing requests and " + | ||
"products not cached, starting SKProducts request for: \(identifiers)" |
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.
Ditto, you're still using this in SK2, maybe change it to Store product request
?
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.
was leaving this one in case we decided to cache requests... might be worth a different string until we decide to cache
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.
We cache products, just not concurrent requests, which might be good enough?
@@ -119,9 +121,11 @@ private extension ProductsManager { | |||
productsFetcherSK1.products(withIdentifiers: removedProductIdentifiers, completion: { result in | |||
switch result { | |||
case.success: | |||
Logger.debug(Strings.storeKit.skproductsrequest_finished) | |||
// this one is duplicate, though other in ProductsFetcherSK1 is rcSuccess |
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 don't think we've looked into these logs very much in detail, so this refactor is great 👍🏻
rcSuccess
is just a success LogIntent
, but still LogLevel.debug
. I think that's good for these success cases.
case .failure(let error): | ||
Logger.debug(Strings.storeKit.skproductsrequest_failed(error: error)) | ||
// this one is duplicate, though other in ProductsFetcherSK1 is appleError |
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.
Yeah I'd say this one should be appleError
too.
@@ -119,9 +121,11 @@ private extension ProductsManager { | |||
productsFetcherSK1.products(withIdentifiers: removedProductIdentifiers, completion: { result in | |||
switch result { | |||
case.success: |
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.
Hun looks like this is missing a space?
case.success: | |
case .success: |
case .skproductsrequest_failed(let error): | ||
return "SKProductsRequest failed! error: \(error.localizedDescription)" | ||
case .store_products_request_failed(let error): | ||
return "Store products request failed! error: \(error.localizedDescription)" |
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.
Nit:
return "Store products request failed! error: \(error.localizedDescription)" | |
return "Store products request failed! Error: \(error.localizedDescription)" |
} catch { | ||
Logger.debug(Strings.storeKit.skproductsrequest_failed(error: error)) | ||
Logger.debug(Strings.storeKit.store_products_request_failed(error: error)) |
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.
Nit: For consistency I'd keep the "finished" wording, or change this to did_fail
as well.
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.
sorry, i'm lost, which finished wording?
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.
store_products_request_failed
vs store_product_request_did_finish
vs store_product_request_received_response
.
I would use "did {fail/finish/receive}" for all, or the past tense, for consistency.
Just mentioning that since you changed skproductsrequest_finished
to store_product_request_did_finish
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.
oh got it, i was looking for "finished" in that string you commented on. monday brain, thanks
While investigating #1765, I noticed logs between the
ProductsFetcherSK1
andProductsFetcherSK2
don't match, causing confusion. This PR adds some missing logs toProductsFetcherSK2