Add support for Google Analytics.

Add support for Google Analytics.



https://code.launchpad.net/~bac/juju-gui/ga-test/+merge/168191

(do not edit description out of merge proposal)

[bac, 2013-06-07 21:12:17]

Please take a look.

[rharding, 2013-06-10 12:31:36]

LGTM though I'd really like to see the id as configurable as well and then part
of the charm config to allow both settings.

https://codereview.appspot.com/10024046/diff/1/app/config-prod.js
File app/config-prod.js (right):

https://codereview.appspot.com/10024046/diff/1/app/config-prod.js#newcode50
app/config-prod.js:50: useAnalytics: true,
This might be outside the scope, but I think it'd be great to allow the GA id to
be set via config as well, and into the charm. In this way I could use my own GA
on a Gui instance running in my own environment.

https://codereview.appspot.com/10024046/diff/1/app/index.html
File app/index.html (right):

https://codereview.appspot.com/10024046/diff/1/app/index.html#newcode56
app/index.html:56: window.ga_id = 'UA-41463568-2';
but by making the id configurable I should be able to use it on jujucharms.com
as part of our deploy.

[benji, 2013-06-10 13:06:24]

The looks good.

I had a couple of small questions, but the only blocker to landing I see is that
we really should have tests for the one bit of data collection we are doing.  We
should be sure that when the units for a service change (and not otherwise) the
data is collected.

https://codereview.appspot.com/10024046/diff/1/app/index.html
File app/index.html (right):

https://codereview.appspot.com/10024046/diff/1/app/index.html#newcode56
app/index.html:56: window.ga_id = 'UA-41463568-2';
On 2013/06/10 12:31:36, rharding wrote:
> but by making the id configurable I should be able to use it on
http://jujucharms.com
> as part of our deploy.

Yeah, we should either separate out the demo site into its own ID or disable
tracking there all together because people do weird things there that they
wouldn't do in a real deployment, skewing the analytics.

https://codereview.appspot.com/10024046/diff/1/docs/analytics.rst
File docs/analytics.rst (right):

https://codereview.appspot.com/10024046/diff/1/docs/analytics.rst#newcode24
docs/analytics.rst:24: event you won't be able to see what the data will look
like until 12-24 hours
I assume that creating a new event is something we do in code.  Is that so?

https://codereview.appspot.com/10024046/diff/1/docs/analytics.rst#newcode135
docs/analytics.rst:135: The call to _trackEvent[2] look like:
"calls" perhaps.

https://codereview.appspot.com/10024046/diff/1/docs/analytics.rst#newcode149
docs/analytics.rst:149: question.
Does each environment get a UUID or something similar?  If not we could add one
as an environment annotation.

https://codereview.appspot.com/10024046/diff/1/docs/analytics.rst#newcode160
docs/analytics.rst:160: reporting will be affected but country data should still
be accurate.
That might give us something to tell people who are on the fence about us
collecting information about them.

[benji, 2013-06-10 13:07:04]

Oops, I forgot to put some alphabet soup in my review:

SIFHWSKDJFFLGTMSIFJSEIJFS

[bac, 2013-06-10 14:39:09]

On 2013/06/10 12:31:36, rharding wrote:
> LGTM though I'd really like to see the id as configurable as well and then
part
> of the charm config to allow both settings.


Thanks for walking me through your thinking.  Making the ID configurable via the
charm makes sense and we should make it toggle on/off and replace the ID.  It
may be feasible to allow for multiple IDs too which might prevent people from
turning off our feed entirely.

I will open a bug for that.

You also suggested not making the default be 'on' until it is configurable via
the charm which is a good idea.

[bac, 2013-06-10 19:48:34]

Please take a look.

https://codereview.appspot.com/10024046/diff/1/app/config-prod.js
File app/config-prod.js (right):

https://codereview.appspot.com/10024046/diff/1/app/config-prod.js#newcode50
app/config-prod.js:50: useAnalytics: true,
Saved for a future branch.  For now s/true/false/.

https://codereview.appspot.com/10024046/diff/1/app/index.html
File app/index.html (right):

https://codereview.appspot.com/10024046/diff/1/app/index.html#newcode56
app/index.html:56: window.ga_id = 'UA-41463568-2';
Agreed.  See bug 1189502

https://codereview.appspot.com/10024046/diff/1/docs/analytics.rst
File docs/analytics.rst (right):

https://codereview.appspot.com/10024046/diff/1/docs/analytics.rst#newcode24
docs/analytics.rst:24: event you won't be able to see what the data will look
like until 12-24 hours
Yes, I meant adding new instrumentation. But it is true of exercising an
existing event too, the actual data is delayed.

But the point of this comment was that writing new events is hard because of a
very long delay in seeing the results.  I'll try to reword it.

https://codereview.appspot.com/10024046/diff/1/docs/analytics.rst#newcode135
docs/analytics.rst:135: The call to _trackEvent[2] look like:
On 2013/06/10 13:06:25, benji wrote:
> "calls" perhaps.

Done.

https://codereview.appspot.com/10024046/diff/1/docs/analytics.rst#newcode149
docs/analytics.rst:149: question.
Dunno.  That's why it is an open question.  :)

https://codereview.appspot.com/10024046/diff/1/docs/analytics.rst#newcode160
docs/analytics.rst:160: reporting will be affected but country data should still
be accurate.
Agreed.  I'll turn it on.

[bac, 2013-06-10 20:02:56]

*** Submitted:

Add support for Google Analytics.

R=rharding, benji
CC=
https://codereview.appspot.com/10024046