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

HTTPClient: improved log for failed requests #2669

Merged
merged 1 commit into from
Jun 16, 2023

Conversation

NachoSoto
Copy link
Contributor

Changed this log:

[network] DEBUG: ℹ️ API request failed: GET /v1/subscribers/17571811-B0A4-49DC-B326-572E602BF195: Request failed signature verification.

To this:

[network] DEBUG: ℹ️ API request failed: GET /v1/subscribers/BD9F38FF-33C6-4A77-86F7-6AEA92E6717D (200): Request failed signature verification.

This was useful when debugging #2668 and #2659.

Changed this log:
```
[network] DEBUG: ℹ️ API request failed: GET /v1/subscribers/17571811-B0A4-49DC-B326-572E602BF195: Request failed signature verification.
```

To this:
```
[network] DEBUG: ℹ️ API request failed: GET /v1/subscribers/BD9F38FF-33C6-4A77-86F7-6AEA92E6717D (200): Request failed signature verification.
```

This was useful when debugging #2668 and #2659.
@NachoSoto NachoSoto requested a review from a team June 16, 2023 01:47
@codecov
Copy link

codecov bot commented Jun 16, 2023

Codecov Report

Merging #2669 (63dea0f) into main (dececcb) will decrease coverage by 0.06%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main    #2669      +/-   ##
==========================================
- Coverage   86.37%   86.32%   -0.06%     
==========================================
  Files         207      207              
  Lines       14652    14677      +25     
