Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Guard Android logs #8824

Merged
dnfield merged 9 commits into from
May 3, 2019
Merged

Guard Android logs #8824

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d67a843
remove asserts
dnfield May 2, 2019
c31d7a6
remove comment
dnfield May 2, 2019
9af3fa5
format
dnfield May 2, 2019
43db149
Catch a few more missing asserts, update baseline
dnfield May 2, 2019
2b36e51
Guard logging calls using BuildConfig
dnfield May 2, 2019
a4200ac
missing imports
dnfield May 2, 2019
4c27404
merge
dnfield May 3, 2019
9e08955
cleanup
dnfield May 3, 2019
f66638e
stray import
dnfield May 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public class FlutterJNI {

@UiThread
public static native String nativeGetObservatoryUri();

private Long nativePlatformViewId;
private FlutterRenderer.RenderSurface renderSurface;
private AccessibilityDelegate accessibilityDelegate;
Expand Down
10 changes: 8 additions & 2 deletions shell/platform/android/io/flutter/plugin/common/ErrorLogResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

package io.flutter.plugin.common;

import io.flutter.BuildConfig;

import android.support.annotation.Nullable;
import android.util.Log;

Expand All @@ -30,11 +32,15 @@ public void success(@Nullable Object result) {}
@Override
public void error(String errorCode, @Nullable String errorMessage, @Nullable Object errorDetails) {
String details = (errorDetails != null) ? " details: " + errorDetails : "";
Log.println(level, tag, errorMessage + details);
if (level >= Log.WARN || BuildConfig.DEBUG) {
Log.println(level, tag, errorMessage + details);
}
}

@Override
public void notImplemented() {
Log.println(level, tag, "method not implemented");
if (level >= Log.WARN || BuildConfig.DEBUG) {
Log.println(level, tag, "method not implemented");
}
}
}
22 changes: 12 additions & 10 deletions shell/platform/android/io/flutter/view/AccessibilityBridge.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1821,16 +1821,18 @@ private boolean didChangeLabel() {
}

