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

Status: NEW

(Thunderbird :: General, defect)

Description

[David :Bienvenu]

Attachment: wip

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.

Comment 1

[David :Bienvenu]

Attachment: wip v2

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.

Comment 2

[David :Bienvenu]

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.

Comment 3

[David :Bienvenu]

Attachment: make default client dialog close, call uninit method

Comment 4

[David :Bienvenu]

Attachment: proposed fix for feedback

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.

Comment 5

[Blake Winton (:bwinton) (:☕️)]

(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…)

Comment 6

[David :Bienvenu]

(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...

Comment 7

[Andrei Hajdukewycz [:sancus]]

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 8

[Blake Winton (:bwinton) (:☕️)]

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.

Comment 9

[David :Bienvenu]

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 10

[Rain [:rain1] (inactive)]

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.

Comment 11

[David :Bienvenu]

(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.

Comment 12

[David :Bienvenu]

Attachment: make default client dialog sync - checked in

addressed feedback comments

Comment 13

[Rain [:rain1] (inactive)]

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.

Comment 14

[David :Bienvenu]

Attachment: addresses Blake's comments - checked in

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...

Comment 15

[David :Bienvenu]

oh, the diff claims that wrapper.py has a ^M line ending, but I don't think it does.

Comment 16

[David :Bienvenu]

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.

Comment 17

[Blake Winton (:bwinton) (:☕️)]

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.

Comment 18

[David :Bienvenu]

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?

Comment 19

[David :Bienvenu]

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).

Comment 20

[Andrei Hajdukewycz [:sancus]]

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.

Comment 21

[Bryan Clark (DevTools PM) [:clarkbw]]

(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.

Comment 22

[David :Bienvenu]

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.

Comment 23

[Ludovic Hirlimann [:Usul]]

What's the status here ?

Comment 24

[David :Bienvenu]

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

Comment 25

[Wayne Mery (:wsmwk)]

(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

Comment 26

[:aceman]

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