Update expensify common with new logging methods and improve logging #4191
Conversation
… and app inactive
iwiznia
force-pushed
the
ionatan_improve_logs
branch
from
fb3b2f4
to
2aa3b90
Compare
iwiznia
changed the title
src/App.js
Outdated
| Log.info('Flushing logs as app is going inactive', true, {}, true); | ||
| } | ||
| this.setState({appState: nextAppState}); | ||
| } |
There was a problem hiding this comment.
I think maybe we should do this somewhere else. There's no reason to have this logic in the main App component and we definitely don't need to store appState in this.state - setting state will trigger an entire re-render of the app which could be fine, but seems potentially undesirable.
Maybe it can go in libs/actions/App.js ?
There was a problem hiding this comment.
Great point. Will see where I can move it.
src/libs/API.js
Outdated
| @@ -471,7 +471,7 @@ function GetRequestCountryCode() { | |||
| */ | |||
| function Log(parameters) { | |||
| const commandName = 'Log'; | |||
| requireParameters(['message', 'parameters', 'expensifyCashAppVersion'], | |||
| requireParameters(['logPacket'], | |||
There was a problem hiding this comment.
What happened to expensifyCashAppVersion ?
There was a problem hiding this comment.
Oh, this should still be here, my bad
src/App.js
Outdated
|
|
||
| handleAppStateChange(nextAppState) { | ||
| if (nextAppState.match(/inactive|background/) && this.state.appState === 'active') { | ||
| Log.info('Flushing logs as app is going inactive', true, {}, true); |
There was a problem hiding this comment.
Did we test this on an actual device to see if the API call succeeds?
Just curious because there might things that prevent this like the network logic setting itself to "offline" or something or just inability to make API calls when the app is no longer active.
There was a problem hiding this comment.
I have not. Will do that test once I address the comments in the review
src/libs/Network.js
Outdated
| @@ -1,6 +1,7 @@ | |||
| import _ from 'underscore'; | |||
| import lodashGet from 'lodash/get'; | |||
| import Onyx from 'react-native-onyx'; | |||
| // eslint-disable-next-line import/no-cycle | |||
There was a problem hiding this comment.
I missed it.. which dependency is creating a cycle?
There was a problem hiding this comment.
Oh, this did not work, I "fixed" this with the hack of info = Log.info;, removing it.
| }; | ||
|
|
||
| window.Log = Log; |
|
LGTM!
…On Fri, Aug 6, 2021 at 7:01 AM Ionatan Wiznia ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/libs/HttpUtils.js
<#4191 (comment)>:
> @@ -28,10 +30,30 @@ function processHTTPRequest(url, method = 'get', body = null) {
* @returns {Promise}
*/
function xhr(command, data, type = CONST.NETWORK.METHOD.POST, shouldUseSecure = false) {
+ if (command !== 'Log') {
+ info('Making API request', false, {
Not at all, go ahead.
So, does the PR looks good? I need to retest it and update the other PRs
too.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#4191 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AH3RC73VB77VAIV7GZCNJNTT3QIN5ANCNFSM5A4DUHBQ>
.
--
github.com/marcaaron
|
|
Nice, thanks! Monday (or sometime when I don't need to do alloacations) will fix conflicts, update all the other PRs and retest them all. |
|
Updated and retested, please review. I will merge all related PRs once they are all ready. |
src/Expensify.js
Outdated
| registerStorageEventListener: (onStorageEvent) => { | ||
| listenToStorageEvents(onStorageEvent); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Heads up this logic was moved to another file which is why it's showing as an addition. Probably we don't want to call it in both places.
App/src/components/OnyxProvider.js
Lines 11 to 14 in 9368288
There was a problem hiding this comment.
Oh... that's why I had to re-add the imports 😄
Removing.
src/libs/Navigation/Navigation.js
Outdated
| @@ -54,6 +55,7 @@ function goBack(shouldOpenDrawer = true) { | |||
| * @param {String} route | |||
| */ | |||
| function navigate(route = ROUTES.HOME) { | |||
| Log.info('Navigating to route', false, {route}); | |||
There was a problem hiding this comment.
NAB. Nice addition of this log! It might be even better to add a logger when the navigation state changes. Which will also capture things like goBack() or openDrawer(). Probably can be done later as 90% of navigations use this method - but thought you might like to know this exists.
There was a problem hiding this comment.
So, I would need to add the log in this event right (and remove the logging from here)?
There was a problem hiding this comment.
Yep, I think the "currentURL" or "path" is probably the most valuable to have now and moving the log there should capture everything and not just when we explicit use of navigate()?
There's also some other information in the "state" besides the path that could be useful to log - but less sure about that - more info here if you are curious.
|
Alright updated, retested and off hold, please merge if all looks good. |
iwiznia
changed the title
| @@ -2,6 +2,8 @@ import _ from 'underscore'; | |||
| import CONFIG from '../CONFIG'; | |||
| import CONST from '../CONST'; | |||
|
|
|||
| let info = () => {}; | |||
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
Oh hmm actually I think the logging of the command is pretty noisy for a local console log. It would be good to be able to turn off these logs on dev. The same information can easily be gotten from Flipper or Network tab in Chrome Dev Tools. Lines 35 to 40 in a3597a8 |
|
I kind of liked seeing the logs 😄 |
|
Oh, also you can hide all those messages by removing the verbose logs |
|
🚀 Deployed to staging by @marcaaron in version: 1.0.85-10 🚀
|
Removing verbose logs also removes 99% of the
I'd rather make it opt-in via a |
|
Why would those want to be silenced? They are good information, no? |
|
We can get this information from Network tab in Chrome Dev Tools (web) or Flipper (native). Adding them to the console doesn't solve any problem where this information is hard to access. But I can think of a couple of reasons why we should not do it by default:
|
|
The logs will not only include network requests, but everything we log. I still think more data and context is better, but I guess that's preference, I'll add something up. I checked the env configuration, but not sure how it works exactly. Do I add it to the env file and it automatically will load in the Config |
|
🚀 Deployed to production by @roryabraham in version: 1.0.86-11 🚀
|
Details
Uses Expensify/expensify-common#397
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/171640
Tests
In addition to the QA tests, I also checked that the logs look good. Here's an example:
QA
Only on web:
On desktop and apps:
blob:"Flushing logs as app is going inactive".blob:"Flushing logs before signing out".Tested On