private void log(@NonNull String indent, boolean recursive) {
Log.i(TAG,
indent + "SemanticsNode id=" + id + " label=" + label + " actions=" + actions
+ " flags=" + flags + "\n" + indent + " +-- textDirection="
+ textDirection + "\n" + indent + " +-- rect.ltrb=(" + left + ", "
+ top + ", " + right + ", " + bottom + ")\n" + indent
+ " +-- transform=" + Arrays.toString(transform) + "\n");
if (childrenInTraversalOrder != null && recursive) {
String childIndent = indent + " ";
for (SemanticsNode child : childrenInTraversalOrder) {
child.log(childIndent, recursive);
if (BuildConfig.DEBUG) {
Log.i(TAG,
Copy link
Contributor

@matthew-carroll matthew-carroll May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@dnfield dnfield May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dnfield dnfield May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(specifically for logging levels below warning)

Copy link
Contributor

@matthew-carroll matthew-carroll May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dnfield dnfield May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@dnfield dnfield May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@mklim mklim May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dnfield dnfield May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dnfield dnfield May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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)

Copy link
Contributor

@mklim mklim May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

indent + "SemanticsNode id=" + id + " label=" + label + " actions=" + actions
+ " flags=" + flags + "\n" + indent + " +-- textDirection="
+ textDirection + "\n" + indent + " +-- rect.ltrb=(" + left + ", "
+ top + ", " + right + ", " + bottom + ")\n" + indent
+ " +-- transform=" + Arrays.toString(transform) + "\n");
if (childrenInTraversalOrder != null && recursive) {
String childIndent = indent + " ";
for (SemanticsNode child : childrenInTraversalOrder) {
child.log(childIndent, recursive);
}
}
}
}
Expand Down
6 changes: 5 additions & 1 deletion shell/platform/android/io/flutter/view/ResourceCleaner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
import android.os.Handler;
import android.util.Log;

import io.flutter.BuildConfig;

import java.io.File;
import java.io.FilenameFilter;

Expand All @@ -32,7 +34,9 @@ boolean hasFilesToDelete() {

@Override
protected Void doInBackground(Void... unused) {
Log.i(TAG, "Cleaning " + mFilesToDelete.length + " resources.");
if (BuildConfig.DEBUG) {
Log.i(TAG, "Cleaning " + mFilesToDelete.length + " resources.");
}
for (File file : mFilesToDelete) {
if (file.exists()) {
deleteRecursively(file);
Expand Down
18 changes: 12 additions & 6 deletions shell/platform/android/io/flutter/view/ResourceExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,9 +164,9 @@ private boolean extractAPK(File dataDir) {
OutputStream os = new FileOutputStream(output)) {
copy(is, os);
}

Log.i(TAG, "Extracted baseline resource " + resource);

if (BuildConfig.DEBUG) {
Log.i(TAG, "Extracted baseline resource " + resource);
}
} catch (FileNotFoundException fnfe) {
continue;

Expand Down Expand Up @@ -202,17 +202,23 @@ private String checkTimestamp(File dataDir) {
final String[] existingTimestamps = getExistingTimestamps(dataDir);

if (existingTimestamps == null) {
Log.i(TAG, "No extracted resources found");
if (BuildConfig.DEBUG) {
Log.i(TAG, "No extracted resources found");
}
return expectedTimestamp;
}

if (existingTimestamps.length == 1) {
Log.i(TAG, "Found extracted resources " + existingTimestamps[0]);
if (BuildConfig.DEBUG) {
Log.i(TAG, "Found extracted resources " + existingTimestamps[0]);
}
}

if (existingTimestamps.length != 1
|| !expectedTimestamp.equals(existingTimestamps[0])) {
Log.i(TAG, "Resource version mismatch " + expectedTimestamp);
if (BuildConfig.DEBUG) {
Log.i(TAG, "Resource version mismatch " + expectedTimestamp);
}
return expectedTimestamp;
}

Expand Down
2 changes: 2 additions & 0 deletions third_party/bsdiff/io/flutter/util/BSDiff.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

package io.flutter.util;

import io.flutter.BuildConfig;

import java.io.*;
import java.util.zip.GZIPInputStream;

Expand Down
48 changes: 2 additions & 46 deletions tools/android_lint/baseline.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@
errorLine2=" ~~~~~~~~~">
<location
file="../../../flutter/shell/platform/android/io/flutter/view/ResourceCleaner.java"
line="22"
line="24"
column="19"/>
</issue>

Expand All @@ -129,54 +129,10 @@
errorLine2=" ~~~~~~~~~~~">
<location
file="../../../flutter/shell/platform/android/io/flutter/view/ResourceExtractor.java"
line="51"
line="52"
column="19"/>
</issue>

<issue
id="LogConditional"
message="The log call Log.i(...) should be conditional: surround with `if (Log.isLoggable(...))` or `if (BuildConfig.DEBUG) { ... }`"
errorLine1=" Log.i(TAG,"
errorLine2=" ^">
<location
file="../../../flutter/shell/platform/android/io/flutter/view/AccessibilityBridge.java"
line="1824"
column="13"/>
</issue>

<issue
id="LogConditional"
message="The log call Log.println(...) should be conditional: surround with `if (Log.isLoggable(...))` or `if (BuildConfig.DEBUG) { ... }`"
errorLine1=" Log.println(level, tag, errorMessage + details);"
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
<location
file="../../../flutter/shell/platform/android/io/flutter/plugin/common/ErrorLogResult.java"
line="33"
column="9"/>
</issue>

<issue
id="LogConditional"
message="The log call Log.i(...) should be conditional: surround with `if (Log.isLoggable(...))` or `if (BuildConfig.DEBUG) { ... }`"
errorLine1=" Log.i(TAG, &quot;Cleaning &quot; + mFilesToDelete.length + &quot; resources.&quot;);"
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
<location
file="../../../flutter/shell/platform/android/io/flutter/view/ResourceCleaner.java"
line="35"
column="13"/>
</issue>

<issue
id="LogConditional"
message="The log call Log.i(...) should be conditional: surround with `if (Log.isLoggable(...))` or `if (BuildConfig.DEBUG) { ... }`"
errorLine1=" Log.i(TAG, &quot;Extracted baseline resource &quot; + resource);"
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
<location
file="../../../flutter/shell/platform/android/io/flutter/view/ResourceExtractor.java"
line="168"
column="17"/>
</issue>

<issue
id="UseSparseArrays"
message="Use `new SparseArray&lt;SemanticsNode>(...)` instead for better performance"
Expand Down