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

[BUG]: ConsoleLogger overwrites local log file for each line write #5232

Closed
BenHenning opened this issue Nov 16, 2023 · 8 comments · Fixed by #5550
Closed

[BUG]: ConsoleLogger overwrites local log file for each line write #5232

BenHenning opened this issue Nov 16, 2023 · 8 comments · Fixed by #5550
Assignees
Labels
bug End user-perceivable behaviors which are not desirable. good first issue This item is good for new contributors to make their pull request. Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Low Solution is clear and broken into good-first-issue-sized chunks.

Comments

@BenHenning
Copy link
Member

BenHenning commented Nov 16, 2023

Describe the bug

ConsoleLogger has support for logging output lines to a local log (rather than just logcat). However, it seems that this is currently configured such that each line will completely overwrite the original log file.

Steps To Reproduce

  • Build & install a developer version of Oppia Android.
  • Play around in the app for a bit to generate logs.
  • Pull logs from logcat (e.g. either in Android Studio or via adb logcat | grep org.oppia.android).
  • Pull the logs file: $ANDROID_HOME/platform-tools/adb pull /data/data/org.oppia.android/files/oppia_app.log ~/opensource/oppia_app.log
  • Compare these logs. Notice that the last entry in oppia_app.log is just the last log written in the logcat log.

Expected Behavior

All lines should be kept, not just the most recent.

Screenshots/Videos

No response

What device/emulator are you using?

Pixel 6a emulator

Which Android version is your device/emulator running?

SDK 33

Which version of the Oppia Android app are you using?

0.11-dev-80a9a09723

Additional Context

From the logic in ConsoleLogger:

blockingScope.launch { logDirectory.printWriter().use { out -> out.println(text) } }

I suspect this is caused by the open FileWriter just always, by default, overwriting the file. We should, instead, open the writer in append mode & add tests to verify that this functionality now works.

Also, it's probably a good idea to keep a long-lived PrintStream open for the log file rather than reopening it for each line (which is going to be less performant). We should overwrite the file in this case (to avoid indefinitely using more disk space for debug logs). This includes adding tests for verifying that multiple ConsoleLogger instances do overwrite the file from the previous logger.

@BenHenning BenHenning added bug End user-perceivable behaviors which are not desirable. triage needed labels Nov 16, 2023
@BenHenning
Copy link
Member Author

Suggest that this is a good potential starter issue since it should be straightforward to investigate, fix, and add tests for it.

@adhiamboperes adhiamboperes added Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Low Solution is clear and broken into good-first-issue-sized chunks. good first issue This item is good for new contributors to make their pull request. labels Nov 27, 2023
@kmanikanta335
Copy link
Contributor

can i work on this issue @adhiamboperes

@adhiamboperes
Copy link
Collaborator

Hi @kmanikanta335. Could you please share an overview of your proposed solution?

@kmanikanta335
Copy link
Contributor

@adhiamboperes
proposed solution

  1. Open ConsoleLogger.kt:
    Locate the file ConsoleLogger.kt in the Oppia Android project.

  2. Modify File Opening Mode:
    Change the file opening mode to append. Replace the existing line:
    blockingScope.launch { logDirectory.printWriter().use { out -> out.println(text) } }
    with:
    blockingScope.launch { logDirectory.appendText("$text\n") }

  3. Use Long-Lived PrintStream:
    Maintain a long-lived PrintStream for the log file. You can create it during the initialization of the ConsoleLogger or at an
    appropriate point where it won't be repeatedly opened. For example:

    private val logPrintStream: PrintStream = logDirectory.appendText().bufferedWriter().use { it }

    // ...

    blockingScope.launch { logPrintStream.println(text) }

@adhiamboperes
Copy link
Collaborator

@kmanikanta335, you can put up a draft PR with these changes that you have proposed.

@Saifuddin53
Copy link

Can I work on this issue as @kmanikanta335 has not posted any PR till now?

@manas-yu
Copy link
Contributor

manas-yu commented Oct 7, 2024

Hi @adhiamboperes can i work on this issue?
The issue seems to stem from how PrintWriter is used in the logToFileInBackground() method within the ConsoleLogger class.
we can modify the logToFileInBackground function to use appendText() to add content to the log file instead of overwriting it. and it seems to resolve the issue

private fun logToFileInBackground(text: String) {
    blockingScope.launch {
      if (printWriter == null) {
        printWriter = PrintWriter(FileWriter(logDirectory, true)) // Open in append mode
      }
      printWriter?.println(text)
      printWriter?.flush()
    }
  }

  /** Close the log file when logging is finished. */
  fun closeLogFile() {
    printWriter?.close()
    printWriter = null
  }
}

@adhiamboperes
Copy link
Collaborator

Hi @manas-yu, I have assigned the issue to you. Please feel free to put up a PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug End user-perceivable behaviors which are not desirable. good first issue This item is good for new contributors to make their pull request. Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Low Solution is clear and broken into good-first-issue-sized chunks.
Development

Successfully merging a pull request may close this issue.

6 participants