Skip to content

Add console URL logging. #2632

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 16 commits into from
May 18, 2021
Merged

Add console URL logging. #2632

merged 16 commits into from
May 18, 2021

Conversation

jeremyjiang-dev
Copy link
Contributor

@jeremyjiang-dev jeremyjiang-dev commented Apr 29, 2021 •
edited
Loading

See go/fireperf-logging-console-links-on-client Option 6 Log the URL for every trace metrics

@google-cla google-cla bot added the cla: yes Override cla label Apr 29, 2021
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Apr 29, 2021 •
edited
Loading

Coverage Report

Affected SDKs

No changes between base commit (4bc47df) and head commit (bae2c583).

Test Logs

Notes

HTML coverage reports can be produced locally with ./gradlew <product>:checkCoverage.
Report files are located at <product-build-dir>/reports/jacoco/.

Head commit (bae2c583) is created by Prow via merging commits: 4bc47df f4c3ecd.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Apr 29, 2021 •
edited
Loading

Binary Size Report

Affected SDKs

  • firebase-perf

    Type Base (4bc47df) Head (bae2c583) Diff
    aar 296 kB 298 kB +1.45 kB (+0.5%)
    apk (aggressive) 968 kB 969 kB +976 B (+0.1%)
    apk (release) 2.43 MB 2.43 MB +388 B (+0.0%)

Test Logs

Notes

Head commit (bae2c583) is created by Prow via merging commits: 4bc47df f4c3ecd.

@jeremyjiang-dev
Copy link
Contributor Author

/test check-changed

@jeremyjiang-dev jeremyjiang-dev changed the title Add console URL logging for unique trace metrics. Add console URL logging for trace metrics. May 3, 2021
@jeremyjiang-dev jeremyjiang-dev requested a review from visumickey May 4, 2021 15:14
@jeremyjiang-dev jeremyjiang-dev changed the title Add console URL logging for trace metrics. Add console URL logging. May 5, 2021
visumickey
visumickey reviewed May 6, 2021
View reviewed changes
@jeremyjiang-dev jeremyjiang-dev mentioned this pull request May 7, 2021
Merged
@jeremyjiang-dev
Copy link
Contributor Author

/test check-changed

jposuna
jposuna reviewed May 14, 2021
View reviewed changes
firebase-perf/src/main/java/com/google/firebase/perf/FirebasePerformance.java Outdated
@@ -191,6 +193,14 @@ public static FirebasePerformance getInstance() {
gaugeManager.setApplicationContext(appContext);

mPerformanceCollectionForceEnabledState = configResolver.getIsPerformanceCollectionEnabled();
if (isPerformanceCollectionEnabled()) {
Log.i(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you use the AndroidLogger instead of exposing the LogWrapper? It has the added advantage that it already does the check as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose is to log regardless of isLogcatEnabled defined in AndroidLogger. We want to log the console URL if FirebasePerformance is initialized per run.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation. If isLogcatEnabled = false, wouldn't logging go against the user's expectation / desires?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. This decision is made based on the fact that not many developers will turn on the isLogcatEnabled and the flag is off by default. As a result, we would like to log only the dashboard URL only once per run. Happy to hear your thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jeremyjiang-dev How are we logging in the rest of the Firebase codebase? I would align with that. I would not recommend to supersede the choice of the developer to show/hide console logs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would not recommend to supersede the choice of the developer to show/hide console logs

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sg. I should have highlighted this in the design doc to catch attention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@jeremyjiang-dev jeremyjiang-dev requested a review from jposuna May 17, 2021 20:54
jposuna
jposuna approved these changes May 17, 2021
View reviewed changes
firebase-perf/src/main/java/com/google/firebase/perf/FirebasePerformance.java Outdated
@@ -191,6 +193,14 @@ public static FirebasePerformance getInstance() {
gaugeManager.setApplicationContext(appContext);

mPerformanceCollectionForceEnabledState = configResolver.getIsPerformanceCollectionEnabled();
if (isPerformanceCollectionEnabled()) {
Log.i(
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not recommend to supersede the choice of the developer to show/hide console logs

+1

@jeremyjiang-dev jeremyjiang-dev merged commit 0f04810 into master May 18, 2021
@jeremyjiang-dev jeremyjiang-dev deleted the perfConsoleURLUnique branch May 18, 2021 16:57
@firebase firebase locked and limited conversation to collaborators Jun 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@visumickey visumickey visumickey approved these changes

@jposuna jposuna jposuna approved these changes

Assignees
No one assigned
Labels
cla: yes Override cla size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jeremyjiang-dev @google-oss-bot @jposuna @visumickey