Skip to content
This repository was archived by the owner on Mar 19, 2024. It is now read-only.

Better logging #275

Closed
wants to merge 4 commits into from
Closed

Better logging #275

wants to merge 4 commits into from

Conversation

hannesa2
Copy link
Contributor

@hannesa2 hannesa2 commented Nov 2, 2019

It's about to change logging from self-made to Timber.
All is encapsulated with a logging lib which I use in several places.
I kept to old Log_OC , to easy migrate and do not touch all files. Especially when you do a huge refactoring

One major advantage for developers:
Before:

image

NEW: you can click log log line and jump direct to code ! You see the linenumber as well

image

Copy link
Contributor

@abelgardep abelgardep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of comments @hannesa2 .

By the way, it could be interesting to deprecate those logs with TAG as parameter, and create new ones without it to use them from now on.

Comment on lines -202 to -205
// Check if current log file size is bigger than the max file size defined
if (mLogFile.length() > MAX_FILE_SIZE) {
isMaxFileSizeReached = true;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if we lose this check? Could logs be huge or how is logs size controlled now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before

There were two files. Old and new. The new overwrites the old. So it means overall log were split into two similar size files and you see always the current.

New

EveryTime you run into Application create a new file with current date is created/reused. The old keeps there till you manually remove it.
Btw, the file-logging is in my point of view pointless, you see current logcat (in release !) anyways. It was a workaround to see what happens in release anyhow

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks for this explanation! I think this is something to check with @davigonz and @jesmrec

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will check in QA stage. Just one question: does the rotation happen every time you open the app? does this not lead to log file flooding?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, only if it's really opened. Only a click on the icon does not start app, when it's already open in background.
But when you remove app from task list, it will create a file, but maximum only one per day

@hannesa2
Copy link
Contributor Author

hannesa2 commented Nov 15, 2019

By the way, it could be interesting to deprecate those logs with TAG as parameter, and create new ones without it to use them from now on.

We could, but as more sensefull second step, a complete replacement (deletion) of Log_OC with Timber would make sense

Anyway, I introduced this @Deprecation

@hannesa2
Copy link
Contributor Author

I keep this Log_OC only because of seamless logging change during huge refactoring.
Without your refactoring, I would immediate remove this facade.

@hannesa2
Copy link
Contributor Author

Let's continue there #283

@hannesa2 hannesa2 closed this Nov 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants