Open Bug 632639 Opened 9 years ago Updated 3 years ago

add instrumentation to Thunderbird to try to improve first time user setup

Categories

(Thunderbird :: General, defect)

defect
Not set

Tracking

(Not tracked)

People

(Reporter: Bienvenu, Unassigned)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

Attached patch wip (obsolete) — Splinter Review
We'd like to improve our setup process and first run experience, so we first need to measure how that process is working for our users (on an opt-in basis, of course). This bug is about adding that instrumentation.
OS: Windows 7 → All
Hardware: x86 → All
Version: 3.0 → Trunk
Attached patch wip v2 (obsolete) — Splinter Review
this adds some more notifications, and makes the helper app that registers as the default client synchronous, so that we can send the right notification.
Attachment #510871 - Attachment is obsolete: true
With this patch, we track the following events, with timestamps:

"msgSent" - user sent a message successfully
"setAsDefault" - thunderbird set as the default mail client
"msgDownloaded" - a message was added to a folder
"accountAdded" - an account was added (meaning an incoming server was added)
"smtpServerAdded" - an smtp server was added

We don't track failed attempts to send a message, though I can look at that.

I've designed this to be all notification driven, so that we aren't sprinkling calls to the instrumentation module throughout the code. I've also tried to make it as unobtrusive as possible, by unregistering itself as a listener for the various notifications once it receives them, and not registering at all on startup if accounts already exist, or instrumentation has been turned off.

I need to get a test http url so we can verify that the data is actually getting posted. And I need to think which parts of this we can test with mozmill. I think sancus signed up for helping with the former - I basically just need a url that I can http post to, and some verification that the data is getting through, for testing purposes.
Attachment #512235 - Attachment is obsolete: true
Attached patch proposed fix for feedback (obsolete) — Splinter Review
I haven't gotten a test server to try this with yet, but everything else is basically working, so I'd like to get feedback in preparation for getting reviews. Sid, I was hoping to get feedback on the nsMailWinIntegration.cpp change that we talked about (or even an r+), and Blake, I was hoping to get your feedback on everything else, so we can get at least some of this landed soon. You'll need the patch in Bug 634907 in order to get the mozmill test to run.
Attachment #512502 - Attachment is obsolete: true
Attachment #515204 - Flags: feedback?(sid.bugzilla)
Attachment #515204 - Flags: feedback?(bwinton)
Status: NEW → ASSIGNED
Depends on: 634907
(In reply to comment #2)
> I need to get a test http url so we can verify that the data is actually
> getting posted. And I need to think which parts of this we can test with
> mozmill. I think sancus signed up for helping with the former - I basically
> just need a url that I can http post to, and some verification that the data is
> getting through, for testing purposes.

If you can send the data as a GET instead of a POST, then it gets way easier to set up.  (See bug 629270 for details…)
(In reply to comment #5)

> If you can send the data as a GET instead of a POST, then it gets way easier to
> set up.  (See bug 629270 for details…)

Thx, can I do that with a json blob? If it's just a string, I would think so...
I'll take care of this on Monday, it's really straightforward just between being/sick off it kinda got lost in the shuffle, sorry. You can use POST, I'll just write the JSON blobs to a file much like I do with the account provisioning logs.
Comment on attachment 515204 [details] [diff] [review]
proposed fix for feedback

It would be interesting to see how often people couldn't send email, instead of just the successfully sent times.

It looks like you forgot to add the tests themselves.  ;)

For mailInstrumentation.js:
  The line:
    if (aTopic == "mail:composeSendSucceeded") 
  has a trailing space.

  nsIMFListener isn't used.

  You could probably inline mFNSContractID.  Or not.  Either way is fine.  :)

  _currentStateString seems more like "_lastStateString" from the comment above it.

  There are some dump statements that should probably be removed or turned into log statements.

  Why do you remove yourself as a listener in the _accountsChanged method?  Is it that we're only interested in the first account being added, and don't care how many accounts people add?

  I guess we also remove ourselves as GFM listeners in the msgAdded method.  Is that for the same reason?  We're only concerned with the first message sent successfully?

  Of course, if those two are correct to remove themselves, then should we also remove ourselves in _smtpServerAdded?

  For _postStateObject would it make sense to post an empty object, instead of logging an exception, if the account wasn't set up?  And I believe you've already implemented "// But what if userOptedIn has changed? Pref change listener?", so we can remove the comment…

  I see calls to:
    mailInstrumentationManager.addEvent("msgSent", true);
    mailInstrumentationManager.addEvent("setAsDefault", true);
    mailInstrumentationManager.addEvent("msgDownloaded", true);
  But I also see calls to:
    mailInstrumentationManager.addEvent("accountAdded", "true");
    mailInstrumentationManager.addEvent("smtpServerAdded", "true");
  I would be happy with strings or booleans in both cases, but we should probably be consistent.

  The function definitions for observe, _loadState, _postStateObject, _createStateObject, init, and uninit should probably have their {s moved to the same line.

  You don't seem to be using _initialized.

  Should you remove the prefs observers in uninit?

  And finally, we're only adding the first event of any given type?  (I think that's correct, but wanted to double-check.)


