Skip to content

Disable verbose logging in transport-runtime. #3648

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 19, 2022
Merged

Disable verbose logging in transport-runtime. #3648

merged 2 commits into from
Apr 19, 2022

Conversation

vkryachko
Copy link
Member

fixes #3441

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Apr 14, 2022 •
edited
Loading

Coverage Report 1

Affected Products

  • transport-runtime

    Overall coverage changed from 55.44% (11dc9e9) to 55.05% (8792505) by -0.39%.

    FilenameBase (11dc9e9)Merge (8792505)Diff
    Logging.java46.67%37.93%-8.74%
    TransportContext.java100.00%69.23%-30.77%

Test Logs

Notes

  • Commit (8792505) is created by Prow via merging PR base commit (11dc9e9) and head commit (a8c3550).
  • Run gradle <product>:checkCoverage to produce HTML coverage reports locally. After gradle commands finished, report files can be found under <product-build-dir>/reports/jacoco/.

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/5pGDyrV1nY.html

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Apr 14, 2022 •
edited
Loading

Size Report 1

Affected Products

  • transport-backend-cct

    TypeBase (11dc9e9)Merge (8792505)Diff
    aar53.5 kB53.5 kB-33 B (-0.1%)
    apk (aggressive)58.0 kB58.0 kB+60 B (+0.1%)
    apk (release)105 kB105 kB+36 B (+0.0%)

  • transport-runtime

    TypeBase (11dc9e9)Merge (8792505)Diff
    aar178 kB178 kB+136 B (+0.1%)
    apk (aggressive)43.8 kB43.8 kB+72 B (+0.2%)
    apk (release)82.6 kB82.7 kB+100 B (+0.1%)

Test Logs

Notes

  • Commit (8792505) is created by Prow via merging PR base commit (11dc9e9) and head commit (a8c3550).

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/WVrckbguBF.html

@jeremyjiang-dev
Copy link
Contributor

Should we also update the log level for the logging statement that the ticket is mentioning? For example, https://github.com/firebase/firebase-android-sdk/blob/master/transport/transport-runtime/src/main/java/com/google/android/datatransport/runtime/scheduling/jobscheduling/AlarmManagerScheduler.java#L107

@vkryachko
Copy link
Member Author

@jeremyjiang-dev note the change in the implementation of Logging.d(), it now checks if debug level is enabled, which it is not by default, see https://developer.android.com/reference/android/util/Log#isLoggable(java.lang.String,%20int)

@jeremyjiang-dev
Copy link
Contributor

@jeremyjiang-dev note the change in the implementation of Logging.d(), it now checks if debug level is enabled, which it is not by default, see https://developer.android.com/reference/android/util/Log#isLoggable(java.lang.String,%20int)

How should we advise the developer to solve the issue in the ticket. I think the developer needs to set logging level to INFO in order to get rid off the logs in the tickets.

@vkryachko
Copy link
Member Author

@jeremyjiang-dev note the change in the implementation of Logging.d(), it now checks if debug level is enabled, which it is not by default, see https://developer.android.com/reference/android/util/Log#isLoggable(java.lang.String,%20int)

How should we advise the developer to solve the issue in the ticket. I think the developer needs to set logging level to INFO in order to get rid off the logs in the tickets.

As you can see in the docs I linked, info is the default, nothing "lower" than INFO will be logged and has to be explicitly enabled via setprop to be able to see debug messages(which we will do when we need to debug issues going forward while nothing will be logged for our customers by default)

@rlazo
Copy link
Collaborator

rlazo commented Apr 14, 2022

As per the logging documentation , it's possible to strip blocks to isLoggable using proguard rules. If a developer uses this, our wrapper will still be called to create the concatenated TAG (since it's outside the block). Is this something we worry about, performance wise?

rlazo
rlazo approved these changes Apr 14, 2022
View reviewed changes
Copy link
Collaborator

@rlazo rlazo left a comment

Choose a reason for hiding this comment

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

My previous comment shouldn't prevent this change from being merged BTW.

@vkryachko
Copy link
Member Author

As per the logging documentation , it's possible to strip blocks to isLoggable using proguard rules. If a developer uses this, our wrapper will still be called to create the concatenated TAG (since it's outside the block). Is this something we worry about, performance wise?

I mean, that's fair. However those rules are not the default and need to be explicitly added to the app configuration. At which point imo it's reasonable for the developer to deal with the side effects of such changes.

Also, I don't think with the link you provided it would be possible to only remove isLoggable without removing Log.d(), iiuc assumenosideeffects only informs proguard that the call does not produce side effects, however since the result of isLoggable is used in the if statement, proguard would not remove such calls unless all if branches are also determined to have no side effects in them.

@vkryachko
Copy link
Member Author

/retest

@google-oss-bot
Copy link
Contributor

@vkryachko: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
smoke-tests a8c3550 link /test smoke-tests
device-check-changed a8c3550 link /test device-check-changed

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@vkryachko vkryachko merged commit eb7bb1e into master Apr 19, 2022
@vkryachko vkryachko deleted the vk.transport_logging branch April 19, 2022 18:24
gevorg-kopalyan
gevorg-kopalyan reviewed May 11, 2022
View reviewed changes
...ransport-runtime/src/main/java/com/google/android/datatransport/runtime/logging/Logging.java
}

public static void e(String tag, String message, Throwable e) {
Log.e(getTag(tag), message, e);
tag = getTag(tag);
if (Log.isLoggable(tag, Log.ERROR)) {
Copy link

@gevorg-kopalyan gevorg-kopalyan May 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Log.isLoggable will produce an error because sometimes it can exceed the limit of 23 characters
From Documentation
IllegalArgumentException is thrown if the tag.length() > 23 for Nougat (7.0) and prior releases (API <= 25), there is no tag limit of concern after this API level.

@gevorg-kopalyan gevorg-kopalyan mentioned this pull request May 11, 2022
Closed
@firebase firebase locked and limited conversation to collaborators May 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gevorg-kopalyan gevorg-kopalyan gevorg-kopalyan left review comments

@rlazo rlazo rlazo approved these changes

@jeremyjiang-dev jeremyjiang-dev jeremyjiang-dev approved these changes

Assignees
No one assigned
Labels
size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Firebase Performance Monitoring Console/Logcat Spam
5 participants
@vkryachko @google-oss-bot @jeremyjiang-dev @rlazo @gevorg-kopalyan