[FEATURE REQUEST] Logging changes by manuelplazaspalacio · Pull Request #4204 · owncloud/android

manuelplazaspalacio · 2023-10-31T07:41:39Z

Related Issues

App: #4151

Added changelog files for the fixed issues in folder changelog/unreleased. More info here

QA

(1) Name of the file [FEATURE REQUEST] Logging changes #4204 (comment) MOVED TO [BUG] Two logs are displayed when you open the app #4236
(2) Issues in request logging [FEATURE REQUEST] Logging changes #4204 (comment)

manuelplazaspalacio · 2023-11-03T10:43:06Z

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

JuancaG05

Some questions and changes requested here @manuelplazaspalacio! 🚀

JuancaG05

Approved from my side! Good job 👍

jesmrec · 2023-11-07T18:16:29Z

JuancaG05

Approved after the latest changes 👍

jesmrec · 2023-11-16T07:47:10Z

Let's QA this one

jesmrec · 2023-11-16T07:50:02Z

(1)

Name of the file is not re-generated?

Enable Logging
Generate some logs by browsing etc... -> log file generated with name (f.ex): owncloud.2023-11-17_08.35.34.log
Remove that file by clicking on the trashbin icon
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 · 2023-11-16T10:04:54Z

(2)

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

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

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?
There are some lines with Operation finished with HTTP status code -1 (success) (225, 289, 298...).
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?

Aitorbp · 2023-11-24T13:33:06Z

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 · 2023-11-27T07:30:15Z

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 · 2023-11-27T08:50:07Z

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 · 2023-11-27T08:59:29Z

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 · 2023-11-28T07:59:31Z

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

Enable logs and do some operations to create a file (check a log file)
In device settings, move date to the following day to create a new log file correspondent to that date
Open the app and generate logs (at this point, we have 2 different log files)
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 · 2023-11-28T12:03:29Z

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 · 2023-11-28T12:04:50Z

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

Enable logs and do some operations to create a file (check a log file)

In device settings, move date to the following day to create a new log file correspondent to that date

Open the app and generate logs (at this point, we have 2 different log files)

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:

[BUG] Two logs are displayed when you open the app #4236

Aitorbp · 2023-11-28T12:19:40Z

I update the general outline of the PR:

Fix 1: Related to tho name of the file is not re-generated. (Waiting QA approval)
Fix 2:

Response body scapes the quotation marks. (answered in previous comment)

Lines with Operation finished with HTTP status code -1. (Waiting QA approval)

Some requests like avatar (check line 143) lacks of body response in the JSON when it is empty. (Need more info)

Two logs are created when no logs in screen and close and open app. (Created new issue)

jesmrec · 2023-11-28T12:54:02Z

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 · 2023-11-28T13:01:35Z

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?

- Updating the appdevnext logcat library. - Creating an OCFileLogginTree to modify the name of the loggin file. - Adding missing libraries in data. - Updating files related with the missing libraries.

…patibility issues with the app tests.

[FEATURE REQUEST] Logging changes

manuelplazaspalacio changed the title ~~[FEATURE REQUEST]loggin changes~~ [FEATURE REQUEST] Loggin changes Oct 31, 2023

JuancaG05 changed the title ~~[FEATURE REQUEST] Loggin changes~~ [FEATURE REQUEST] Logging changes Oct 31, 2023

manuelplazaspalacio marked this pull request as ready for review October 31, 2023 07:52

manuelplazaspalacio force-pushed the feature/loggin_changes branch from bd549a7 to 890cb6b Compare October 31, 2023 08:01

manuelplazaspalacio linked an issue Oct 31, 2023 that may be closed by this pull request

[FEATURE REQUEST] Logging changes #4151

Closed

12 tasks

manuelplazaspalacio force-pushed the feature/loggin_changes branch 2 times, most recently from cabcda4 to b21e1a5 Compare November 3, 2023 10:34

JuancaG05 requested changes Nov 6, 2023

View reviewed changes

manuelplazaspalacio force-pushed the feature/loggin_changes branch 2 times, most recently from b535cde to 963c1dd Compare November 6, 2023 12:52