To sum up, there are some minor things, and I'm not sure whether we want the instrumentation to only log the first event or all the events, but assuming we only want the first event, this seems like a good direction to go.  Oh, and I haven't seen the tests.  :)

Later,
Blake.
Attachment #515204 - Flags: feedback?(bwinton) → feedback+
thx for the feedback. Yes, I'm only interested in the first event of any given type, and for events that might be frequent, like msgAdded, it's a big performance win to remove ourselves as listeners for the events. For other events, it's no big deal.
Comment on attachment 515204 [details] [diff] [review]
proposed fix for feedback

>+  executeInfo.cbSize = sizeof(SHELLEXECUTEINFOW);
>+  executeInfo.hwnd = hWnd;
>+  executeInfo.fMask = SEE_MASK_NOCLOSEPROCESS;
>+  executeInfo.lpDirectory = NULL;
>+  executeInfo.lpFile = appHelperPath.get();
>+  executeInfo.lpParameters = params.get();
>+  executeInfo.nShow = SW_SHOWNORMAL;
> 
>-  CloseHandle(pi.hProcess);
>-  CloseHandle(pi.hThread);
>+  DWORD dwRet;
> 
>+  if (ShellExecuteExW(&executeInfo))
>+  {
>+    // We want to block until the program exits
>+    DWORD dwSignaled = WaitForSingleObject(executeInfo.hProcess, INFINITE);

Note that this will be synchronous iff the non-elevated helper.exe stays alive while the elevated helper.exe does its work. Have you tested to make sure that's the case?

>+    if (dwSignaled == WAIT_OBJECT_0)
>+      if (!GetExitCodeProcess(executeInfo.hProcess, &dwRet))
>+        dwRet = GetLastError();
>+  }
>+  else
>+    return NS_ERROR_ABORT;
>+
>+  // We're going to ignore errors here since there's nothing we can do about
>+  // them, and helper.exe seems to return non-zero ret on success.

If you don't actually use the error, it's probably best to get rid of dwRet and the GetExitCodeProcess call.
Attachment #515204 - Flags: feedback?(sid.bugzilla) → feedback+
(In reply to comment #10)
> 
> Note that this will be synchronous iff the non-elevated helper.exe stays alive
> while the elevated helper.exe does its work. Have you tested to make sure
> that's the case?

I've tested that we're set as the default client immediately after the dialog comes down, no matter how long I wait with the elevated helper app up, so I believe that's the case.
> 
> 
> If you don't actually use the error, it's probably best to get rid of dwRet and
> the GetExitCodeProcess call.

OK - it bothered me a little that I wasn't getting a success code back even though it worked fine, but there's nothing I can do with an error anyway.
addressed feedback comments
Attachment #518788 - Flags: review?(sid.bugzilla)
Comment on attachment 518788 [details] [diff] [review]
make default client dialog sync - checked in

>+  // We need an hWnd to cause UAC to pop up immediately
>+  // If GetForegroundWindow returns NULL, then the UAC prompt will still appear,
>+  // but minimized.
>+  HWND hWnd = GetForegroundWindow();
>

I think this isn't required because helper.exe causes the UAC prompt, not us, but I'm not sure. r+ if you try removing this bit (setting hWnd below to null) and seeing if the UAC prompt still shows up in the foreground.
Attachment #518788 - Flags: review?(sid.bugzilla) → review+
This addresses most of Blake's comments, except that I left one dump statement in there, because I want to leave it until I've tested the http stuff (and until Bryan has hooked up the opt-in UI), and I left the exception on no account setup in, because if there's no account, I can't post anything useful to the server, since we're going to use a hash of the e-mail address to identify the posts. Perhaps Dave will see some benefit in tracking users who opt-in but don't setup an account, but I want to see the opt-in UI first...
Attachment #515204 - Attachment is obsolete: true
Attachment #518802 - Flags: review?(bwinton)
oh, the diff claims that wrapper.py has a ^M line ending, but I don't think it does.
Comment on attachment 518788 [details] [diff] [review]
make default client dialog sync - checked in

you were right that the foreground hwnd stuff wasn't needed so I removed that.
Attachment #518788 - Attachment description: make default client dialog sync → make default client dialog sync - checked in
Comment on attachment 518802 [details] [diff] [review]
addresses Blake's comments - checked in

Remove the second dump statement <dump("after wait for autoconfig");>, and I'm happy with it.

(Note: I haven't run the tests, so you should also make sure they work on all the platforms before committing, but I trust you to do that.)

Later,
Blake.
Attachment #518802 - Flags: review?(bwinton) → review+
initial support has been checked in, preffed off. Bryan, do you have a bug for doing the opt-in UI? Sancus, do you have a bug for the test server?
marking in test-suite+, because the things that are testable are tested (mozmill can't do a message send, or set TB as the default app).
Flags: in-testsuite+
You can post json to:
http://momodev.org/tbreceive

And see the results at:
http://www.momodev.org/tb_data.json

Anything you post will be added to the array verbatim, so you'll want to post, say, {"data":"HereIsSomeData"}. It just adds the whole body of the POST request as one element to the array, no parameters are necessary.
(In reply to comment #18)
> initial support has been checked in, preffed off. Bryan, do you have a bug for
> doing the opt-in UI? Sancus, do you have a bug for the test server?

Created bug 641935 for tracking the UI needed to get people to opt-in to this work.
Depends on: 642103
Andrei, would we still use http://momodev.org/tbreceive post moco-assimilation? I'm hoping to sneak the instrumentation in along with the test-pilot work.
What's the status here ?
No longer blocks: 641935
Depends on: 641935
We're still waiting on the opt-in ui, or a decision that the test-pilot opt-in ui is sufficient. We probably need to fix up the url we deliver the info to by changing a pref, and I think there's a change that needs to be made because of some core changes to the way we handle http request responses. See Bug 687332
(In reply to David :Bienvenu from comment #24)
> We're still waiting on the opt-in ui, or a decision that the test-pilot
> opt-in ui is sufficient. We probably need to fix up the url we deliver the
> info to by changing a pref, 

TP addon has been removed. Does that change the game.

> I think there's a change that needs to be
> made because of some core changes to the way we handle http request
> responses. See Bug 687332

bug was fixed 2012-08-31
Assignee: mozilla → nobody
Status: ASSIGNED → NEW
Summary: add instrumentation to Thunderbird to try to improve setup → add instrumentation to Thunderbird to try to improve first time user setup
Comment on attachment 518802 [details] [diff] [review]
addresses Blake's comments - checked in

This has also landed:
http://hg.mozilla.org/comm-central/rev/478ab6a5e8d5
Attachment #518802 - Attachment description: addresses Blake's comments → addresses Blake's comments - checked in
You need to log in before you can comment on or make changes to this bug.