Last modified: 2014-05-19 17:24:08 UTC
The SignupExpAccountCreationComplete (and SignupExpAccountCreationImpression, which should be updated similarly while we're at this) does not say which conditions it's fired for (this should be clarified as a documentation improvement). However, I would have thought it's for all (pre, post, and control). The docs currently say, "Log server side event if we acquired the user through pre or post edit call to action", which is not consistent with that. However, I don't see anything in the code that actually restricts to pre/post. Another issue is that we previously (when preparing the second deployment after bugfixes, https://trello.com/c/dw4XzcAC/418-implement-instrumentation-for-the-trackedpagecontentsavecomplete-schema) specified that only users on an edit page would get a token. That means users creating an account may not yet have a token. However, there was some discussion on IRC that we may want to token all users hitting the create account page (even so, if they have JavaScript off they won't get one, so we can either not log the event if they have no token or make token optional, like https://meta.wikimedia.org/wiki/Schema:TrackedPageContentSaveComplete). If we do assign a token for visits to the create account page, we'll want to log the create account impression in JavaScript (otherwise, the server will check for a token before the client can assign one due to the 'create account' visit). Finally, I think it should use AddNewAccount (https://www.mediawiki.org/wiki/Manual:Hooks/AddNewAccount), since that actually fires for new accounts, so e.g. refreshing a page or bookmarking/returning will not cause extra firings. Similarly, UserCreateForm is a simpler and more performant (since there's not an extra if on every unrelated page load) way of checking if they're on the signup page (but it only works if we don't need to assign a token to every visitor the 'create account' page).
> clarified as a documentation improvement). However, I would have thought > it's for all (pre, post, and control). The docs currently say, "Log server > side event if we acquired the user through pre or post edit call to action", > which is not consistent with that. > > However, I don't see anything in the code that actually restricts to > pre/post. At one point pre-merge we were logging only impression and creation schemas when directly handed to account creation from the CTA. Phuedx and I discussed this and we determined to only check for the token. Then we can compare that token against the CTA. This affects our ability to detect if they came directly from the CTA prior to signing up. > Finally, I think it should use AddNewAccount > (https://www.mediawiki.org/wiki/Manual:Hooks/AddNewAccount), since that > actually fires for new accounts, so e.g. refreshing a page or > bookmarking/returning will not cause extra firings. > I agree that might be a better approach to gathering the user. Though we are already subscribing to the central auth redirect hook.
(In reply to Matthew Flaschen from comment #0) > The SignupExpAccountCreationComplete (and > SignupExpAccountCreationImpression, which should be updated similarly while > we're at this) does not say which conditions it's fired for (this should be > clarified as a documentation improvement). However, I would have thought > it's for all (pre, post, and control). The docs currently say, "Log server > side event if we acquired the user through pre or post edit call to action", > which is not consistent with that. > > However, I don't see anything in the code that actually restricts to > pre/post. It's fine to do it for all, as long as we know the user's token. > Another issue is that we previously (when preparing the second deployment > after bugfixes, > https://trello.com/c/dw4XzcAC/418-implement-instrumentation-for-the- > trackedpagecontentsavecomplete-schema) specified that only users on an edit > page would get a token. > > That means users creating an account may not yet have a token. However, > there was some discussion on IRC that we may want to token all users hitting > the create account page (even so, if they have JavaScript off they won't get > one, so we can either not log the event if they have no token or make token > optional, like > https://meta.wikimedia.org/wiki/Schema:TrackedPageContentSaveComplete). If > we do assign a token for visits to the create account page, we'll want to > log the create account impression in JavaScript (otherwise, the server will > check for a token before the client can assign one due to the 'create > account' visit). This sounds like an issue also for the pre-edit CTA. If we're assigning token only on edit screen view, then how do we set/collect token for the pre-edit version? > > Finally, I think it should use AddNewAccount > (https://www.mediawiki.org/wiki/Manual:Hooks/AddNewAccount), since that > actually fires for new accounts, so e.g. refreshing a page or > bookmarking/returning will not cause extra firings. > > Similarly, UserCreateForm is a simpler and more performant (since there's > not an extra if on every unrelated page load) way of checking if they're on > the signup page (but it only works if we don't need to assign a token to > every visitor the 'create account' page).
(In reply to Matthew Flaschen from comment #0) > Another issue is that we previously (when preparing the second deployment > after bugfixes, > https://trello.com/c/dw4XzcAC/418-implement-instrumentation-for-the- > trackedpagecontentsavecomplete-schema) specified that only users on an edit > page would get a token. This part is wrong. All anonymous users will also get a token if the experiment is in progress (as a side effect of both getToken and getBucket in ext.gettingstarted.anonymousEditorAcquisition) (plus, all users, including logged in, continue to get tokens on edit pages). So re logging the create account page impression on the server/client, the issue is if we're concerned about people hitting CreateAccount directly without going to any other pages. If so, we have to log on the client (to let the token get assigned before logging).
(In reply to Matthew Flaschen from comment #3) > > So re logging the create account page impression on the server/client, the > issue is if we're concerned about people hitting CreateAccount directly > without going to any other pages. > > If so, we have to log on the client (to let the token get assigned before > logging). I don't think we care about this. All users in the experiment should hit another page before coming to signup.
(In reply to Steven Walling from comment #4) > (In reply to Matthew Flaschen from comment #3) > > > > So re logging the create account page impression on the server/client, the > > issue is if we're concerned about people hitting CreateAccount directly > > without going to any other pages. > > > > If so, we have to log on the client (to let the token get assigned before > > logging). > > I don't think we care about this. All users in the experiment should hit > another page before coming to signup. Following up from IRC, Aaron's comment is "If we get an event for most-to-all newly registered users, then it's cool with me." which is the case with this setup I think.
I'm not sure what that's actually referring to, but if it means direct hits to CreateAccount, that's only one of the issues with this bug. There are also apparent uses of the wrong hook, and documentation issues.
Aaron and I discussed further on IRC, and there are in fact some schema issues, as well as the hooks to be updated. (In reply to Matthew Flaschen from comment #0) > The SignupExpAccountCreationComplete (and > SignupExpAccountCreationImpression, which should be updated similarly while > we're at this) does not say which conditions it's fired for (this should be > clarified as a documentation improvement). However, I would have thought > it's for all (pre, post, and control). The docs currently say, "Log server > side event if we acquired the user through pre or post edit call to action", > which is not consistent with that. Documentation clarified: * https://meta.wikimedia.org/w/index.php?title=Schema%3ASignupExpAccountCreationComplete&diff=8539421&oldid=8102589 * https://meta.wikimedia.org/w/index.php?title=Schema%3ASignupExpAccountCreationImpression&diff=8539445&oldid=8102591 It is for everyone, and token is an optional field (this clarification implies a code change). > However, I don't see anything in the code that actually restricts to > pre/post. This is correct behavior. I've removed that comment in https://gerrit.wikimedia.org/r/133667. > Finally, I think it should use AddNewAccount > (https://www.mediawiki.org/wiki/Manual:Hooks/AddNewAccount), since that > actually fires for new accounts, so e.g. refreshing a page or > bookmarking/returning will not cause extra firings. Done. > Similarly, UserCreateForm is a simpler and more performant (since there's > not an extra if on every unrelated page load) way of checking if they're on > the signup page (but it only works if we don't need to assign a token to > every visitor the 'create account' page). Done.
Change 133731 had a related patch set uploaded by Phuedx: Log account creation with AddNewAccount, impression with UserCreateForm https://gerrit.wikimedia.org/r/133731
Change 133731 merged by jenkins-bot: Log account creation with AddNewAccount, impression with UserCreateForm https://gerrit.wikimedia.org/r/133731
Change 134122 had a related patch set uploaded by Mattflaschen: Log account creation with AddNewAccount, impression with UserCreateForm https://gerrit.wikimedia.org/r/134122
Change 134122 merged by jenkins-bot: Log account creation with AddNewAccount, impression with UserCreateForm https://gerrit.wikimedia.org/r/134122
