1324051 - Add telemetry for ActivityStream highlights query time

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P1)

Description

[Andrzej Hunt :ahunt]

This will be useful for:
- Bug 1298786 - adds domain blacklist to highlights
- Bug 1312017 - optimise the highlights query

Comment 1

[Andrzej Hunt :ahunt]

Attachment: Bug 1324051 - Pre: fix constant name typo

Review commit: https://reviewboard.mozilla.org/r/99176/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99176/

Comment 2

[Andrzej Hunt :ahunt]

Attachment: Bug 1324051 - Add highlights histogram telemetry

Review commit: https://reviewboard.mozilla.org/r/99178/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99178/

Comment 3

[:Grisha Kruglov]

Comment on attachment 8819354 [details]
Bug 1324051 - Pre: fix constant name typo

https://reviewboard.mozilla.org/r/99176/#review99466

Comment 4

[:Grisha Kruglov]

Comment on attachment 8819355 [details]
Bug 1324051 - Add highlights histogram telemetry

https://reviewboard.mozilla.org/r/99178/#review99472

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d3e7a0b5ee00
Pre: fix constant name typo r=Grisha
https://hg.mozilla.org/integration/autoland/rev/f93bcba13541
Add highlights histogram telemetry r=Grisha

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d3e7a0b5ee00
https://hg.mozilla.org/mozilla-central/rev/f93bcba13541

Comment 7

[Georg Fritzsche [:gfritzsche]]

Please note that all new probes require data collection review:
https://wiki.mozilla.org/Firefox/Data_Collection

Comment 8

[Andrzej Hunt :ahunt]

Comment on attachment 8819355 [details]
Bug 1324051 - Add highlights histogram telemetry

I'd like to request data collection review.

This probe measures the time taken for the Activity Stream highlights query. Activity Stream is still experimental and under active development, we would like to be able to track the performance impact caused by any changes to our highlights query. This query is used every time the new tab page is shown, it's important to minimise performance impacts to make sure Fennec is responsive.

(E.g. we will be making the highlights query more complicated in Bug 1298786, and want to make sure that change doesn't significantly impact performance. We are likely to fiddle with this query a lot more in the future too.)

Comment 9

[Chenxia Liu [:liuche]]

Comment on attachment 8819355 [details]
Bug 1324051 - Add highlights histogram telemetry

https://reviewboard.mozilla.org/r/99178/#review100456

::: toolkit/components/telemetry/Histograms.json:6304
(Diff revision 1)
>      "alert_emails": ["[e-mail]"],
>      "bug_numbers": [1293790],
>      "cpp_guard": "ANDROID"
>    },
> +  "FENNEC_ACTIVITY_STREAM_HIGHLIGHTS_LOADER_TIME_MS": {
> +    "expires_in_version": "never",

lol I guess that this is already landed, but I would comment that I'd prefer if this was not "never". I'm fine with sticking it at something far in the future like 60, giving a full ride on Nightly to release + 2 cycles, but there should be an expiration.

Comment 10

[Andrzej Hunt :ahunt]

(In reply to Chenxia Liu [:liuche] from comment #9)
> lol I guess that this is already landed, but I would comment that I'd prefer
> if this was not "never". I'm fine with sticking it at something far in the
> future like 60, giving a full ride on Nightly to release + 2 cycles, but
> there should be an expiration.

I'll think about filing a followup bug to discuss this more formally - and I'm happy to revise this if needed.

Originally I'd just copied the Activity Stream topsites probe, which is also "never". However I think that is actually based on the old Topsites probe (FENNEC_TOPSITES_LOADER_TIME_MS), which also a "never".

I do imagine we'd want to keep these all forever though:
- These queries are run every time the new tab screen is shown (i.e. high user impact) - regressions should therefore be avoided since this is highly visible to pretty much all users, and repeatedly so, during normal use of the app. (In other words: a regression could make the app feel significantly slower to most users.)
- These queries could be impacted by any database changes: even unrelated schema changes could potentially slow down DB queries (at least that's what I've been told regarding changes in desktop's places - schema changes are avoided partly for performance reasons). Changes to the size of the DB could cause slowdowns (e.g. changes regarding how much history is synced, or changes wrt clearing of old history, etc.).
- We seem to make significant changes at least once a year, so we'd have to keep recreating this probe every time we do that.

In other words: it's really important to avoid regressions here, and it's fairly easy to cause regressions in changes that would otherwise seem unrelated - so we probably want to keep monitoring the performance forever.

On the other hand, it's likely the old topsites implementation will be removed at some point in the near future, so being reminded of its expiry would help ensure that it's removed in a timely fashion.

Comment 11

[Chenxia Liu [:liuche]]

Hmmm, not entirely sure about whether telemetry is the right place to do perf regression, however, since there doesn't seem to be an easy alternative, I'll just keep this in mind for the future.