Log wordpress-rs request errors via AppLog and centralize WpComApiClient construction#22979
Log wordpress-rs request errors via AppLog and centralize WpComApiClient construction#22979oguzkocer wants to merge 3 commits into
wordpress-rs request errors via AppLog and centralize WpComApiClient construction#22979Conversation
Project dependencies changeslist! Upgraded Dependencies
rs.wordpress.api:android:trunk-97d62ffff2983c0c71f2bbf95254156eba9aa57c, (changed from trunk-96105487c29b4205f25ff722ec80704ef8baa263)
rs.wordpress.api:kotlin:trunk-97d62ffff2983c0c71f2bbf95254156eba9aa57c, (changed from trunk-96105487c29b4205f25ff722ec80704ef8baa263)tree +--- project :libs:fluxc
-| \--- rs.wordpress.api:android:trunk-96105487c29b4205f25ff722ec80704ef8baa263
-| +--- com.squareup.okhttp3:okhttp:5.4.0 (*)
-| +--- com.squareup.okhttp3:okhttp-tls:5.4.0
-| | +--- org.jetbrains.kotlin:kotlin-stdlib:2.1.21 -> 2.3.21 (*)
-| | +--- com.squareup.okio:okio:3.17.0 (*)
-| | \--- com.squareup.okhttp3:okhttp:5.4.0 (*)
-| +--- net.java.dev.jna:jna:5.19.0
-| +--- rs.wordpress.api:kotlin:trunk-96105487c29b4205f25ff722ec80704ef8baa263
-| | +--- com.squareup.okhttp3:okhttp:5.4.0 (*)
-| | +--- com.squareup.okhttp3:okhttp-tls:5.4.0 (*)
-| | +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.10.2 -> 1.11.0 (*)
-| | \--- org.jetbrains.kotlin:kotlin-stdlib:2.1.21 -> 2.3.21 (*)
-| \--- org.jetbrains.kotlin:kotlin-stdlib:2.1.21 -> 2.3.21 (*)
+| \--- rs.wordpress.api:android:trunk-97d62ffff2983c0c71f2bbf95254156eba9aa57c
+| +--- com.squareup.okhttp3:okhttp:5.4.0 (*)
+| +--- com.squareup.okhttp3:okhttp-tls:5.4.0
+| | +--- org.jetbrains.kotlin:kotlin-stdlib:2.1.21 -> 2.3.21 (*)
+| | +--- com.squareup.okio:okio:3.17.0 (*)
+| | \--- com.squareup.okhttp3:okhttp:5.4.0 (*)
+| +--- net.java.dev.jna:jna:5.19.0
+| +--- rs.wordpress.api:kotlin:trunk-97d62ffff2983c0c71f2bbf95254156eba9aa57c
+| | +--- com.squareup.okhttp3:okhttp:5.4.0 (*)
+| | +--- com.squareup.okhttp3:okhttp-tls:5.4.0 (*)
+| | +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.10.2 -> 1.11.0 (*)
+| | \--- org.jetbrains.kotlin:kotlin-stdlib:2.1.21 -> 2.3.21 (*)
+| \--- org.jetbrains.kotlin:kotlin-stdlib:2.1.21 -> 2.3.21 (*)
-\--- rs.wordpress.api:android:trunk-96105487c29b4205f25ff722ec80704ef8baa263 (*)
+\--- rs.wordpress.api:android:trunk-97d62ffff2983c0c71f2bbf95254156eba9aa57c (*) |
|
|
|
|
Bump wordpress-rs to the PR #1398 build that adds RequestErrorLogger, and wire RequestErrorLogger { AppLog.e(T.API, it) } into every WpComApiClient and WpApiClient construction site. Failed requests are now logged under the API tag (and crash-reporting breadcrumbs) instead of going unlogged.
Route DataViewViewModel (with its SubscribersViewModel, TermsViewModel, and ApplicationPasswordsViewModel subclasses) and AddSubscribersViewModel through WpComApiClientProvider instead of constructing WpComApiClient directly. Keeps the RequestErrorLogger wiring in one place and drops the per-call auth/ interceptor/logger boilerplate. The provider now attaches TrackNetworkRequestsInterceptor, so all its consumers (previously only these ViewModels) become inspectable via the in-app network tracker when that debug setting is enabled.
oguzkocer
force-pushed
the
wpcom-api-client-error-logging
branch
from
7495634 to
6e3e0c5
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #22979 +/- ##
==========================================
- Coverage 37.44% 37.44% -0.01%
==========================================
Files 2325 2325
Lines 125214 125208 -6
Branches 17159 17159
==========================================
- Hits 46884 46881 -3
+ Misses 74523 74520 -3
Partials 3807 3807 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| private val wpComClients = mutableMapOf<Long, WpApiClient>() | ||
| private val selfHostedClients = mutableMapOf<Int, WpApiClient>() | ||
|
|
||
| private val errorLogger = RequestErrorLogger { AppLog.e(AppLog.T.API, it) } |
There was a problem hiding this comment.
❓ Can't the RequestErrorLogger be injected?
There was a problem hiding this comment.
What would be the point of injecting it?
There was a problem hiding this comment.
I'm thinking on detaching the class from the RequestErrorLogger, so it's more testable and non-dependent.
But also, I see WpComApiClientProvider creating the object again, so the object creation could be centralised.
Not a blocker, though.
There was a problem hiding this comment.
All it does is this AppLog.e(AppLog.T.API, it). Injecting that seems like too much ceremony and there is nothing to test.
If we add any logic to it, it'll make sense, but in my opinion it's premature to do it now.
|
Claude pointed a couple of privacy concerns, so it would be great to double-check them: |
|
@adalpari Any reason you merged |
Oh boy... sorry I hit the button on the wrong PR :( |
This is a fair point. I looked into it again and verified that we do not log the headers which is the important thing. The main risk with the current implementation is the failed response body and to a lesser extent the query params being logged. If we want to be extra sure, we can strip the url params and only include the error information. @jkmassel Any thoughts on whether we want to keep the full details for the logs or go with a leaner log in favor of extra security? |
If we are sure about that, then I think it should be enough, + maybe query params? Just a note that when we worked on the network logger, we had to hide the body on RPC calls because it was exposing sensitive data. I guess it doesn't apply here, but just to have it in mind. |
It's unlikely to be an issue here especially since it's response body, but it might. Since it's out of our control, it's probably safer to not log the response. For the query params, it's probably fine to log and could be useful for debugging, but I can see an argument for being extra cautious and stripping those out as well. |
4 participants
Description
Failed
wordpress-rsrequests were not surfaced anywhere in the app's own diagnostics. This PR routes them toAppLog(so they appear in the in-app log viewer and crash-reporting breadcrumbs under theAPItag) and, as a follow-up, consolidatesWpComApiClientconstruction so that wiring lives in a single place.The PR is split into two independent commits. The second is a cleanup and can be dropped without affecting the first.
Changes
1. Route request errors to
AppLogRequestErrorLogger { AppLog.e(AppLog.T.API, it) }into everyWpComApiClient/WpApiClientconstruction site (WpComApiClientProvider, the three constructions in fluxc'sWpApiClientProvider, and the two ViewModels that built clients directly).Self-contained and shippable on its own.
2. Centralize
WpComApiClientconstructionDataViewViewModel(and itsSubscribersViewModel,TermsViewModel,ApplicationPasswordsViewModelsubclasses) andAddSubscribersViewModelthroughWpComApiClientProvider.getWpComApiClient(token)instead of constructingWpComApiClientdirectly.trackNetworkRequestsInterceptor/networkAvailabilityProviderconstructor params from those ViewModels (a net simplification).WpComApiClientProvidernow attachesTrackNetworkRequestsInterceptoritself, so the moved clients keep their network-inspector behavior.Behavior change
Because
WpComApiClientProvidernow attachesTrackNetworkRequestsInterceptor, all of its consumers (StatsDataSourceImpl,NewDomainsSearchRepository,HESupportRepository,AIBotSupportRepository, plus the migrated ViewModels) become inspectable via the in-app Track network requests debug tool. This is preference-gated (off unless the user enables it) and makes coverage uniform across WP.com clients.Testing instructions
Me → Domains → Find a domainrequest to fail) and confirm aWordPress-API-tagged error appears in logcat / the in-app log viewer, e.g.E/WordPress-API: UnknownError(status=400, method=GET, url=…, response=…).Me → Subscribers(and Add subscribers), Terms (categories/tags), and Application Passwords.