==========================================
+ Hits        12656    12670      +14     
- Misses       1996     2007      +11     
Impacted Files Coverage Δ
Sources/Purchasing/Purchases/Purchases.swift 74.84% <0.00%> (-0.83%) ⬇️
Sources/Support/DebugUI/DebugContentViews.swift 38.05% <0.00%> (ø)
Sources/Logging/Strings/PurchaseStrings.swift 88.64% <90.90%> (+<0.01%) ⬆️
Sources/Logging/Strings/NetworkStrings.swift 100.00% <100.00%> (ø)
Sources/Networking/HTTPClient/HTTPClient.swift 98.40% <100.00%> (+<0.01%) ⬆️
...s/Purchasing/Purchases/PurchasesOrchestrator.swift 85.64% <100.00%> (+0.04%) ⬆️
...urces/Purchasing/Purchases/TransactionPoster.swift 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@@ -320,7 +320,9 @@ private extension HTTPClient {
httpCode: urlResponse?.httpStatusCode ?? response.statusCode
))
case let .failure(error):
Logger.debug(Strings.network.api_request_failed(request.httpRequest, error: error))
Logger.debug(Strings.network.api_request_failed(request.httpRequest,
httpCode: urlResponse?.httpStatusCode,
Copy link
Contributor

Choose a reason for hiding this comment

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

In the success case we fallback to the cached status code. Is there a reason we need that? Just wondering if it's the same 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.

Because urlResponse is optional here, for cases when the response fails catastrophically and there’s nothing. In the case of an error we don’t have a parsed response anyway, only this optional.

@NachoSoto NachoSoto merged commit 208bf53 into main Jun 16, 2023
@NachoSoto NachoSoto deleted the http-client-improved-log branch June 16, 2023 14:49
NachoSoto pushed a commit that referenced this pull request Jun 22, 2023
**This is an automatic release.**

### Bugfixes
* `PurchasesOrchestrator`: update `CustomerInfoManager` cache after
processing transactions (#2676) via NachoSoto (@NachoSoto)
* `ErrorResponse`: drastically improved error messages, no more "unknown
error"s (#2660) via NachoSoto (@NachoSoto)
* `PaywallExtensions`: post purchases with `Offering` identifier (#2645)
via NachoSoto (@NachoSoto)
* Support `product_plan_identifier` for purchased subscriptions from
`Google Play` (#2654) via Josh Holtz (@joshdholtz)
### Performance Improvements
* `copy(with: VerificationResult)`: optimization to avoid copies (#2639)
via NachoSoto (@NachoSoto)
### Other Changes
* `ETagManager`: refactored e-tag creation and tests (#2671) via
NachoSoto (@NachoSoto)
* `getPromotionalOffer`: return `ErrorCode.ineligibleError` if receipt
is not found (#2678) via NachoSoto (@NachoSoto)
* `TimingUtil`: removed slow purchase logs (#2677) via NachoSoto
(@NachoSoto)
* `CI`: changed `Codecov` to `informational` (#2670) via NachoSoto
(@NachoSoto)
* `LoadShedderIntegrationTests`: verify requests are actually handled by
load shedder (#2663) via NachoSoto (@NachoSoto)
* `ETagManager.httpResultFromCacheOrBackend`: return response headers
(#2666) via NachoSoto (@NachoSoto)
* `Integration Tests`: added tests to verify 304 behavior (#2659) via
NachoSoto (@NachoSoto)
* `HTTPClient`: disable `URLSession` cache (#2668) via NachoSoto
(@NachoSoto)
* Documented `HTTPStatusCode.isSuccessfullySynced` (#2661) via NachoSoto
(@NachoSoto)
* `NetworkError.signatureVerificationFailed`: added status code to error
`userInfo` (#2657) via NachoSoto (@NachoSoto)
* `HTTPClient`: improved log for failed requests (#2669) via NachoSoto
(@NachoSoto)
* `ETagManager`: added new verbose logs (#2656) via NachoSoto
(@NachoSoto)
* `Signature Verification`: added test-only log for debugging invalid
signatures (#2658) via NachoSoto (@NachoSoto)
* Fixed `HTTPResponse.description` (#2664) via NachoSoto (@NachoSoto)
* Changed `Logger` to use `os_log` (#2608) via NachoSoto (@NachoSoto)
* `MainThreadMonitor`: increased threshold (#2662) via NachoSoto
(@NachoSoto)
* `debugRevenueCatOverlay`: display `receiptURL` (#2652) via NachoSoto
(@NachoSoto)
* `PurchaseTester`: added ability to display `debugRevenueCatOverlay`
(#2653) via NachoSoto (@NachoSoto)
* `debugRevenueCatOverlay`: ability to close on `macOS`/`Catalyst`
(#2649) via NachoSoto (@NachoSoto)
* `debugRevenueCatOverlay`: added support for `macOS` (#2648) via
NachoSoto (@NachoSoto)
* `LoadShedderIntegrationTests`: enable signature verification (#2655)
via NachoSoto (@NachoSoto)
* `ImageSnapshot`: fixed Xcode 15 compilation (#2651) via NachoSoto
(@NachoSoto)
* `OfferingsManager`: don't clear offerings cache timestamp when request
fails (#2359) via NachoSoto (@NachoSoto)
* `StoreKitObserverModeIntegrationTests`: added test for posting
renewals (#2590) via NachoSoto (@NachoSoto)
* Always initialize `StoreKit2TransactionListener` even on SK1 mode
(#2612) via NachoSoto (@NachoSoto)
* `ErrorUtils.missingReceiptFileError`: added receipt URL `userInfo`
context (#2650) via NachoSoto (@NachoSoto)
* Added `.xcprivacy` for Xcode 15 (#2619) via NachoSoto (@NachoSoto)
* `Trusted Entitlements`: added debug log with
`ResponseVerificationMode` (#2647) via NachoSoto (@NachoSoto)
* `debugRevenueCatOverlay`: simplified title (#2641) via NachoSoto
(@NachoSoto)
* Simplified `Purchases.updateAllCachesIfNeeded` (#2626) via NachoSoto
(@NachoSoto)
* `HTTPResponseTests`: fixed disabled test (#2643) via NachoSoto
(@NachoSoto)
* Add `InternalDangerousSettings.forceSignatureFailures` (#2635) via
NachoSoto (@NachoSoto)
* `IntegrationTests`: explicit `StoreKit 1` mode (#2636) via NachoSoto
(@NachoSoto)
* `Signing`: removed API for loading key from a file (#2638) via
NachoSoto (@NachoSoto)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants