search

owncloud / android

:phone: The ownCloud Android App
GNU General Public License v2.0
3.84k stars 3.05k forks source link

[FEATURE REQUEST] Logging changes #4204

Closed manuelplazaspalacio closed 11 months ago

manuelplazaspalacio commented 12 months ago

Related Issues

App: https://github.com/owncloud/android/issues/4151

QA

manuelplazaspalacio commented 12 months ago

com.github.AppDevNext.Logcat:LogcatCoreLib library deleted due to compatibility issues with the app tests.

jesmrec commented 11 months ago

QA checks

Will compare request/responses caught with mitmproxy with the same ones in the logs. They must be identical:

jesmrec commented 11 months ago

Let's QA this one

jesmrec commented 11 months ago

(1)

Name of the file is not re-generated?

  1. Enable Logging
  2. Generate some logs by browsing etc... -> log file generated with name (f.ex): owncloud.2023-11-17_08.35.34.log
  3. Remove that file by clicking on the trashbin icon
  4. Generate more logs after some minutes

Current: log file name is the same: owncloud.2023-11-17_08.35.34.log Expected: new log file with different timestamp

Pixel 2 Android11 94c9fbafc

jesmrec commented 11 months ago

(2)

I generate a log file including several operations, i will comment some issues about that:

owncloud.2023-11-17_08.56.03.log

Aitorbp commented 11 months ago

In relation to the second QA report, related to the status code -1, it is caused because the mHttpCode variable has no value at this point and sets the default value, which is -1. When we have this case, where we have a resultCode.OK, there is no condition in the getLogMessage function that handles this case. Therefore, I would put one more condition for results with mCode == ResultCode.Ok, putting a message like: successful response.

jesmrec commented 11 months ago

putting a message like: successful response.

i don't get this point. Code statuses should not be replaced by strings, we are adding noises to the log. A "successful response" could be 200, 201, 207... is there no way to keep the code status?

Aitorbp commented 11 months ago

putting a message like: successful response.

i don't get this point. Code statuses should not be replaced by strings, we are adding noises to the log. A "successful response" could be 200, 201, 207... is there no way to keep the code status?

I have seen that in other parts of the application the log is managed from the Operation. For example, in GetUrlToOpenInWebRemoteOperation the following timber is written: Timber.d("Open in web for file: $fileId - $status${if (!isSuccess(status)) "(FAIL)" else ""}"). Here the status will appear correctly. In any case, the solution shown above, code statuses are replaced by strings in all cases.

JuancaG05 commented 11 months ago

In any case, the solution shown above, code statuses are replaced by strings in all cases.

It shouldn't be like that. We're losing information in such case.

jesmrec commented 11 months ago

About (1), not sure if directly related. Follow these steps:

  1. Enable logs and do some operations to create a file (check a log file)
  2. In device settings, move date to the following day to create a new log file correspondent to that date
  3. Open the app and generate logs (at this point, we have 2 different log files)
  4. In logs view, remove the latest file and generate more files

Current: two files (entries) are generated. Expected: only one generated

Pixel 2 Android11 aa4448660

Aitorbp commented 11 months ago
  • I noticed that the response body scapes the quotation marks in the HTTP RESPONSE: {"response":{"body":{"data":"{\"ocs\":{\"meta\":{\"status\"... (line 229). Why is that required?

Regarding the first QA bug, the method that parses the json responseJsonAdapter.toJson is managed by the Moshi library, in this case we cannot do much, the library considers parsing the data in this way.

Aitorbp commented 11 months ago

About (1), not sure if directly related. Follow these steps:

  1. Enable logs and do some operations to create a file (check a log file)
  2. In device settings, move date to the following day to create a new log file correspondent to that date
  3. Open the app and generate logs (at this point, we have 2 different log files)
  4. In logs view, remove the latest file and generate more files

Current: two files (entries) are generated. Expected: only one generated

Pixel 2 Android11 aa4448660

We are opened an issue to fix this:

Aitorbp commented 11 months ago

I update the general outline of the PR:

jesmrec commented 11 months ago

Summarizing:

I noticed that the response body scapes the quotation marks in the HTTP RESPONSE: {"response":{"body":{"data":"{\"ocs\":{\"meta\":{\"status\"... (line 229). Why is that required?

won't fix

There are some lines with Operation finished with HTTP status code -1 (success) (225, 289, 298...).

fixed

I noticed that some requests like avatar (check line 143) lacks of body response in the JSON when it is empty. However, if response is empty, it's logged {"response":{"body":{"data":"","length":0}... . Should be the same for both requests and responses?

won't fix, let's take this as correct

jesmrec commented 11 months ago

Will get this as first approach, open to new improvements.

I'd go for a better formatting on the lines. Let's take a look to an iOS log:

ownCloud_28_Nov_2023_at_10_11_37.log.txt

seems to be more structured (beyond the specific json-format of reqa/resps). Wdyt?