Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Implement start up type telemetry #18836

Closed
mcomella opened this issue Apr 7, 2021 · 2 comments
Closed

Implement start up type telemetry #18836

mcomella opened this issue Apr 7, 2021 · 2 comments
Assignees
Labels
eng:qa:not-needed Added by QA to issues that cannot be tested needs:triage Issue needs triage performance Possible performance wins

Comments

@mcomella
Copy link
Contributor

mcomella commented Apr 7, 2021

To choose the area of start up perf to focus on, we need to leverage frequency vs. impact, i.e. these variables:

  1. (frequency) What are the most common types of start up?
  2. Does this change for the users with the slowest start ups? (as we assume they are the most likely to change stop using FF)
  3. (impact) Which types of start up are the least performant and by how much? (e.g. if it's real bad, it might outweigh frequency)
  4. Does it change for users with slowest start ups?

To answer 1), we can implement telemetry to count the number of each type of start up. We have existing telemetry that answers this but we've discovered a flaw in its methodology (it doesn't take into account the process starting for Services in COLD start) and it's not easy to analyze (an event with extras doesn't work in GLAM). We can use labeled_counter to implement something we can read in GLAM.

Answering 2) is harder as we need to combine start up type and timing measurements. Let's save that for later.

┆Issue is synchronized with this Jira Task

@mcomella mcomella added the performance Possible performance wins label Apr 7, 2021
@mcomella mcomella added this to Needs prioritization in Performance, front-end roadmap via automation Apr 7, 2021
@mcomella mcomella moved this from Needs prioritization to In progress in Performance, front-end roadmap Apr 7, 2021
@mcomella mcomella self-assigned this Apr 7, 2021
@github-actions github-actions bot added the needs:triage Issue needs triage label Apr 7, 2021
mcomella added a commit to mcomella/fenix that referenced this issue Apr 8, 2021
mcomella added a commit to mcomella/fenix that referenced this issue Apr 8, 2021
…initely.

We do this is as a separate commit over the original implementation
because it's simpler to implement the class without this optimization.
mcomella added a commit to mcomella/fenix that referenced this issue Apr 8, 2021
mcomella added a commit to mcomella/fenix that referenced this issue Apr 8, 2021
…tartupStateProvider.

The StartupActivityStateProvider uses an imperative implementation,
driven by callbacks, to set the state of the application. This is hard
to follow as you need to understand which callbacks will be called in
which order. For example, to make sense of an implementation like this,
COLD, WARM, AND HOT would likely need to be implemented in separate
ActivityLifecycleCallbacks.

I feel the StartupStateProvider is an improvement because it leverages
the StartupActivityLog to query a linear state for a more understandable
implementation. Furthermore, it seems accessible to write COLD, WARM,
and HOT in the same class because they can all be approached the same
way.
mcomella added a commit to mcomella/fenix that referenced this issue Apr 8, 2021
mcomella added a commit to mcomella/fenix that referenced this issue Apr 8, 2021
…ActivityCreated and similar.

It's too much work to squash "correctly".
mcomella added a commit that referenced this issue Apr 13, 2021
We do this is as a separate commit over the original implementation
because it's simpler to implement the class without this optimization.
mcomella added a commit that referenced this issue Apr 13, 2021
mcomella added a commit that referenced this issue Apr 13, 2021
…vider.

The StartupActivityStateProvider uses an imperative implementation,
driven by callbacks, to set the state of the application. This is hard
to follow as you need to understand which callbacks will be called in
which order. For example, to make sense of an implementation like this,
COLD, WARM, AND HOT would likely need to be implemented in separate
ActivityLifecycleCallbacks.

I feel the StartupStateProvider is an improvement because it leverages
the StartupActivityLog to query a linear state for a more understandable
implementation. Furthermore, it seems accessible to write COLD, WARM,
and HOT in the same class because they can all be approached the same
way.
mcomella added a commit that referenced this issue Apr 13, 2021
…d and similar.

It's too much work to squash "correctly".
mcomella added a commit to mcomella/fenix that referenced this issue Apr 14, 2021
mcomella added a commit to mcomella/fenix that referenced this issue Apr 14, 2021
…test.

The test failed with the rewrite of the code because it violates
one of our assumptions that only one Activity will be started. However,
since it doesn't rely on observed behavior and we made up the events,
it's value is questionable so it seems okay to remove, especially for
the gain of conciseness in the code.
mcomella added a commit to mcomella/fenix that referenced this issue Apr 14, 2021
mcomella added a commit to mcomella/fenix that referenced this issue Apr 14, 2021
mcomella added a commit to mcomella/fenix that referenced this issue Apr 14, 2021
mcomella added a commit to mcomella/fenix that referenced this issue Apr 14, 2021
mcomella added a commit to mcomella/fenix that referenced this issue Apr 14, 2021
…test.

The test failed with the rewrite of the code because it violates
one of our assumptions that only one Activity will be started. However,
since it doesn't rely on observed behavior and we made up the events,
it's value is questionable so it seems okay to remove, especially for
the gain of conciseness in the code.
mcomella added a commit to mcomella/fenix that referenced this issue Apr 14, 2021
mcomella added a commit to mcomella/fenix that referenced this issue Apr 14, 2021
mcomella added a commit to mcomella/fenix that referenced this issue Apr 14, 2021
mcomella added a commit that referenced this issue Apr 16, 2021
The test failed with the rewrite of the code because it violates
one of our assumptions that only one Activity will be started. However,
since it doesn't rely on observed behavior and we made up the events,
it's value is questionable so it seems okay to remove, especially for
the gain of conciseness in the code.
@mcomella
Copy link
Contributor Author

This could conceivably be QA'd but it'd be difficult to convey the nuances so I'm concerned it'd mean diminishing returns so I won't flag for it.

@mcomella mcomella added the eng:qa:not-needed Added by QA to issues that cannot be tested label Apr 16, 2021
Performance, front-end roadmap automation moved this from In progress to Done Apr 16, 2021
pkirakosyan pushed a commit to gexsi/user-agent-android that referenced this issue Aug 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
eng:qa:not-needed Added by QA to issues that cannot be tested needs:triage Issue needs triage performance Possible performance wins
Development

No branches or pull requests

1 participant