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

For #20519: renew perf telemetry probes (August expiry). #20623

Merged
merged 3 commits into from
Aug 2, 2021

Conversation

mcomella
Copy link
Contributor

@mcomella mcomella commented Aug 2, 2021

I used eliserichards' PR #20519 to
renew more easily.


Request for Data Collection Renewal

** This form is for the renewal of an existing, reviewed data collection.**

** All questions are mandatory.
You must receive Data Review from a
Data Steward
on a filled-out Request before shipping your renewed data collection.**

  1. Provide a link to the initial Data Collection Review Request for this collection.

cold_*_to_first_frame + start_reason_*_error:

application_on_create:

storage.stats:

  1. When will this collection now expire?

2022-02-01

  1. Why was the initial period of collection insufficient?

cold_*_to_first_frame + start_reason_*_error + application_on_create: attempted to analyze the telemetry but it required other telemetry analysis to be performed first and was deferred. It's useful to monitor the perf over time before we attempt to analyze it again.

storage.stats: unclear ownership of probe between fenix and perf team. It's useful to monitor over time though to look for large regressions so we'd like to keep analyzing it.

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.

To download an APK when reviewing a PR:

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

@mcomella mcomella requested review from a team as code owners August 2, 2021 17:03
Copy link

@eliserichards eliserichards left a comment

Choose a reason for hiding this comment

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

The following tests were breaking on main, (ignores being added here which is going to land before this) so we should make sure that they are fixed in this PR:

  • StorageStatsMetricsTest.WHEN reporting THEN the values from the storageStats are accumulated
  • StorageStatsMetricsTest.WHEN reporting THEN the query duration is measured

@mcomella mcomella force-pushed the perf-probe-renewal branch from 763df04 to 4972732 Compare August 2, 2021 17:27
@boek
Copy link
Contributor

boek commented Aug 2, 2021

r+

Data Collection Renewal Review (to be filled by Data Stewards)

  1. Is the provided Data Collection Review complete, correct, and data-review+ by a Data Steward?
    Yes
  2. Is the data collection covered by the existing Firefox Privacy Notice?
    Yes

@mcomella
Copy link
Contributor Author

mcomella commented Aug 2, 2021

The following tests were breaking on main, (ignores being added here which is going to land before this) so we should make sure that they are fixed in this PR:

I confirmed the StorageStatsMetrics tests are fixed in my PR through local testing.

@mcomella
Copy link
Contributor Author

mcomella commented Aug 2, 2021

FenixApplicationTest.when setStartupMetrics is called... is not fixed by my test, as suggested by #20616, but the issue seems unrelated to my PR.

@mcomella
Copy link
Contributor Author

mcomella commented Aug 2, 2021

Test failures all look like things that'd be addressed if I rebased so I will rebase and rerun:

    FenixApplicationTest. WHEN setStartupMetrics is called THEN sets some base metrics
    GleanMetricsServiceTest. TabsTray events are correctly recorded
    GleanMetricsServiceTest. awesomebar events are correctly recorded
    GleanMetricsServiceTest. synced tab event is correctly recorded

@mcomella mcomella force-pushed the perf-probe-renewal branch from 4972732 to fd7258a Compare August 2, 2021 17:52
@mcomella
Copy link
Contributor Author

mcomella commented Aug 2, 2021

Locally, I still get two test failures:

  • FenixApplicationTest...
  • GleanMetricsService.TabsTray events...

@eliserichards
Copy link

These two are fixed in #20621. Once that lands you can rebase and those failures should be resolved 👍

@mcomella
Copy link
Contributor Author

mcomella commented Aug 2, 2021

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Aug 2, 2021

Command rebase: success

Branch has been successfully rebased

@mcomella mcomella added the pr:needs-landing PRs that are ready to land [Will be merged by Mergify] label Aug 2, 2021
Copy link

@eliserichards eliserichards left a comment

Choose a reason for hiding this comment

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

🙌 🚀

@mcomella
Copy link
Contributor Author

mcomella commented Aug 2, 2021

Unit test failure in HistoryMenuItemTest:

io.mockk.MockKException: Can't instantiate proxy for class kotlin.Function1
	at io.mockk.impl.instantiation.JvmMockFactory.newProxy(JvmMockFactory.kt:64)
	at io.mockk.impl.instantiation.AbstractMockFactory.newProxy$default(AbstractMockFactory.kt:29)

It looks like one of those Java 11 test failures.

@mcomella
Copy link
Contributor Author

mcomella commented Aug 2, 2021

It doesn't fail locally so... re-run?

@mcomella mcomella closed this Aug 2, 2021
@mcomella mcomella reopened this Aug 2, 2021
@mcomella
Copy link
Contributor Author

mcomella commented Aug 2, 2021

Unit test failure in HistoryMenuItemTest:

io.mockk.MockKException: Can't instantiate proxy for class kotlin.Function1
	at io.mockk.impl.instantiation.JvmMockFactory.newProxy(JvmMockFactory.kt:64)
	at io.mockk.impl.instantiation.AbstractMockFactory.newProxy$default(AbstractMockFactory.kt:29)

It looks like one of those Java 11 test failures.

I filed #20627

@mergify mergify bot merged commit 700033e into mozilla-mobile:main Aug 2, 2021
@mcomella mcomella deleted the perf-probe-renewal branch August 2, 2021 19:21
@eliserichards
Copy link

@Mergifyio backport releases_v91.0.0

@mergify
Copy link
Contributor

mergify bot commented Aug 25, 2021

Command backport releases_v91.0.0: success

Backports have been created

eliserichards pushed a commit that referenced this pull request Aug 25, 2021
…0623) (#21020)

* For #20518: renew perf telemetry probes (August expiry).

I used eliserichards' PR #20519 to
renew more easily.

(cherry picked from commit bbd80b9)

* For #20518: link to data renewal request.

(cherry picked from commit 296dc9c)

* For #20518: disable metrics we don't want to renew.

(cherry picked from commit 700033e)

* For #20518: disable metrics we don't want to renew.

Co-authored-by: Michael Comella <michael.l.comella@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr:needs-landing PRs that are ready to land [Will be merged by Mergify]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants