Skip to content
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.

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

Fix custom trace event dispatch before initialization for Performance Monitoring #3850

Merged
merged 7 commits into from
Oct 1, 2020

Conversation

zijianjoy
Copy link
Contributor

@zijianjoy zijianjoy commented Sep 28, 2020

Developer reported that custom trace right after initializing perf object was not able to be recorded. That is because initialization didn't finish, thus was not able to flip the bit of SettingsService.loggingEnabled from false to true. Therefore custom trace was discarded.

Fix

Check SettingsService.loggingEnabled after initialization finishes, thus avoid dropping events before reading RemoteConfig value.

Internal tracking bug: b/169126072

@changeset-bot
Copy link

changeset-bot bot commented Sep 28, 2020

🦋 Changeset detected

Latest commit: 8f7a46a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@firebase/performance Patch
firebase Patch
@firebase/rules-unit-testing Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Sep 28, 2020

Binary Size Report

Affected SDKs

  • @firebase/analytics

    Type Base (a6af7c2) Head (c480f14) Diff
    esm2017 18.4 kB 18.6 kB +224 B (+1.2%)
    main 23.3 kB 23.8 kB +467 B (+2.0%)
    module 22.8 kB 23.3 kB +455 B (+2.0%)
  • @firebase/firestore

    Type Base (a6af7c2) Head (c480f14) Diff
    browser 249 kB 249 kB +77 B (+0.0%)
    esm2017 196 kB 196 kB +78 B (+0.0%)
    main 483 kB 484 kB +165 B (+0.0%)
    module 247 kB 247 kB +77 B (+0.0%)
    react-native 196 kB 197 kB +78 B (+0.0%)
  • @firebase/firestore/exp

    Type Base (a6af7c2) Head (c480f14) Diff
    browser 189 kB 189 kB +78 B (+0.0%)
    main 477 kB 477 kB +169 B (+0.0%)
    module 189 kB 189 kB +78 B (+0.0%)
    react-native 189 kB 189 kB +78 B (+0.0%)
  • @firebase/firestore/lite

    Type Base (a6af7c2) Head (c480f14) Diff
    main 140 kB 140 kB +6 B (+0.0%)
  • @firebase/performance

    Type Base (a6af7c2) Head (c480f14) Diff
    browser 27.4 kB 27.5 kB +57 B (+0.2%)
    esm2017 25.4 kB 25.4 kB +59 B (+0.2%)
    main 27.4 kB 27.5 kB +57 B (+0.2%)
    module 27.1 kB 27.2 kB +57 B (+0.2%)
  • firebase

    Type Base (a6af7c2) Head (c480f14) Diff
    firebase-analytics.js 35.8 kB 35.9 kB +70 B (+0.2%)
    firebase-firestore.js 287 kB 287 kB +77 B (+0.0%)
    firebase-performance-standalone.es2017.js 71.3 kB 71.4 kB +35 B (+0.0%)
    firebase-performance-standalone.js 48.0 kB 48.0 kB +26 B (+0.1%)
    firebase-performance.js 38.4 kB 38.4 kB +26 B (+0.1%)
    firebase.js 831 kB 831 kB +175 B (+0.0%)

Test Logs

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Sep 28, 2020

Size Analysis Report

Affected Products

@firebase/performance-exp

  • getPerformance

    Size

    Type Base (a6af7c2) Head (c480f14) Diff
    size 16.8 kB 16.9 kB +67 B (+0.4%)
    size_with_ext_deps 21.8 kB 33.9 kB +12.0 kB (+55.0%)
  • trace

    Size

    Type Base (a6af7c2) Head (c480f14) Diff
    size 16.7 kB 16.8 kB +67 B (+0.4%)
    size_with_ext_deps 21.7 kB 33.8 kB +12.1 kB (+55.7%)

Test Logs

Copy link
Member

@Feiyang1 Feiyang1 left a comment

Choose a reason for hiding this comment

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

LGTM. Can you please add changeset? Is there a GitHub issue we can reference?

@github-actions
Copy link
Contributor

github-actions bot commented Oct 1, 2020

Changeset File Check ⚠️

Warning: This PR modifies files in the following packages but they have not been included in the changeset file:

  • @firebase/performance-exp

Make sure this was intentional.

@zijianjoy
Copy link
Contributor Author

LGTM. Can you please add changeset? Is there a GitHub issue we can reference?

Thank you! I added a changeset, and the issue comes from internal support flow, Internal tracking bug: b/169126072.

@zijianjoy zijianjoy merged commit 48b0b0f into master Oct 1, 2020
@google-oss-bot google-oss-bot mentioned this pull request Oct 6, 2020
@firebase firebase locked and limited conversation to collaborators Nov 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants