Skip to content

Enable global custom attributes on Network Requests #3399

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 3 commits into from
Feb 4, 2022
Merged

Enable global custom attributes on Network Requests #3399

merged 3 commits into from
Feb 4, 2022

Conversation

visumickey
Copy link
Contributor

No description provided.

@visumickey visumickey requested a review from jposuna February 3, 2022 22:00
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Feb 3, 2022 •
edited
Loading

Coverage Report 1

Affected Products

  • firebase-perf

    Overall coverage changed from ? (5bbd6be) to 70.79% (e9a7657) by ?.

    97 individual files with coverage change

    FilenameBase (5bbd6be)Merge (e9a7657)Diff
    AddTrace.java?0.00%?
    AndroidApplicationInfo.java?34.71%?
    AndroidApplicationInfoOrBuilder.java?0.00%?
    AndroidLogger.java?100.00%?
    AndroidMemoryReading.java?38.36%?
    AndroidMemoryReadingOrBuilder.java?0.00%?
    ApplicationInfo.java?45.00%?
    ApplicationInfoOrBuilder.java?0.00%?
    ApplicationProcessState.java?73.91%?
    AppStartTrace.java?86.54%?
    AppStateMonitor.java?86.63%?
    AppStateUpdateHandler.java?92.59%?
    Clock.java?100.00%?
    ConfigResolver.java?97.23%?
    ConfigurationConstants.java?99.21%?
    ConfigurationFlag.java?100.00%?
    ConsoleUrlGenerator.java?37.50%?
    Constants.java?95.65%?
    Counter.java?90.91%?
    CpuGaugeCollector.java?92.77%?
    CpuMetricReading.java?39.33%?
    CpuMetricReadingOrBuilder.java?0.00%?
    DaggerFirebasePerformanceComponent.java?100.00%?
    DeviceCacheManager.java?76.03%?
    FirebasePerfApplicationInfoValidator.java?92.86%?
    FirebasePerfGaugeMetricValidator.java?100.00%?
    FirebasePerfHttpClient.java?93.85%?
    FirebasePerfMetricProto.java?0.00%?
    FirebasePerfNetworkValidator.java?86.67%?
    FirebasePerfOkHttpClient.java?44.90%?
    FirebasePerformance.java?79.57%?
    FirebasePerformanceAttributable.java?0.00%?
    FirebasePerformanceComponent.java?0.00%?
    FirebasePerformanceInitializer.java?33.33%?
    FirebasePerformanceModule.java?100.00%?
    FirebasePerformanceModule_ProvidesConfigResolverFactory.java?100.00%?
    FirebasePerformanceModule_ProvidesFirebaseAppFactory.java?100.00%?
    FirebasePerformanceModule_ProvidesFirebaseInstallationsFactory.java?100.00%?
    FirebasePerformanceModule_ProvidesRemoteConfigComponentFactory.java?100.00%?
    FirebasePerformanceModule_ProvidesRemoteConfigManagerFactory.java?100.00%?
    FirebasePerformanceModule_ProvidesSessionManagerFactory.java?100.00%?
    FirebasePerformanceModule_ProvidesTransportFactoryProviderFactory.java?100.00%?
    FirebasePerformance_Factory.java?100.00%?
    FirebasePerfProvider.java?76.92%?
    FirebasePerfRegistrar.java?100.00%?
    FirebasePerfTraceValidator.java?89.01%?
    FirebasePerfUrlConnection.java?44.26%?
    FlgTransport.java?83.33%?
    GaugeManager.java?98.43%?
    GaugeMetadata.java?32.21%?
    GaugeMetadataManager.java?84.21%?
    GaugeMetadataOrBuilder.java?0.00%?
    GaugeMetric.java?39.47%?
    GaugeMetricOrBuilder.java?0.00%?
    HttpMetric.java?91.78%?
    ImmutableBundle.java?100.00%?
    InstrHttpInputStream.java?92.86%?
    InstrHttpOutputStream.java?98.00%?
    InstrHttpsURLConnection.java?94.32%?
    InstrHttpURLConnection.java?93.42%?
    InstrumentApacheHttpResponseHandler.java?100.00%?
    InstrumentOkHttpEnqueueCallback.java?100.00%?
    InstrURLConnectionBase.java?95.24%?
    LogWrapper.java?23.08%?
    MemoryGaugeCollector.java?91.38%?
    NetworkConnectionInfo.java?0.00%?
    NetworkConnectionInfoOrBuilder.java?0.00%?
    NetworkRequestMetric.java?49.16%?
    NetworkRequestMetricBuilder.java?95.97%?
    NetworkRequestMetricBuilderUtil.java?75.00%?
    NetworkRequestMetricOrBuilder.java?0.00%?
    Optional.java?86.67%?
    PendingPerfEvent.java?100.00%?
    PerfMetric.java?33.67%?
    PerfMetricOrBuilder.java?0.00%?
    PerfMetricValidator.java?90.32%?
    PerfSession.java?93.22%?
    PerfSessionOrBuilder.java?0.00%?
    Rate.java?100.00%?
    RateLimiter.java?89.83%?
    RemoteConfigManager.java?92.86%?
    ResourceType.java?0.00%?
    SessionAwareObject.java?0.00%?
    SessionManager.java?100.00%?
    SessionVerbosity.java?68.42%?
    StorageUnit.java?57.89%?
    Timer.java?93.75%?
    Trace.java?96.69%?
    TraceMetric.java?43.42%?
    TraceMetricBuilder.java?100.00%?
    TraceMetricOrBuilder.java?0.00%?
    TransportInfo.java?0.00%?
    TransportInfoOrBuilder.java?0.00%?
    TransportManager.java?93.95%?
    URLAllowlist.java?94.44%?
    URLWrapper.java?0.00%?
    Utils.java?78.57%?

Test Logs

Notes

  • Commit (e9a7657) is created by Prow via merging PR base commit (5bbd6be) and head commit (d8b77fc).
  • 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/8lO4Bg7e8j.html

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Feb 3, 2022 •
edited
Loading

Size Report 1

Affected Products

  • base

    TypeBase (5bbd6be)Merge (e9a7657)Diff
    apk (aggressive)?8.39 kB? (?)
    apk (release)?8.65 kB? (?)

  • firebase-abt

    TypeBase (5bbd6be)Merge (e9a7657)Diff
    aar?13.7 kB? (?)
    apk (aggressive)?80.9 kB? (?)
    apk (release)?642 kB? (?)

  • firebase-annotations

    TypeBase (5bbd6be)Merge (e9a7657)Diff
    apk (aggressive)?8.39 kB? (?)
    apk (release)?8.83 kB? (?)

  • firebase-common

    TypeBase (5bbd6be)Merge (e9a7657)Diff
    aar?47.5 kB? (?)
    apk (aggressive)?80.3 kB? (?)
    apk (release)?637 kB? (?)

  • firebase-components

    TypeBase (5bbd6be)Merge (e9a7657)Diff
    aar?41.4 kB? (?)
    apk (aggressive)?8.68 kB? (?)
    apk (release)?29.5 kB? (?)

  • firebase-config

    TypeBase (5bbd6be)Merge (e9a7657)Diff
    aar?62.7 kB? (?)
    apk (aggressive)?92.9 kB? (?)
    apk (release)?720 kB? (?)

  • firebase-datatransport

    TypeBase (5bbd6be)Merge (e9a7657)Diff
    aar?4.83 kB? (?)
    apk (aggressive)?126 kB? (?)
    apk (release)?723 kB? (?)

  • firebase-encoders

    TypeBase (5bbd6be)Merge (e9a7657)Diff
    apk (aggressive)?8.68 kB? (?)
    apk (release)?15.3 kB? (?)

  • firebase-encoders-json

    TypeBase (5bbd6be)Merge (e9a7657)Diff
    aar?10.5 kB? (?)
    apk (aggressive)?8.68 kB? (?)
    apk (release)?20.1 kB? (?)

  • firebase-encoders-proto

    TypeBase (5bbd6be)Merge (e9a7657)Diff
    apk (aggressive)?8.68 kB? (?)
    apk (release)?21.6 kB? (?)

  • firebase-installations

    TypeBase (5bbd6be)Merge (e9a7657)Diff
    aar?54.7 kB? (?)
    apk (aggressive)?81.8 kB? (?)
    apk (release)?658 kB? (?)

  • firebase-installations-interop

    TypeBase (5bbd6be)Merge (e9a7657)Diff
    aar?8.06 kB? (?)
    apk (aggressive)?55.4 kB? (?)
    apk (release)?607 kB? (?)

  • firebase-perf

    TypeBase (5bbd6be)Merge (e9a7657)Diff
    aar?301 kB? (?)
    apk (aggressive)?982 kB? (?)
    apk (release)?2.45 MB? (?)

  • protolite-well-known-types

    TypeBase (5bbd6be)Merge (e9a7657)Diff
    aar?993 kB? (?)
    apk (aggressive)?134 kB? (?)
    apk (release)?661 kB? (?)

  • transport-api

    TypeBase (5bbd6be)Merge (e9a7657)Diff
    aar?6.57 kB? (?)
    apk (aggressive)?8.68 kB? (?)
    apk (release)?14.9 kB? (?)

  • transport-backend-cct

    TypeBase (5bbd6be)Merge (e9a7657)Diff
    aar?53.5 kB? (?)
    apk (aggressive)?58.0 kB? (?)
    apk (release)?105 kB? (?)

  • transport-runtime

    TypeBase (5bbd6be)Merge (e9a7657)Diff
    aar?178 kB? (?)
    apk (aggressive)?43.8 kB? (?)
    apk (release)?82.6 kB? (?)

Test Logs

Notes

  • Commit (e9a7657) is created by Prow via merging PR base commit (5bbd6be) and head commit (d8b77fc).

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

jposuna
jposuna approved these changes Feb 3, 2022
View reviewed changes
Copy link
Contributor

@jposuna jposuna left a comment

Choose a reason for hiding this comment

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

The changes look good. Do you think there's any danger to adding this to network requests? (I.e. why was it not added to network requests from the beginning?)

@visumickey
Copy link
Contributor Author

AFAICT - I don't see any risk in doing this. I spent very less time to debug and make these changes. But I spent a way longer time to understand why this mismatch did happen - and I could not find any clear reason. So, I assume this was an oversight rather than anything else.

jeremyjiang-dev
jeremyjiang-dev approved these changes Feb 4, 2022
View reviewed changes
Copy link
Contributor

@jeremyjiang-dev jeremyjiang-dev left a comment

Choose a reason for hiding this comment

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

The changes LGTM. Have you checked the e2e experience on console? I know this PR is a bit urgent. We need to update docs and api reference later.

@jeremyjiang-dev
Copy link
Contributor

Could you also update CHANGELOG?

@visumickey
Copy link
Contributor Author

Could you also update CHANGELOG?

Done!

@visumickey visumickey merged commit 39c062b into master Feb 4, 2022
@visumickey visumickey deleted the perfGlobalCustomAttriubtesNetwork branch February 4, 2022 20:02
@firebase firebase locked and limited conversation to collaborators Mar 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jposuna jposuna jposuna approved these changes

@leotianlizhan leotianlizhan leotianlizhan approved these changes

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

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

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