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

Send all logs to server! #5624

Closed
roryabraham opened this issue Oct 1, 2021 · 13 comments
Closed

Send all logs to server! #5624

roryabraham opened this issue Oct 1, 2021 · 13 comments
Assignees
Labels

Comments

@roryabraham
Copy link
Contributor

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

Something unexpected happens on staging or production when we're not explicitly looking for it.

Expected Result:

We should be able to find a paper-trail of logs to investigate the failure.

Actual Result:

Many logs are client-only and not logged to the server.

Workaround:

None (at least, not on mobile apps)

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number:
Reproducible in staging?:
Reproducible in production?:
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:
Issue reported by:
Slack conversation:

View all open jobs on GitHub

@roryabraham roryabraham added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Oct 1, 2021
@MelvinBot
Copy link

Triggered auto assignment to @davidcardoza (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Oct 1, 2021
@roryabraham
Copy link
Contributor Author

This should be pretty easy to switch out for now. But maybe we can also set up an ESLint rule to disallow console.debug to prevent this from happening in the future.

@marcaaron
Copy link
Contributor

Are you up for discussing this some more before opening a PR?

@marcaaron
Copy link
Contributor

A few questions I have...

  • Should we continue using console.debug at all?
  • Will the solution be to just change all console.debug to Log.info?
  • Will we send those logs the the server immediately or not?
  • Should we clean up logs that clearly have no value to log to the server?
  • Is there some way to make sure these server logs do not show in the JS console on production? This may happen already by default, but I'm not sure.

@roryabraham
Copy link
Contributor Author

Should we continue using console.debug at all?

I think no. If something is useful to log, log it to the server. Otherwise, don't log it at all!

Will the solution be to just change all console.debug to Log.info?

Prettymuch, but maybe we can delete some console.debug that aren't actually going to help us diagnose a problem.

Will we send those logs the the server immediately or not?

Not sure. What do you think?

Should we clean up logs that clearly have no value to log to the server?

Yes.

Is there some way to make sure these server logs do not show in the JS console on production? This may happen already by default, but I'm not sure.

I just logged in on production, and it looks like all the Log.info logs are not showing up. But they do show up on dev. So we should be good there.

@marcaaron
Copy link
Contributor

Not sure. What do you think?

I am thinking maybe all of these logs happening immediately will create a lot of network traffic so we should batch them and not send immediately. At least locally my dev environment tends to hang when there are a lot of calls to Log command.

I just logged in on production, and it looks like all the Log.info logs are not showing up. But they do show up on dev. So we should be good there.

I think it's because of this so we should not remove that console.debug ?

App/src/libs/Log.js

Lines 37 to 40 in ef1eb11

clientLoggingCallback: (message) => {
console.debug(message);
},
isDebug: !CONFIG.IS_IN_PRODUCTION,

@marcaaron
Copy link
Contributor

Maybe some logs like this don't really make sense to log to the server since they only happen on dev

// Sometimes there might be no start mark recorded and the measure will fail with an error
console.debug(error.message);

@marcochavezf
Copy link
Contributor

marcochavezf commented Oct 1, 2021

my two cents here since I'm working on this issue which is part of the same thread that gave origin to this issue :)

Will we send those logs the the server immediately or not?

Since the app is offline first I think we should have a mechanism to collect the logs when the app is offline and send them to the server when the app is back online.

@marcaaron
Copy link
Contributor

Since the app is offline first I think we should have a mechanism to collect the logs when the app is offline and send them to the server when the app is back online.

Interesting, I guess that problem does fall under "send all logs". The network layer is supposed to work like this:

  1. Try to log but we are offline and the request gets queued
  2. Come back online and we try to make all the queued requests
  3. Any place where we logged should then send a log to the server

I have no idea whether that works in practice or if the logs actually get sent in the order that they were created or if they retain timing information about when they happened. Maybe @iwiznia knows since he worked on some logging stuff recently.

@iwiznia
Copy link
Contributor

iwiznia commented Oct 4, 2021

I assume logs are sent correctly when you reconnect, but I have not tested it.
As for the timestamp, we do send the original timestamp collected in the device, but IIRC we are not logging it to the file and the timestamp you will see there is when it got to the server.

@MelvinBot
Copy link

@roryabraham Whoops! This issue is 2 days overdue. Let's get this updated quick!

@roryabraham
Copy link
Contributor Author

Sorry, no update on this and won't get to it today. TBH this feels like a weekly.

@roryabraham
Copy link
Contributor Author

PR is up and ready for review.

@MelvinBot MelvinBot removed the Overdue label Oct 13, 2021
@roryabraham roryabraham added the Reviewing Has a PR in review label Oct 13, 2021
@botify botify closed this as completed Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants