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

169 improve logging #499

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

watcher6280
Copy link

@watcher6280 watcher6280 commented Jan 24, 2024

Description

Looking into _loadEntries() there are a lot of initialisation calls being triggered and manually tracking each one to see if they are wrapped correctly in try/catches or if the response body is valid then testing if it is logged would be very time consuming.

  • I think overall the app would benefit from levelled logging where each provider will check for success and failure states and log as needed.
  • I added a logging_adaptor based on https://pub.dev/packages/logging to hopefully help do this and organise logs better.
  • I added two examples of how I think logs should be done in providers and how we can replace the current logs all at once.
  • If this is approved and merged seperate PRs can be made to replace all the standard log() calls with logger.info() calls over time while all new logs should use the relevant logger function.
  • It would also be good if we can come up with guidelines as to what logs fit into what levels. e.g
    'info' -> page navigation logs.
    'fine' -> api calls
    'finer' -> api calls and responses
    etc...

Benefits:

  • Logs can be enabled and disabled as needed e.g if you only want to see 'info' logs where logs for things like the initialisation flow, or if you want to show 'fine' logs where api calls would show up but not responses.
  • All logs pass through a single stream that can be called in other classes.
    e.g. If you want to add firebase crashlytics later, on initialisation of crashlytics you can call a function much like the logging adaptor does and pipe all the error logs to it.

static void listenForLogs() {

  Logger.root.onRecord.listen((record) {
    if (Logging.level != LogLevel.off) {
      final parsedLog = _parse(
        message: record.message,
        level: record.level,
        loggerName: record.loggerName,
      );
      Crashlytics.logException(parsedLog);
    }
  });
}

Link to the issue :

#169

Example log outputs

Screenshot 2024-01-24 at 10 32 47 am

Checklist

Please check that the PR fulfills all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • Set a 100 character limit in your editor/IDE to avoid white space diffs in the PR
  • Tests for the changes have been added (for bug fixes / features)
  • Added yourself to AUTHORS.md
  • Updated/added relevant documentation (doc comments with ///).
  • Added relevant reviewers.

Costas Korai added 4 commits January 24, 2024 10:33
- Handles the log functions.
- Handles listening and outputting logs to
the console.
@watcher6280 watcher6280 marked this pull request as ready for review January 24, 2024 00:03
@rolandgeider
Copy link
Member

definitely yes 😍! There is indeed a lack of logging everywhere in the application. Is logger.abs.dart generated from someting or does it come from some example in the logging package?

@watcher6280
Copy link
Author

Logger.abs.dart is based on the log levels you can find here https://github.com/dart-lang/logging/tree/master/lib/src and the functions used to log at each level.

It's nice to have since by setting up the logger with

late final Logging logger = LoggingAdaptor('HomeTabsScreen');

in classes, if you ever want to replace the logger, you can make a new adaptor and switch it in one location and have all the logger calls still passed to it.

E.g. updating to
late final Logging logger = NewLoggingAdaptor('HomeTabsScreen');

Would just replace the function that logger.fine() calls.

@Dieterbe
Copy link
Contributor

Dieterbe commented May 7, 2024

Logs can be enabled and disabled as needed e.g if you only want to see 'info' logs where logs for things like the initialisation flow, or if you want to show 'fine' logs where api calls would show up but not responses.

i don't think i've ever found levelled logging useful. in my experience, you spend work up front categorizing logs into categories, but when there's an issue and you actually need to debug, you want to see detail of some components, but not others (e.g. http request+response of a specific api call, but not all the other ones). switching to "fine" or "debug" or whatever just to see what you need, would also result in way more output of a gazillion things you don't care about. so in practice i've always ended up using tools like network inspectors, debuggers, or adding some prints to the code to troubleshoot a specific issue.
so all the work to set up levelled logging becomes wasted coding work.

Looking into _loadEntries() there are a lot of initialisation calls being triggered and manually tracking each one to see if they are wrapped correctly in try/catches or if the response body is valid then testing if it is logged would be very time consuming.

maybe i'm missing something here, but it seems trivial to me to glance at this function and see that everything is wrapped in try/catch blocks. i agree that to check correctness of individual calls, you'd need to go into each one, check that it handles http responses properly, etc; but i don't see how this is any different from the case where you want to check that each call logs correctly. you'd also need to go into each one and do the same.

The bigger problem i see is that we don't communicate about error conditions to the user. i'm pretty sure i've run into it where things would fail, and the app would just hang

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants