Closed Bug 1367246 Opened 7 years ago Closed 7 years ago

Run a Telemetry experiment to vet the unaccelerated compositor process in Nightly 56

Categories

(Core :: Graphics, defect)

56 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 + fixed

People

(Reporter: u279076, Assigned: u279076)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(5 files, 2 obsolete files)

This is bug tracks running a Telemetry Experiment to A/B test the unaccelerated compositor process as implemented in bug 1356091.
Assigning to myself, code incoming.
Depends on: 1356091
QA Contact: anthony.s.hughes
Whiteboard: [gfx-noted]
Attached patch bug1367246_v1.patch (obsolete) (deleted) — Splinter Review
Initial patch to implement Telemetry Experiment. Note, this experiment has three cohorts: 1) control: users with unaccelerated compositor process enabled 2) disabled: users with unaccelerated compositor process disabled 3) disqualified: users with hardware acceleration The reason for the 'disqualified' cohort is so that accelerated users are not mistakenly factored in to the 'control' cohort data. All three conditions have been tested successfully on a local machine. David, please take a quick look and let me know if you see any issues. Felipe, please review this when you get a chance as we'd like this experiment to conclude before June 12th. I still need to get the XPI signed and approval from Release Management, pending your review.
Attachment #8870611 - Flags: review?(felipc)
Attachment #8870611 - Flags: feedback?(dvander)
Assignee: nobody → anthony.s.hughes
Status: NEW → ASSIGNED
Attached image Screenshot (deleted) —
I left the Nightly running overnight which had the experiment active and came back to this screen. It looks like Nightly had decided to disable my TelEx overnight and is prompting me to re-enable it. I've never seen this before. Is it expected?
(In reply to Anthony Hughes (:ashughes) [GFX][QA][Mentor] from comment #3) > Created attachment 8870929 [details] > Screenshot > > I left the Nightly running overnight which had the experiment active and > came back to this screen. It looks like Nightly had decided to disable my > TelEx overnight and is prompting me to re-enable it. I've never seen this > before. Is it expected? Sorry, forgot to flag the need-infos.
Flags: needinfo?(felipc)
Flags: needinfo?(dothayer)
I never saw this doorhanger before. Andy, do you know if this is from the work to disable legacy addons?
Flags: needinfo?(felipc) → needinfo?(amckay)
I believe thats the doorhanger that occurs for sideloading and add-on. Looking at the add-on it is not multi process compatible, according to the install.rdf. We shouldn't be shipping any non-e10s add-ons. /cc'ing aswan to check what I said.
Flags: needinfo?(amckay) → needinfo?(aswan)
The doorhanger in the "Screenshot" attachment is indeed the one used for sideloading. I don't know much about telemetry experiments but I thought that we were trying to deprecate them in favor of shield? As for multiprocessCompatible, we don't pay attention to it for addons of types other than "extension" (not intentionally anyway...)
Flags: needinfo?(aswan)
Ah this notice used to be an about: page, I didn't know it was changed to a doorhanger. Hm I'm guessing this might have been triggered by the fact that the xpi is not signed yet, but I'm not sure. If after signing the same problem continues to happen, we should investigate because it would be a recent bug.
Flags: needinfo?(dothayer)
Comment on attachment 8870611 [details] [diff] [review] bug1367246_v1.patch Review of attachment 8870611 [details] [diff] [review]: ----------------------------------------------------------------- ::: experiments/unaccelerated-compositor-process-nightly55/code/bootstrap.js @@ +86,5 @@ > + if (compositor == "basic") { > + initializeExperiment(); > + } else { > + Experiments.instance().setExperimentBranch(kSELF_ID, "disqualified").then(null, Cu.reportError); > + initializeExperiment(); since you're now already setting the branch here, you don't need to call initializeExperiment() here or in the similar case below. r+ with this changed. ::: experiments/unaccelerated-compositor-process-nightly55/manifest.json @@ +12,5 @@ > + "appName" : ["Firefox"], > + "channel" : ["nightly"], > + "minVersion" : "55.0a1", > + "maxVersion" : "55.0a1", > + "os" : ["WINNT"] we should use a sample here to not go to 100% of users
Attachment #8870611 - Flags: review?(felipc) → review+
(In reply to :Felipe Gomes (needinfo me!) from comment #8) > Ah this notice used to be an about: page, I didn't know it was changed to a > doorhanger. Hm I'm guessing this might have been triggered by the fact that > the xpi is not signed yet, but I'm not sure. If after signing the same > problem continues to happen, we should investigate because it would be a > recent bug. The doorhanger is unrelated to signing, it is shown for any sideloaded extension. To be clear, your test uses the experiment manifest to have Experiments.jsm set up the experiment right? Kris, could this (telemetry experiments showing the sideload doorhanger) be an (unintended?) side effect of bug 1358846?
Flags: needinfo?(kmaglione+bmo)
(In reply to Andrew Swan [:aswan] from comment #10) > (In reply to :Felipe Gomes (needinfo me!) from comment #8) > > Ah this notice used to be an about: page, I didn't know it was changed to a > > doorhanger. Hm I'm guessing this might have been triggered by the fact that > > the xpi is not signed yet, but I'm not sure. If after signing the same > > problem continues to happen, we should investigate because it would be a > > recent bug. > > The doorhanger is unrelated to signing, it is shown for any sideloaded > extension. > To be clear, your test uses the experiment manifest to have Experiments.jsm > set up the experiment right? Correct
(In reply to Andrew Swan [:aswan] from comment #10) > Kris, could this (telemetry experiments showing the sideload doorhanger) be > an (unintended?) side effect of bug 1358846? That or bug 1356826 seem like likely culprits, yes. I was under the impression that we were no longer deploying new telemetry experiments in favor of Shield studies, though.
Flags: needinfo?(kmaglione+bmo)
Ah no, telemetry experiments are still being used, and there's a few coming up soon.
OK, then I'll look into this.
(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #14) > OK, then I'll look into this. Do you want me to file a bug?
(In reply to Anthony Hughes (:ashughes) [GFX][QA][Mentor] from comment #15) > (In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment > #14) > > OK, then I'll look into this. > > Do you want me to file a bug? Yes, that would help. Thanks.
Depends on: 1367823
Be advised that we're not passed a point where we can run this experiment on Nightly 55 due to bug 1367823. With any luck we might be able to do this in Beta, assuming the lack of an experiment in Nightly doesn't block this feature from riding the trains.
Depends on: 1371363
Updating this to reflect that I plan to run this experiment in 56 now, not 55.
Summary: Run a Telemetry experiment to vet the unaccelerated compositor process in Nightly 55 → Run a Telemetry experiment to vet the unaccelerated compositor process in Nightly 56
Version: unspecified → 56 Branch
Hi Felipe, Here is an updated patch. I've tested it locally and everything works as expected. I made the changes you previously requested but given the amount of time that's past can you please review it? Thanks
Attachment #8883662 - Flags: review?(felipc)
Attachment #8883662 - Flags: review?(felipc) → review+
Felipe, thanks for the r+. I'm uploading a new patch with the following cosmetic changes: - Revised timestamps and duration to start on July 10, end on July 20 - Removed previously commented out code - Made new XPI with these changes I trust this doesn't need re-review.
Attachment #8870611 - Attachment is obsolete: true
Attachment #8883662 - Attachment is obsolete: true
Attachment #8870611 - Flags: feedback?(dvander)
Attachment #8884072 - Flags: review+
Attached file experiment.xpi (unsigned) (deleted) —
Jason, can you please sign this XPI?
Attachment #8884075 - Flags: feedback?(jthomas)
Attached file experiment.xpi signed (deleted) —
Please see attached.
Thanks Jason. Felipe, can you please push this to staging so I can test it before requesting RelMan approval?
Flags: needinfo?(felipc)
Attachment #8884075 - Flags: feedback?(jthomas)
(In reply to :Felipe Gomes (needinfo me!) from comment #24) > Done! > https://hg.mozilla.org/webtools/telemetry-experiment-server/rev/ > 61df6e6c03cf4232a869699c1775ae6f51931c2e Hi Felipe, I'm not seeing any active experiments when I point to https://telemetry-experiment-dev.allizom.org/firefox-manifest.json
Flags: needinfo?(felipc)
I cloned the repo in a different folder (to ensure it wasn't a problem of untracked files that weren't pushed), and built it locally without problems. I wonder if it's some infra issue
Flags: needinfo?(felipc)
(In reply to :Felipe Gomes (needinfo me!) from comment #26) > I cloned the repo in a different folder (to ensure it wasn't a problem of > untracked files that weren't pushed), and built it locally without problems. > I wonder if it's some infra issue I concur, locally it works but pointing to the staging server does not. I'm returned with no active experiments on staging. I don't see anything on the mailing lists that would indicate an infra issue. Who can I contact to look in to this?
:Usul, do you know what's happening here? I'm confused as to why this would work locally but not on staging.
Flags: needinfo?(ludovic)
(In reply to Anthony Hughes (:ashughes) [GFX][QA][Mentor] from comment #28) > :Usul, do you know what's happening here? I'm confused as to why this would > work locally but not on staging. If staging doesn't have video drivers then you'd see a difference (wondering why you asked me though :))
Flags: needinfo?(ludovic)
(In reply to Ludovic Hirlimann [:Usul] from comment #29) > (In reply to Anthony Hughes (:ashughes) [GFX][QA][Mentor] from comment #28) > > :Usul, do you know what's happening here? I'm confused as to why this would > > work locally but not on staging. > > If staging doesn't have video drivers then you'd see a difference (wondering > why you asked me though :)) Reading more , I have no clue and no access to these systems ask someone from :jason's team
(In reply to Ludovic Hirlimann [:Usul] from comment #30) > Reading more , I have no clue and no access to these systems ask someone from :jason's team Sorry, it was suggested I reach out to you. Jason, do you have any idea why I'm seeing no active experiments on staging? This experiment should be active there but I don't see it when I point to https://telemetry-experiment-dev.allizom.org/firefox-manifest.json.
Flags: needinfo?(jthomas)
Sorry I don't think I am going to be much help here. The service telemetry-experiment-dev.allizom.org is currently not managed by my team (Data Operations) and is in IT's control (resides on IT's infrastructure, specifically the generic cluster). Data Operations *should* take control over this service and move it out of IT's control. IT Webops might be the right person to reach out. The Mozilla MOC should be able to assist with contacting the correct people. https://mana.mozilla.org/wiki/pages/viewpage.action?pageId=38553804 has some documentation on this serivce.
Flags: needinfo?(jthomas)
Component: Graphics → WebOps: Other
Product: Core → Infrastructure & Operations
Version: 56 Branch → unspecified
Whiteboard: [gfx-noted] → [kanban:https://webops.kanbanize.com/ctrl_board/2/5061] [gfx-noted]
Taking a look at this now.
There was a hung hg pull that was clogging up things. I've cleared it out and this is working now (though you may have a local cache). If hg pull hangs are a known thing we should probably run this with a timeout command so it doesn't hang forever.
Thanks Eric, this is now working and I've confirmed my experiment is operating as expected on staging. Will now send an email to get approval to push to production.
Component: WebOps: Other → Graphics
Product: Infrastructure & Operations → Core
Whiteboard: [kanban:https://webops.kanbanize.com/ctrl_board/2/5061] [gfx-noted] → [gfx-noted]
Version: unspecified → 56 Branch
Hi Felipe, I just got sign-off from Release Management. The dates in the manifest will need to be updated before pushing to production. Given the setbacks I'd like this to start today (July 17) and run through July 27. Thanks in advance.
Flags: needinfo?(felipc)
Alright, I updated the start/end dates in the manifest, and added the release_tag for it. Once it appears correctly on staging, all that's left is to file a bug for it to be deployed to production: https://hg.mozilla.org/webtools/telemetry-experiment-server/rev/a8a91936dac30a53c64aa027a07a8b85c8e0e7c2 https://hg.mozilla.org/webtools/telemetry-experiment-server/rev/6022f3f24315a62063e09a1b6b9e37a2d0a03c1a
Flags: needinfo?(felipc)
Blocks: 1381599
(In reply to :Felipe Gomes (needinfo me!) from comment #37) > All that's left is to file a bug for it to be deployed to production Thanks Felipe!
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Experiment is now live. Thanks again everyone for your help!
Target Milestone: --- → mozilla56
Flagging some folks to spread this around before it becomes a larger issue... From bug 1371363: The problem I was facing in bug 1367246 comment 3 related to this bug has just been shown to me in the wild on one of my officemates' computers. He updated his Nightly and saw the prompt to enable the experiment. I don't think there's anything I can do now but this may impact the number of users getting my experiment and the prompt may be "scary" to some users.
Flags: needinfo?(milan)
Flags: needinfo?(lhenry)
Flags: needinfo?(dvander)
Anthony, can you get the build details for the Firefox version that exhibited this problem?
Flags: needinfo?(anthony.s.hughes)
(In reply to Andrew Swan [:aswan] from comment #41) > Anthony, can you get the build details for the Firefox version that > exhibited this problem? Adding Roland who should be able to get you the details from his system.
Flags: needinfo?(anthony.s.hughes) → needinfo?(rtanglao)
I'll track this issue in bug 1371363. I wouldn't want users on release to see this warning.
Flags: needinfo?(lhenry)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #43) > I'll track this issue in bug 1371363. I wouldn't want users on release to > see this warning. As a note, I originally saw this warning when I was developing this experiment back in Nightly 55 so I assume it'd affect 55 Beta users had I experimented there instead.
Flags: needinfo?(milan)
Blocks: 1394903
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: