#13899 fixed readme #13900
#13899 fixed readme #13900
Conversation
Codecov Report
@@ Coverage Diff @@
## master #13900 +/- ##
=========================================
Coverage 29.82% 29.82%
- Complexity 1115 1117 +2
=========================================
Files 422 422
Lines 17172 17172
Branches 2229 2229
=========================================
Hits 5121 5121
- Misses 11676 11677 +1
+ Partials 375 374 -1
Continue to review full report at Codecov.
|
README.md
Outdated
| - Recommendation: use a debuggable variant (see "local.properties helpers" below) with local Leanplum, Adjust, & Sentry API tokens: contact the front-end perf group for access to them | ||
| The only debuggable variant at the moment is the debug variant. However, as highlighted in the guide linked above, there are a few options when using a non-debuggable build. |
There was a problem hiding this comment.
| - Recommendation: use a debuggable variant (see "local.properties helpers" below) with local Leanplum, Adjust, & Sentry API tokens: contact the front-end perf group for access to them | |
| The only debuggable variant at the moment is the debug variant. However, as highlighted in the guide linked above, there are a few options when using a non-debuggable build. | |
| - Recommendation: use a debuggable variant (see "local.properties helpers" below) with local Leanplum, Adjust, & Sentry API tokens: contact the front-end perf group for access to them. The only debuggable variant at the moment is the debug variant. However, as highlighted in the guide linked above, there are a few options when using a non-debuggable build. |
MarcLeclair
force-pushed
the
issue_13899_add_perf_link
branch
from
c0ca760
to
07f27b8
Compare
README.md
Outdated
| If you want to analyze performance during **local development** (note: there is a non-trivial performance impact - see caveats): | ||
| - Recommendation: use a debuggable variant (see "local.properties helpers" below) with local Leanplum, Adjust, & Sentry API tokens: contact the front-end perf group for access to them | ||
| - Recommendation: use a debuggable variant (see "local.properties helpers" below) with local Leanplum, Adjust, & Sentry API tokens: contact the front-end perf group for access to them. The only debuggable variant at the moment is the debug variant. However, as highlighted in the guide linked above, there are a few options when using a non-debuggable build. |
There was a problem hiding this comment.
We don't recommend debuggable variants anymore
README.md
Outdated
| @@ -105,8 +105,10 @@ you want these variants to be: | |||
| #### Performance Build Variants | |||
| For accurate performance measurements, read this section! | |||
|
|
|||
| Before heading into performance, read this [quick guide on performance with Fenix](https://wiki.mozilla.org/Performance/How_to_get_started_on_Fenix) | |||
|
|
|||
| If you want to analyze performance during **local development** (note: there is a non-trivial performance impact - see caveats): | |||
There was a problem hiding this comment.
Can we make this whole section friendlier and less nuanced with technical details, like we did with the performance guide? Some specific thoughts:
- we can mention Sentry, etc. as an afterthought
- Having "Recommendation", "Rationale", "Caveats" is really not friendly language :)
- We don't need to mention experiment opt-in yet
- We don't need to tell them it's slower to build
etc. etc.
MarcLeclair
force-pushed
the
issue_13899_add_perf_link
branch
from
07f27b8
to
9c9ebe7
Compare
README.md
Outdated
| - This is slower to build than debug builds because it does additional tasks (e.g. minification) similar to other release builds | ||
| Before heading into performance, read this [quick guide on performance with Fenix](https://wiki.mozilla.org/Performance/How_to_get_started_on_Fenix) | ||
|
|
||
| If you want to analyze performance during **local development** : |
There was a problem hiding this comment.
I don't think we should even tell folks about debuggable yet – I think we should let the profiler docs mention it.
I feel like they should come away from our documentation with, "Use a production variant (or released APK) with Firefox Profiler" as the first step and every option we put in their way before that is going to make it confusing and more likely that they'll put off doing performance
Once they get the Firefox Profiler working and they find out something is missing, they can look for alternative solutions.
README.md
Outdated
|
|
||
| If you want to run **performance tests/benchmarks** in automation or locally: | ||
| - Recommendation: production builds. If debuggable is required, use recommendation above but note the caveat above. If your needs are not met, please contact the front-end perf group to identify a new solution. | ||
| - Rationale: like the rationale above, we need release variants so the choice is based on the debuggable flag. | ||
| - Use production build since it is much closer in behavior compared to what users see in the wild. | ||
|
|
||
| For additional context on these recommendations, see [the perf build variant analysis](https://docs.google.com/document/d/1aW-m0HYncTDDiRz_2x6EjcYkjBpL9SHhhYix13Vil30/edit#). |
There was a problem hiding this comment.
Let's remove this, it's probably out of date
README.md
Outdated
|
|
||
| For additional context on these recommendations, see [the perf build variant analysis](https://docs.google.com/document/d/1aW-m0HYncTDDiRz_2x6EjcYkjBpL9SHhhYix13Vil30/edit#). | ||
|
|
||
| Before you can install any release variants, **you will need to sign them:** see [Automatically signing release builds](#automatically-sign-release-builds) for details. | ||
|
|
||
| **known disabled-by-default features** |
There was a problem hiding this comment.
Using a section header: ##### (5 seems appropriate)
README.md
Outdated
|
|
||
| For additional context on these recommendations, see [the perf build variant analysis](https://docs.google.com/document/d/1aW-m0HYncTDDiRz_2x6EjcYkjBpL9SHhhYix13Vil30/edit#). | ||
|
|
||
| Before you can install any release variants, **you will need to sign them:** see [Automatically signing release builds](#automatically-sign-release-builds) for details. | ||
|
|
||
| **known disabled-by-default features** | ||
| In production builds, Fenix will use some features that are automatically turned off in debug builds ( A good example of that is telemetry). This may cause some slight difference between productions and debuggable builds, but for most use cases, this should be negligible. |
There was a problem hiding this comment.
I think you should start by telling devs why they should care that things are disabled, e.g. "Some features are disabled by default; this can be problematic when performance testing because you want to see how your code interacts with it. The known features that are disabled-by-default are Sentry, Leanplum, ... For debug builds only, telemetry is disabled by default."
README.md
Outdated
|
|
||
| If you want to run **performance tests/benchmarks** in automation or locally: | ||
| - Recommendation: production builds. If debuggable is required, use recommendation above but note the caveat above. If your needs are not met, please contact the front-end perf group to identify a new solution. | ||
| - Rationale: like the rationale above, we need release variants so the choice is based on the debuggable flag. | ||
| - Use production build since it is much closer in behavior compared to what users see in the wild. | ||
|
|
||
| For additional context on these recommendations, see [the perf build variant analysis](https://docs.google.com/document/d/1aW-m0HYncTDDiRz_2x6EjcYkjBpL9SHhhYix13Vil30/edit#). | ||
|
|
||
| Before you can install any release variants, **you will need to sign them:** see [Automatically signing release builds](#automatically-sign-release-builds) for details. |
There was a problem hiding this comment.
nit: concise this: You will need to sign production build variants: see ...
README.md
Outdated
| - debuggable has a non-trivial & variable impact on performance but is needed to take profiles. | ||
| - Random experiment opt-in & feature flags may impact performance (see [perf-frontend-issues#45](https://github.com/mozilla-mobile/perf-frontend-issues/issues/45) for mitigation). | ||
| - This is slower to build than debug builds because it does additional tasks (e.g. minification) similar to other release builds | ||
| Before heading into performance, read this [quick guide on performance with Fenix](https://wiki.mozilla.org/Performance/How_to_get_started_on_Fenix) |
There was a problem hiding this comment.
If we lead with this, I'm concerned they'll skip the information on build variants. I'd suggest making the build variant guidance very short and then link this. e.g.
Use production variants.
Also, Read this guide for what to do with the build.
Known disabled features
MarcLeclair
force-pushed
the
issue_13899_add_perf_link
branch
from
9c9ebe7
to
b5a9f37
Compare
README.md
Outdated
|
|
||
| #####known disabled-by-default features##### |
There was a problem hiding this comment.
The syntax is ##### Title, no suffix. nit: with space and capital
README.md
Outdated
| The known features that are disabled by defaults are: | ||
| - Sentry (for debug builds only) | ||
| - Leanplum ( for debug builds only) | ||
| - Telemetry ( This one is disabled by defaults) |
There was a problem hiding this comment.
nit: whitespace below (between sections)
README.md
Outdated
| - Random experiment opt-in & feature flags may impact performance (see [perf-frontend-issues#45](https://github.com/mozilla-mobile/perf-frontend-issues/issues/45) for mitigation). | ||
| - This is slower to build than debug builds because it does additional tasks (e.g. minification) similar to other release builds | ||
| To analyze performance during **local development** : | ||
| - Build a production variant locally (this could either be the Nightly, beta or release). Then, use the Firefox profiler to profile what you need! |
There was a problem hiding this comment.
Don't need bullet points for a single item – you should integrate this with the previous line (no need for the colon either)
README.md
Outdated
| - Random experiment opt-in & feature flags may impact performance (see [perf-frontend-issues#45](https://github.com/mozilla-mobile/perf-frontend-issues/issues/45) for mitigation). | ||
| - This is slower to build than debug builds because it does additional tasks (e.g. minification) similar to other release builds | ||
| To analyze performance during **local development** : | ||
| - Build a production variant locally (this could either be the Nightly, beta or release). Then, use the Firefox profiler to profile what you need! |
There was a problem hiding this comment.
They could also use release builds – I feel like we should suggest this too (because it's even easier).
README.md
Outdated
|
|
||
| For additional context on these recommendations, see [the perf build variant analysis](https://docs.google.com/document/d/1aW-m0HYncTDDiRz_2x6EjcYkjBpL9SHhhYix13Vil30/edit#). | ||
| If you want to run **performance tests/benchmarks** in automation or locally: | ||
| - Use production build since it is much closer in behavior compared to what users see in the wild. |
There was a problem hiding this comment.
Same: join this with previous line
README.md
Outdated
| If you want to run **performance tests/benchmarks** in automation or locally: | ||
| - Recommendation: production builds. If debuggable is required, use recommendation above but note the caveat above. If your needs are not met, please contact the front-end perf group to identify a new solution. | ||
| - Rationale: like the rationale above, we need release variants so the choice is based on the debuggable flag. | ||
| For more information on how to use the profiler or how to use the build, refer to this [quick guide on performance with Fenix](https://wiki.mozilla.org/Performance/How_to_get_started_on_Fenix) |
There was a problem hiding this comment.
nit: -> how to measure performance with the build?
README.md
Outdated
|
|
||
| #####known disabled-by-default features##### | ||
| Some features are disabled by defaults when Fenix is built locally. This can be problematic at times for checking performance since you might want to know how your code behaves with those features. |
There was a problem hiding this comment.
nit: -> default
Here and below
README.md
Outdated
| The known features that are disabled by defaults are: | ||
| - Sentry (for debug builds only) | ||
| - Leanplum ( for debug builds only) | ||
| - Telemetry ( This one is disabled by defaults) |
There was a problem hiding this comment.
nit: no whitespace after paren, no caps
Here and above
README.md
Outdated
| #####known disabled-by-default features##### | ||
| Some features are disabled by defaults when Fenix is built locally. This can be problematic at times for checking performance since you might want to know how your code behaves with those features. | ||
| The known features that are disabled by defaults are: | ||
| - Sentry (for debug builds only) |
There was a problem hiding this comment.
It's not just for debug builds – it's dependent on whether or not you have an API key, iirc. Same with Leanplum. I think we should just say:
- Sentry
- Leanplum
- Adjust
- Mozilla Location Service, aka MLS
- Telemetry (only disabled by default in debug builds)
If they need those things, they can figure out how to enable them themselves or ask us
Note: I added a few more services here. I think Firebase push is also disabled so maybe include that if you can quickly validate?
13899 re worded the performance readme and removed the reference to debuggable builds 13899 fixed some nits
MarcLeclair
force-pushed
the
issue_13899_add_perf_link
branch
from
b5a9f37
to
c4a372f
Compare
There was a problem hiding this comment.
lgtm, thanks @MarcLeclair – I hope I didn't bully you and you have agreed with my suggestions! 😅
|
|
||
| ##### Known disabled-by-default features | ||
| Some features are disabled by default when Fenix is built locally. This can be problematic at times for checking performance since you might want to know how your code behaves with those features. | ||
| The known features that are disabled by default are: |
There was a problem hiding this comment.
nit: join with previous line or newline above
Pull Request checklist
After merge
To download an APK when reviewing a PR: