For #10434: Handle cases when proc/$pid/stat is not accessible. #10481

mcarare · 2020-05-07T12:00:46Z

Pull Request checklist

Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

After merge

Milestone: Make sure issues finished by this pull request are added to the milestone of the version currently in development.

To download an APK when reviewing a PR:

click on Show All Checks,
click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
click on the "Fenix - assemble" task, then click "Run Artifacts".
the APK links should be on the left side of the screen, named for each CPU architecture

mcarare · 2020-05-07T12:03:33Z

@mcomella I wasn't sure how we would want to handle exceptions. I thought that a value of 0 will be ignored when assessing this metric. Let me know if this should be handled differently. TY!

mcomella

Thanks for addressing this! I think reporting 0 is misleading in our data so I think we should either not record or record a new error.

app/src/main/java/org/mozilla/fenix/perf/StartupFrameworkStartMeasurement.kt

app/src/main/java/org/mozilla/fenix/perf/Stat.kt

mcomella · 2020-05-12T19:44:22Z

app/src/main/java/org/mozilla/fenix/perf/StartupFrameworkStartMeasurement.kt

+            try {
+                stat.getProcessStartTimeTicks(Process.myPid())
+            } catch (e: NegativeArraySizeException) {
+                return 0


Rather than returning 0, I think it's preferable to report an error through a new telemetry probe.

That being said, if that's too much work, it's probably fine to catch the exception in setExpensiveMetric and record nothing if it's thrown. Just bump the metric version and change the docs to make it explicit this is happening.

@mcomella I added a telemetry probe, going to also post data review form and mark this PR with needs:data-review. Let me know if any more changes are needed. TY!

mcomella · 2020-05-12T19:45:54Z

Sorry it took so long to get to this 😞

mcomella · 2020-05-12T22:25:06Z

Ideally, we could extend the tests to cover this case as well (I think Stat & friends have high code coverage).

mcarare · 2020-05-13T06:24:45Z

Sorry it took so long to get to this 😞

No worries. I know it happens. Thank you for the review!

mcarare · 2020-05-13T07:37:04Z

Request for data collection review form

All questions are mandatory. You must receive review from a data steward peer on your responses to these questions before shipping new data collection.

What questions will you answer with this data?

How often a custom OS will block us from reading stats needed for startup time evaluation?

Why does Mozilla need to answer these questions? Are there benefits for users? Do we need this information to address product or business requirements? Some example responses:

Measure how often our startup time implementation is not suitable for OS customizations.

What alternative methods did you consider to answer these questions? Why were they not sufficient?

We could not gather data locally on a wide range of devices and OS customizations that are not suitable for our startup measurement implementation.

Can current instrumentation answer these questions?

No.

List all proposed measurements and indicate the category of data collection for each measurement, using the Firefox data collection categories found on the Mozilla wiki.

Note that the data steward reviewing your request will characterize your data collection based on the highest (and most sensitive) category.

Measurement Description	Data Collection Category	Tracking Bug #
framework_start_error: event	Category 1	#10434

How long will this data be collected? Choose one of the following:

I want this data to be collected until 2020-07-15 (like the rest of startup telemetry data)

What populations will you measure?

All.

If this data collection is default on, what is the opt-out mechanism for users?

Standard telemetry opt-out.

Please provide a general description of how you will analyze this data.

Watch frameworkStartReadError to determine if there are recurrent read errors.

Where do you intend to share the results of your analysis?

Internally.

Is there a third-party tool (i.e. not Telemetry) that you are proposing to use for this data collection? If so:

No.

mcomella

This looks good to me. If we want to avoid the churn, I'm happy for this to land without the nits too (after data review). Thanks mcarare !

mcomella · 2020-05-13T20:04:51Z

app/src/main/java/org/mozilla/fenix/perf/Stat.kt

@@ -25,6 +25,9 @@ private const val FIELD_POS_STARTTIME = 21 // starttime naming matches field in
 open class Stat {

    @VisibleForTesting(otherwise = PRIVATE)
+    /**
+     * @throws [java.io.FileNotFoundException]
+     */


nit: I think kdoc is conventionally above the annotation

mcomella · 2020-05-13T20:05:53Z

app/src/main/java/org/mozilla/fenix/perf/Stat.kt

@@ -25,6 +25,9 @@ private const val FIELD_POS_STARTTIME = 21 // starttime naming matches field in
 open class Stat {

    @VisibleForTesting(otherwise = PRIVATE)
+    /**
+     * @throws [java.io.FileNotFoundException]
+     */


nit: I'd add a kdoc to getFrameworkStartNanos too. It's more important than this method because it's the non-private method of this class

mcomella · 2020-05-19T17:53:34Z

@boek Please data review.

…essible.

codecov-commenter · 2020-05-25T09:32:35Z

Codecov Report

Merging #10481 into master will decrease coverage by 0.00%.
The diff coverage is 50.00%.

@@             Coverage Diff              @@
##             master   #10481      +/-   ##
============================================
- Coverage     19.63%   19.63%   -0.01%     
  Complexity      631      631              
============================================
  Files           363      363              
  Lines         14950    14953       +3     
  Branches       2017     2017              
============================================
  Hits           2936     2936              
- Misses        11737    11740       +3     
  Partials        277      277

Impacted Files	Coverage Δ	Complexity Δ
app/src/main/java/org/mozilla/fenix/perf/Stat.kt	`87.50% <ø> (ø)`	`5.00 <0.00> (ø)`
...lla/fenix/perf/StartupFrameworkStartMeasurement.kt	`90.00% <50.00%> (-10.00%)`	`9.00 <0.00> (ø)`
...ix/home/sessioncontrol/SessionControlController.kt	`71.42% <0.00%> (-0.90%)`	`0.00% <0.00%> (ø%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c7d860...1a68109. Read the comment docs.

mcomella · 2020-05-30T00:46:55Z

@liuche Please data review.

liuche

Data Review Form (to be filled by Data Stewards)

Is there or will there be documentation that describes the schema for the ultimate data set in a public, complete, and accurate way?

Yes, documented usage in startup-timeline ping in metrics.md

Is there a control mechanism that allows the user to turn the data collection on and off?

Yes, custom pings are controlled by the Fenix data settings

If the request is for permanent data collection, is there someone who will monitor the data over time?

Expires in 7/2020

Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?
Type 1, measuring error frequency when blocked reading from /proc
Is the data collection request for default-on or default-off?

default on

Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)?

No

Is the data collection covered by the existing Firefox privacy notice?

yes

Does there need to be a check-in in the future to determine whether to renew the data? (Yes/No) (If yes, set a todo reminder or file a bug if appropriate)**

No, has expiry

Does the data collection use a third-party collection tool? If yes, escalate to legal.

No

mcarare requested a review from mcomella May 7, 2020 12:02

mcarare force-pushed the 10434 branch from a53cb47 to 27447bc Compare May 7, 2020 12:43

mcomella suggested changes May 12, 2020

View reviewed changes

mcarare force-pushed the 10434 branch 2 times, most recently from 0d6a3e7 to 2725aa8 Compare May 13, 2020 07:27

mcarare added the needs:data-review PR is awaiting a data review label May 13, 2020

mcomella approved these changes May 13, 2020

View reviewed changes

mcarare force-pushed the 10434 branch from 5654eff to 9db426c Compare May 14, 2020 06:02

mcomella requested a review from boek May 19, 2020 17:49

mcarare assigned mcarare and boek and unassigned mcarare May 25, 2020

For mozilla-mobile#10434: Handle cases when proc/$pid/stat is not acc…

1a68109

…essible.

mcarare force-pushed the 10434 branch from 9db426c to 1a68109 Compare May 25, 2020 09:19

mcomella requested a review from liuche May 30, 2020 00:46

liuche approved these changes Jun 3, 2020

View reviewed changes

liuche merged commit 2090b11 into mozilla-mobile:master Jun 3, 2020

mcarare deleted the 10434 branch June 3, 2020 10:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For #10434: Handle cases when proc/$pid/stat is not accessible. #10481

For #10434: Handle cases when proc/$pid/stat is not accessible. #10481

mcarare commented May 7, 2020 •

edited

mcarare commented May 7, 2020

mcomella left a comment

mcomella May 12, 2020

mcarare May 13, 2020

mcomella commented May 12, 2020

mcomella commented May 12, 2020

mcarare commented May 13, 2020

mcarare commented May 13, 2020

mcomella left a comment •

edited

mcomella May 13, 2020

mcomella May 13, 2020

mcomella commented May 19, 2020

codecov-commenter commented May 25, 2020

mcomella commented May 30, 2020

liuche left a comment

For #10434: Handle cases when proc/$pid/stat is not accessible. #10481

For #10434: Handle cases when proc/$pid/stat is not accessible. #10481

Conversation

mcarare commented May 7, 2020 • edited

Pull Request checklist

After merge

To download an APK when reviewing a PR:

mcarare commented May 7, 2020

mcomella left a comment

Choose a reason for hiding this comment

mcomella May 12, 2020

Choose a reason for hiding this comment

mcarare May 13, 2020

Choose a reason for hiding this comment

mcomella commented May 12, 2020

mcomella commented May 12, 2020

mcarare commented May 13, 2020

mcarare commented May 13, 2020

Request for data collection review form

mcomella left a comment • edited

Choose a reason for hiding this comment

mcomella May 13, 2020

Choose a reason for hiding this comment

mcomella May 13, 2020

Choose a reason for hiding this comment

mcomella commented May 19, 2020

codecov-commenter commented May 25, 2020

Codecov Report

mcomella commented May 30, 2020

liuche left a comment

Choose a reason for hiding this comment

Data Review Form (to be filled by Data Stewards)

mcarare commented May 7, 2020 •

edited

mcomella left a comment •

edited