Guard Android logs #8824

dnfield · 2019-05-02T22:54:35Z

Builds on previous PR adding BuildConfig - guards logging statements outside of debug mode for anything less than warn.

Should not be considered until after #8821

shell/platform/android/io/flutter/view/ResourceExtractor.java

matthew-carroll · 2019-05-03T02:36:19Z

shell/platform/android/io/flutter/view/AccessibilityBridge.java

-                for (SemanticsNode child : childrenInTraversalOrder) {
-                    child.log(childIndent, recursive);
+            if (BuildConfig.DEBUG) {
+                Log.i(TAG,


Doesn't Android already take care of avoiding log statements when built in release mode?

I don't know if it can because this is a library. Either way, the linter is telling us we should be guarding this this way.

(specifically for logging levels below warning)

Is there someone who could clarify whether we need this in actuality? I've got a lot of logs in the embedding and unfortunately this is quite a tedious practice per log statement. I would definitely appreciate if we could determine with confidence what the behavior is and ideally avoid this. Another option might be to introduce our own Log class that internally contains these guards.

Only needed for log levels below warn. Linter clarifies it, right?

Plus we don't want to be adding more noise to logcat for informational logging outside of debug mode do we?

The documentation says that verbose logs aren't compiled into release build, and debug logs are but are stripped at runtime. INFO statements are higher than both so would always be included, even in regular apps. I'm having trouble finding info on if this automatic stripping is different for libraries vs apps. This page on releasing your app says to manually remove Log calls, so seems like there's conflicting info floating around. I know proguard could handle this too, but we're not using it.

We could do a quick check to confirm that just logs out at every level when the engine initializes and check what gets printed with flutter run --release?

+1 to our own Log class wrapper instead of needing to guard at every call site though.

I'm not strongly opposed to that, but I think we should generally avoid informational logging in the embedder outside of temporary logs for local debugging. Creating a utility class could encourage people to add logging that we don't need.

IOW, informational logging that remains in this codebase should be painful. We only have a few callsites (some of which are already in helper classes or methods), and even of those I think two of them should be upgraded to errors or warnings.

(I say this in part because logcat is already super chatty, and we should avoid adding to chatter when you're looking for flutter specific logs)

Understood. I think requiring the callsite checks makes sense then.

shell/platform/android/io/flutter/plugin/common/ErrorLogResult.java

mklim · 2019-05-03T16:54:51Z

shell/platform/android/io/flutter/view/AccessibilityBridge.java

-                for (SemanticsNode child : childrenInTraversalOrder) {
-                    child.log(childIndent, recursive);
+            if (BuildConfig.DEBUG) {
+                Log.i(TAG,


The documentation says that verbose logs aren't compiled into release build, and debug logs are but are stripped at runtime. INFO statements are higher than both so would always be included, even in regular apps. I'm having trouble finding info on if this automatic stripping is different for libraries vs apps. This page on releasing your app says to manually remove Log calls, so seems like there's conflicting info floating around. I know proguard could handle this too, but we're not using it.

We could do a quick check to confirm that just logs out at every level when the engine initializes and check what gets printed with flutter run --release?

+1 to our own Log class wrapper instead of needing to guard at every call site though.

mklim

LGTM

mklim · 2019-05-03T17:31:30Z

shell/platform/android/io/flutter/view/AccessibilityBridge.java

-                for (SemanticsNode child : childrenInTraversalOrder) {
-                    child.log(childIndent, recursive);
+            if (BuildConfig.DEBUG) {
+                Log.i(TAG,


Understood. I think requiring the callsite checks makes sense then.

matthew-carroll · 2019-05-03T18:26:39Z

If d() and v() logs don't require wrapping then LGTM. If they do require wrapping then I'd like to see a custom Log implementation.

dnfield · 2019-05-03T18:31:57Z

Log.d, Log.v, and Log.i are all priority levels below warn. They require wrapping. I fail to see why they should ever be in a release build, and I fail to see why the codebase should contain them outside of exceptional circumstances (such as locally doing debugging). I'm concerned that a utility class will encourage developers to leave debugging logs that don't really belong.

Log.w, Log.e, and Log.wtf are priority levels that do not require wrapping.

Perhaps we can discuss offline.

matthew-carroll · 2019-05-03T18:34:54Z

Yeah let's discuss offline. Developers would have to go through rounds of adding logs to debug, and then removing them all after debugging, and then doing it all over again the next time. I can't see why that's a good use of anyone's time. As I said, I have numerous logs in the embedding to track lifecycle events and other things. So let's chat in person if you really think those logs shouldn't be there.

dnfield · 2019-05-03T21:32:12Z

For whateer reason, the linter is just fine with the logs in other places in the embedding. I'm not sure if that's a bug or not in the linter, but I think it's good enough for now to just guard these instances and leave it at that. Going to merge this.

flutter/engine@8811392...2c9e37c git log 8811392..2c9e37c --no-merges --oneline 2c9e37c Cause crash in FlutterJNI if invoked on non-main thread (#31263). (flutter/engine#8830) 7ce2666 Guard Android logs (flutter/engine#8824) 8584da2 Roll src/third_party/skia dc19391eef52..70aab823547a (8 commits) (flutter/engine#8836) 1cdc163 Roll src/third_party/dart 1577b95c93..dbe80f3397 (43 commits) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff (amirha@google.com), and stop the roller if necessary.

dnfield added 5 commits May 2, 2019 14:51

remove asserts

d67a843

remove comment

c31d7a6

format

9af3fa5

Catch a few more missing asserts, update baseline

43db149

Guard logging calls using BuildConfig

2b36e51

googlebot added the cla: yes label May 2, 2019

missing imports

a4200ac

dnfield mentioned this pull request May 2, 2019

Remove static leaks #8825

Merged

merge

4c27404

dnfield marked this pull request as ready for review May 3, 2019 02:06

dnfield requested review from matthew-carroll and mklim May 3, 2019 02:06

dnfield force-pushed the guard_log branch from e0433b0 to d4ce6a6 Compare May 3, 2019 02:09

cleanup

9e08955

dnfield force-pushed the guard_log branch from d4ce6a6 to 9e08955 Compare May 3, 2019 02:10

dnfield requested a review from sbaranov May 3, 2019 02:10

dnfield changed the title ~~Guard logs~~ Guard Android logs May 3, 2019

dnfield commented May 3, 2019

View reviewed changes

shell/platform/android/io/flutter/view/ResourceExtractor.java Show resolved Hide resolved

dnfield commented May 3, 2019

View reviewed changes

shell/platform/android/io/flutter/view/ResourceExtractor.java Show resolved Hide resolved

matthew-carroll reviewed May 3, 2019

View reviewed changes

stray import

f66638e

mklim reviewed May 3, 2019

View reviewed changes

mklim approved these changes May 3, 2019

View reviewed changes

dnfield merged commit 7ce2666 into flutter:master May 3, 2019

dnfield deleted the guard_log branch May 3, 2019 21:32

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 3, 2019

7ce2666 Guard Android logs (flutter/engine#8824)

4d5f185

engine-flutter-autoroll mentioned this pull request May 3, 2019

Roll engine 88113925e3a0..7ce2666e54cb (3 commits) flutter/flutter#32065

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 3, 2019

7ce2666 Guard Android logs (flutter/engine#8824)

ed84e3f

engine-flutter-autoroll mentioned this pull request May 3, 2019

Roll engine 88113925e3a0..2c9e37c34e79 (4 commits) flutter/flutter#32069

Merged

Guard Android logs #8824

Guard Android logs #8824

Uh oh!

Conversation

dnfield commented May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mklim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthew-carroll commented May 3, 2019

Uh oh!

dnfield commented May 3, 2019

Uh oh!

matthew-carroll commented May 3, 2019

Uh oh!

dnfield commented May 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dnfield commented May 2, 2019 •

edited

Loading