JuancaG05 approved these changes Nov 6, 2023

View reviewed changes

manuelplazaspalacio force-pushed the feature/loggin_changes branch from b8cd315 to 3e0b009 Compare November 15, 2023 10:37

JuancaG05 approved these changes Nov 15, 2023

View reviewed changes

JuancaG05 force-pushed the feature/loggin_changes branch from b2ba02e to 94c9fba Compare November 15, 2023 13:49

JuancaG05 assigned manuelplazaspalacio Nov 15, 2023

JuancaG05 force-pushed the feature/loggin_changes branch from 94c9fba to 9a26718 Compare November 16, 2023 07:25

Aitorbp mentioned this pull request Nov 28, 2023

[BUG] Two logs are displayed when you open the app #4236

Closed

jesmrec added the Approved by qa label Nov 28, 2023

manuelplazaspalacio and others added 12 commits November 28, 2023 16:58

Changing log file name:

a5bab43

- Updating the appdevnext logcat library. - Creating an OCFileLogginTree to modify the name of the loggin file. - Adding missing libraries in data. - Updating files related with the missing libraries.

Loggin http logs in one line and in Json format

892b18e

Added changelog file to feature logging changes.

83e8762

Added livedata and annotation libraries to the library gradle.

075b2f2

Solving lint bug.

1c31f88

com.github.AppDevNext.Logcat:LogcatCoreLib library deleted due to com…

8993b7c

…patibility issues with the app tests.

Logging changes CR changes

684c579

Changing http log request and response.

1dc1d24

- Changes in http lof format

450d3a1

QA - Fixed date when the only log is deleted

564cf74

QA - Added status in logs

fa61c6d

QA - resolved casuistic when log is enabled

af67d27

Aitorbp force-pushed the feature/loggin_changes branch from cee75c3 to af67d27 Compare November 28, 2023 17:04

Aitorbp and others added 2 commits November 29, 2023 11:12

Bitrise fix

f5d71ab

Calens changelog updated

8466409

Aitorbp merged commit fa7a484 into master Nov 29, 2023

Aitorbp deleted the feature/loggin_changes branch November 29, 2023 11:28

jesmrec mentioned this pull request Dec 13, 2023

Logging improvements (structure) #4259

Open

Aitorbp added a commit that referenced this pull request Feb 5, 2024

Merge pull request #4204 from owncloud/feature/loggin_changes

232515a

[FEATURE REQUEST] Logging changes

Conversation

manuelplazaspalacio commented Oct 31, 2023 • edited by jesmrec Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related Issues

QA

Uh oh!

manuelplazaspalacio commented Nov 3, 2023

Uh oh!

JuancaG05 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JuancaG05 left a comment

Choose a reason for hiding this comment

Uh oh!

jesmrec commented Nov 7, 2023 • edited by Aitorbp Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JuancaG05 left a comment

Choose a reason for hiding this comment

Uh oh!

jesmrec commented Nov 16, 2023

Uh oh!

jesmrec commented Nov 16, 2023

(1)

Uh oh!

jesmrec commented Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

(2)

Uh oh!

Aitorbp commented Nov 24, 2023

Uh oh!

jesmrec commented Nov 27, 2023

Uh oh!

Aitorbp commented Nov 27, 2023

Uh oh!

JuancaG05 commented Nov 27, 2023

Uh oh!

jesmrec commented Nov 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Aitorbp commented Nov 28, 2023

Uh oh!

Aitorbp commented Nov 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Aitorbp commented Nov 28, 2023 • edited by JuancaG05 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jesmrec commented Nov 28, 2023

Uh oh!

jesmrec commented Nov 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

manuelplazaspalacio commented Oct 31, 2023 •

edited by jesmrec

Loading

jesmrec commented Nov 7, 2023 •

edited by Aitorbp

Loading

jesmrec commented Nov 16, 2023 •

edited

Loading

jesmrec commented Nov 28, 2023 •

edited

Loading

Aitorbp commented Nov 28, 2023 •

edited

Loading

Aitorbp commented Nov 28, 2023 •

edited by JuancaG05

Loading