immich
immich copied to clipboard
feat(mobile): show more errors in the app log
Many helpful errors are sent to debugPrint() (i.e. logcat on Android) which requires additional efforts to read them.
Use the app's own logging mechanism instead to make them appear in the builtin log viewer, making them easily available to end-users.
I would like to get some feedback on this one before proceeding:
- Is there any overall reason not to do this? I had a hard time when initially trying to set up my reverse proxy because I couldn't find logs in the app nor in the docker containers. Finally checked logcat and there they were, but I would have really liked to see them in the integrated log viewer (see #6490).
- Is the error message text fine like this? To me it feels a bit strange to merge the error details (usually the raw server response) into the error description (e.g. "Error logging in"). I looked around a bit and noticed that
ImmichLoggerdoesn't care aboutrecord.error- maybe it should save that attribute as well, so that the error description could be kept short and human-readable, while error details could be displayed in the details page? - Should the stack trace be included or not?
Thanks a lot for working on this. I wanted to do this for a long time but never got around to do this.
1. Is there any overall reason not to do this?
There isn't but one issue that I can think of is that if the logs are left uncleared, we'll be keep on adding entries into the db but it shouldn't be that big of an issue though. I'd love more errors to be logged with more suitable logger levels. Not all info logs we have are required by default and few can be moved to fine / finer level and stuff.
2. Is the error message text fine like this? To me it feels a bit strange to merge the error details (usually the raw server response) into the error description (e.g. "Error logging in"). I looked around a bit and noticed that `ImmichLogger` doesn't care about `record.error` - maybe it should save that attribute as well, so that the error description could be kept short and human-readable, while error details could be displayed in the details page?
This is something that has to be fixed as well. Can you fix this in a separate PR though? i.e, Update ImmichLogger to use the error from the record.
3. Should the stack trace be included or not?
It depends. For server side issues and http errors, we might not need the stack trace as long as it is clear where the error is from based on the error message. But since we rely on the openapi generated dart client for http calls as well, a stack trace can be used to catch errors in the auto generated code though
Update ImmichLogger to use the error from the record.
I'll see what I can do, I think this should be the first step. I've never worked with Dart and all the frameworks before, so I'll have to see what is generated by what, how to change the database layout and add migrations (for the app database) etc. Any pointers to docs for these things would be helpful. Keywords for the required steps are sufficient, I can work out the details myself.
Ended up being easier than I thought (unless I missed something). 😄 Just had to add the new column to mobile/lib/shared/models/logger_message.model.dart and run flutter pub run build_runner build to regenerate the accompanying .g.dart file.
@rovo89 Do you still plan to work on this or can we close this PR so you can open one later?
I'll close the PR for now as I'm busy with other things. When I have more time, I'll send a new one or reopen this one.