Skip to content

Fireperf fragments: sampling #3588

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 7 commits into from
Mar 29, 2022
Merged

Fireperf fragments: sampling #3588

merged 7 commits into from
Mar 29, 2022

Conversation

leotianlizhan
Copy link
Contributor

b/225186908

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Mar 24, 2022 •
edited
Loading

Coverage Report 1

Affected Products

  • firebase-perf

    Overall coverage changed from 71.30% (84bb05b) to 71.40% (12ea5c5) by +0.10%.

    FilenameBase (84bb05b)Merge (12ea5c5)Diff
    RateLimiter.java89.83%90.98%+1.15%
    TraceMetric.java43.42%43.98%+0.56%

Test Logs

Notes

  • Commit (12ea5c5) is created by Prow via merging PR base commit (84bb05b) and head commit (6efc6e3).
  • 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/Tc1v1tOofg.html

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Mar 24, 2022 •
edited
Loading

Size Report 1

Affected Products

  • firebase-perf

    TypeBase (84bb05b)Merge (12ea5c5)Diff
    aar307 kB307 kB+369 B (+0.1%)
    apk (aggressive)1.03 MB1.03 MB+136 B (+0.0%)
    apk (release)2.47 MB2.47 MB+784 B (+0.0%)

Test Logs

Notes

  • Commit (12ea5c5) is created by Prow via merging PR base commit (84bb05b) and head commit (6efc6e3).

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

@leotianlizhan leotianlizhan marked this pull request as ready for review March 29, 2022 01:19
@jeremyjiang-dev
Copy link
Contributor

LGTM on check-changed is green

@jeremyjiang-dev jeremyjiang-dev self-requested a review March 29, 2022 21:02
visumickey
visumickey approved these changes Mar 29, 2022
View reviewed changes
Copy link
Contributor

@visumickey visumickey left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM after addressing the unit test comments.

I'm not fully aligned on adding a new parameter to the RateLimiter constructor. This means that for every new metric, we would end up adding a new sampling rate parameter to the rate limiter. But, we are aligning our current implementation with the existing options. So, fine with it for now. But we need to find a better strategy to organize the rate limiter.

@leotianlizhan leotianlizhan merged commit 7472e2e into perfFragmentsEAP Mar 29, 2022
@leotianlizhan leotianlizhan deleted the fragment-sampling branch March 29, 2022 21:57
leotianlizhan added a commit that referenced this pull request Apr 7, 2022
@jeremyjiang-dev @visumickey @leotianlizhan
Fireperf: Add Fragment Screen Performance Support (#3591)
26c905b 
* Add fragment trace sampling rate config flag (#3546)

* Fireperf: fragment lifecycle callbacks (#3565)

* onResume and onPause

* copyright

* rename to

* more specific language

* Fireperf fragments: trace creation and adding custom attributes (#3575)

* implementation

* test

* gjf

* ebugfix

* copyright

* add tests

* long name test

* fix test

* change error to warn message

* Fix hasFrameMetricsAggregator's value not being set.

* Fix googleJavaFormat error

* Add frame metrics to fragment traces (#3592)

* Add frame metrics to fragment traces.

* Fix AppStateMonitor.java

* Rename FrameMetrics to PerfFrameMetrics

* Fix screen trace logging by printing the trace name. (#3599)

* Fireperf fragments: sampling (#3588)

* ssample fragment after trace already sampled

* gjf

* tests and qol for immutable bundle

* test names and revert getFloat

* fragment-sampling

* fix test

* separate bucketId checkArguments

* Fireperf fragments eap: change version number (#3604)

* change version number

* changelog

* changelog edit

* Update the gradle properties to match the EAP release branch (#3606)

* Update the gradle properties to match the release branch

* Revert ktx gradle properties

* default no0 sampling rate

* gradle.properties revert

* gradle.properties revert 2

* fix unit tests

* review

Co-authored-by: Visu <[email protected]>
Co-authored-by: Leo Zhan <[email protected]>
@firebase firebase locked and limited conversation to collaborators Apr 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@visumickey visumickey visumickey approved these changes

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

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

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