Restructure the Logger

[vinitamurthi]

With the way we have loggers right now, app/domain files would have to fetch multiple types of loggers for different types of logging. We want to refactor it so that atleast app/domain modules can use a single logger: Oppia Logger. This restructuring will have multiple parts to it:

• [x] Rename Logger.kt -> ConsoleLogger.kt since that's what it does (Data/Utility module files will use this file directly)

• [x] Create an OppiaLogger.kt file in the domain module. All App/Domain module files will use this logger

• [x] Oppia Logger will route the log command to the lower lying logger: - logException routes to ExceptionLogger (or exceptionController) - logTransition/logClick routes to the AnalyticsController - .v/.d etc commands route to the ConsoleLogger

[BenHenning]

150 seems a bit high for this @Sarthak2601, doesn't it? I'd expect this to be roughly 1 medium-sized PR unless I'm missing some aspect of this. Though maybe 2 PRs if it also includes updating all logger references to use the new OppiaLogger, but that's a fairly straightforward find-and-replace.

[BenHenning]

@vinitamurthi one question: if OppiaLogger is in domain/ and used by domain/ & app/ for console logging, does that mean all other modules will still use ConsoleLogger? That seems like an inconsistency we might not want since it may lead to confusion. WDYT?

[vinitamurthi]

Today yes, but as we had discussed we will move this to its own module in blaze so this confusion should be short lived. Unfortunately with the way its structured right now, we have no other proper choice.

[Sarthak2601]

150 seems a bit high for this @Sarthak2601, doesn't it? I'd expect this to be roughly 1 medium-sized PR unless I'm missing some aspect of this. Though maybe 2 PRs if it also includes updating all logger references to use the new OppiaLogger, but that's a fairly straightforward find-and-replace.

@BenHenning We estimated it to be a 150 pointer since the contributor will have to understand the logging, combine it into one place, replace the existing implementations and write test cases. However, we can initially put it in as a 75 pointer and then later on decide if we want to increase it to 150 when anyone creates a PR.

[BenHenning]

That sounds good, thanks @Sarthak2601.

Ack @vinitamurthi. I suppose the alternative is to not introduce OppiaLogger until we can introduce it cleanly, but since it already exists, we might as well leverage it as broadly as we can. :) I don't have a strong opinion about this.

[Aditiwebd]

Please assign it to me @Sarthak2601

[Arjupta]

@Sarthak2601 and @vinitamurthi I would like to work on this Issue.

Just a simple question. The task which is now left is to use Oppia Logger everywhere and route the log requests according to different log command?

[vinitamurthi]

I think that's right. I beleive all other logging should be part of oppialogger now so we should just have to update callers.

[Sarthak2601]

@ARJUPTA Yes, that's partially correct. You'll have to add public methods in the logger to ensure all other logging (currently it's just analytics that we do) via the logger, update the logging calls everywhere and then write corresponding test cases.

[rt4914]

@ARJUPTA Unassigning you from this issue because of inactivity, please re-assign yourself if you are currently working on this.

[Arjupta]

@rt4914 I guess we can close this issue as it was resolved in the PR #3104

[rt4914]

@ARJUPTA That PR mentions that only part of it was done. Am I missing any context here?

[BenHenning]

Per @adhiamboperes's suggestion, this can be closed now. In fact, it's probably gone from being completed to obsolete since we've decided to expand OppiaLogger back into specialized loggers. I think we still don't have a strong sense of what a good logger architecture looks like in the app yet. I'm still holding out hope for https://github.com/google/flogger/issues/186 as it will allow us to at least differentiate console & error logging with analytics (which can use a more domain-specific paradigm), but I think we should just live with the current approach and iterate on it as seems reasonable.

[github-actions[bot]]

The issue is reopened because of the following unresolved TODOs: https://github.com/oppia/oppia-android/blob/d0c8b81566c956c6edf6a7c19c14f8c8391dd939/domain/src/main/java/org/oppia/android/domain/oppialogger/analytics/AnalyticsController.kt#L197 https://github.com/oppia/oppia-android/blob/d0c8b81566c956c6edf6a7c19c14f8c8391dd939/domain/src/main/java/org/oppia/android/domain/oppialogger/analytics/PerformanceMetricsController.kt#L84 https://github.com/oppia/oppia-android/blob/d0c8b81566c956c6edf6a7c19c14f8c8391dd939/domain/src/main/java/org/oppia/android/domain/oppialogger/exceptions/ExceptionsController.kt#L134

[BenHenning]

Aha, looks like there are a few TODOs to resolve before this can be fully closed.