Add telemetry for ActivityStream highlights query time

RESOLVED FIXED in Firefox 53

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ahunt, Assigned: ahunt)

Tracking

Trunk
Firefox 53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [MobileAS])

Attachments

(2 attachments)

This will be useful for:
- Bug 1298786 - adds domain blacklist to highlights
- Bug 1312017 - optimise the highlights query
Assignee: nobody → ahunt
Iteration: --- → 1.11
Priority: -- → P1
Whiteboard: [MobileAS]
Blocks: 1298786
Comment on attachment 8819354 [details]
Bug 1324051 - Pre: fix constant name typo

https://reviewboard.mozilla.org/r/99176/#review99466
Attachment #8819354 - Flags: review?(gkruglov) → review+
Comment on attachment 8819355 [details]
Bug 1324051 - Add highlights histogram telemetry

https://reviewboard.mozilla.org/r/99178/#review99472
Attachment #8819355 - Flags: review?(gkruglov) → review+
Pushed by ahunt@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/d3e7a0b5ee00
https://hg.mozilla.org/mozilla-central/rev/f93bcba13541
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Please note that all new probes require data collection review:
https://wiki.mozilla.org/Firefox/Data_Collection
Flags: needinfo?(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.)
Flags: needinfo?(ahunt)
Attachment #8819355 - Flags: feedback?(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": ["mobile-frontend@mozilla.com"],
>      "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.
(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.
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.
Attachment #8819355 - Flags: feedback?(liuche)
You need to log in before you can comment on or make changes to this bug.