Homebrew (opt-in) Analytics tweaks. #57

MikeMcQuaid · 2016-04-09T14:09:23Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you successfully ran brew tests with your changes locally?
add HOMEBREW_PRODUCT global variable
only differentiate between /usr/local and non-/usr/local Homebrew
prefixes to avoid sharing sensitive user information
note if e.g. build errors are occurring under CI
Add HOMEBREW_NO_ANALYTICS variable (this will be how people opt-out
when this is enabled for everyone)
Add HOMEBREW_ANALYTICS_DEBUG variable to output all the analytics
that are sent
Move Bash analytics code to Library/Homebrew/utils/analytics.sh
Add documentation for our analytics and why/what/when/how and opt-out

CC @xu-cheng and @UniqMartin for thoughts.

After this I think we've got everything we need to 🚢 analytics to everyone (i.e. remove the HOMEBREW_ANALYTICS checks). If any @Homebrew/maintainers disagree with this: let me know ASAP. Thanks!

xu-cheng · 2016-04-09T14:11:16Z

Library/Homebrew/utils/analytics.sh

+      -d an="$HOMEBREW_PRODUCT" \
+      -d av="$HOMEBREW_VERSION" \
+      -d t="screenview" \
+      -d cd="$HOMEBREW_COMMAND"


We talk about only logging official commands, right?

Updated with this in place. See

brew/Library/Homebrew/utils/analytics.sh

Lines 27 to 34 in 7cb6c73

# Don't report non-official commands.

if ! [[ -f "$HOMEBREW_LIBRARY/Homebrew/cmd/$HOMEBREW_COMMAND.rb" ||

-f "$HOMEBREW_LIBRARY/Homebrew/cmd/$HOMEBREW_COMMAND.sh" ||

-f "$HOMEBREW_LIBRARY/Homebrew/dev-cmd/$HOMEBREW_COMMAND.rb" ||

-f "$HOMEBREW_LIBRARY/Homebrew/dev-cmd/$HOMEBREW_COMMAND.sh" ]]

then

return

fi

I think we should reduce duplication here as argument list is the same.

I don't know how to do that easily in Bash CC @UniqMartin.

Use a variable for url?

Not just the URL but e.g. the arguments like --silent being omitted and not doing a &>/dev/null & disown

Use bash array then?

MikeMcQuaid · 2016-04-09T14:39:04Z

Updated:

Only official Homebrew commands are reported
Ruby analytics are now reported in a forked, background process

MikeMcQuaid · 2016-04-09T14:50:23Z

Updated:

also whitelist cask, bundle, services commands.

xu-cheng · 2016-04-09T17:16:21Z

Library/Homebrew/utils/analytics.rb

+    else
+      "non-/usr/local"
+    end
+    ci = ", CI=1" if ENV["CI"]


Should this just be CI(i.e. without =1)?

tdsmith · 2016-04-09T17:27:40Z

I wonder if it's worth stapling the Google certificate chain to prevent certain classes of MITM attacks. https://src.chromium.org/viewvc/chrome/trunk/src/net/http/transport_security_state_static.json gives some insight into what Google promises for its properties.

MikeMcQuaid · 2016-04-09T18:00:55Z

@tdsmith Feels overkill to me but if you felt like making a PR I'd not oppose that.

MikeMcQuaid · 2016-04-10T15:04:34Z

Updated:

addressed all feedback
only log non-/usr/local prefix
don't run analytics during brew tests

MikeMcQuaid · 2016-04-10T15:15:08Z

Updated:

added Google Analytics screenshot

I'll merge this as-is tomorrow unless there are any objections and open a new PR to 🚢 this to all users.

UniqMartin · 2016-04-10T21:38:47Z

Library/Homebrew/utils/analytics.rb

-  metadata_args = metadata.map do |key, value|
-    ["-d", "#{key}=#{value}"] if key && value
-  end.compact.flatten
+  debug = !!ENV["HOMEBREW_ANALYTICS_DEBUG"]


RuboCop doesn't fully approve. !ENV["HOMEBREW_ANALYTICS_DEBUG"].nil? maybe?

Weird. !! will never be nil. I'll take a look, thanks.

tdsmith · 2016-04-10T23:34:26Z

share/doc/homebrew/Analytics.md

+Homebrew will shortly begin gathering anonymous aggregate user behaviour analytics and reporting these to Google Analytics.
+
+## Why?
+Homebrew is provided free of charge and run entirely by volunteers in their spare time. As a result, we do not have the resources to do detailed user studies of Homebrew users to decide on how best to design future features and prioritise current work. Anonymous aggregate user analytics allow us to prioritise fixes and features based on how, where and when people use Homebrew.


I think a clear and detailed example will help. Identifying buggy formulae is one, since we will report build failures.

tdsmith · 2016-04-10T23:38:06Z

Two other things that seem interesting:

Tracking options specified for formulas
Tracking CI or not-CI

MikeMcQuaid · 2016-04-11T07:23:24Z

This feels a bit like to me. I'd be grateful if more important changes like this one came with a bit more time for review, even if this further delays the change a day or two.

If this was shipped to users I'd agree but I think given it's still an opt-in feature there's no reason we can't keep moving forwards on this and deal with feedback after merging. Point taken, though.

I wonder if we can, before shipping, discuss the option of auto-disabling analytics for non-TTY use. It feels a bit weird to track stuff like $(brew --prefix) (in many shell init scripts) or $(brew commands) (in shell completion), that are run frequently and practically never represent an actual use of Homebrew. My feeling is that this would both improve acceptance of the analytics and make the numbers we gather more useful. Or am I missing some important use case where stdout is not a TTY and that we really want to track? (I'm happy to discuss this someplace else, as that's not something that blocks this PR.)

We shouldn't track $(brew --prefix) as is because it's one of the things short-circuited by brew.sh. I'm open to blacklisting specific commands like commands but I'd rather avoid a where stdout is not a TTY blacklist as I can see that breaking any Homebrew wrappers that we'd want to know about.

MikeMcQuaid · 2016-04-11T07:24:06Z

Tracking options specified for formulas

Agreed. Will address this in the next PR.

Tracking CI or not-CI

This is done already; see analytics_label

MikeMcQuaid · 2016-04-11T09:03:07Z

Updated:

Added all requested style and documentation changes
Blacklisted brew commands command
Added non-default options tracking for formulae installs

xu-cheng · 2016-04-11T09:19:47Z

Library/Homebrew/utils/analytics.sh

+  fi
+
+  # Don't report commands used mostly by our scripts and not users.
+  if [[ "$HOMEBREW_COMMAND" = "commands" ]]


There are many other commands also used in scripts. e.g. search(without any option), list, command(to test the existence of external command) and even tap(without any option or --list-* option). There is also --cache which it's not short circuited as --repo/--prefix does.

Just a FYI note.

Which do you think we should remove? I'd like to know some of them that are actually being run by users.

Clearly, current blacklist approach doesn't scale well. It may be better to put all autocompletion script related commands into a separate group at first. e.g. a dedicate command for script autocompletion? (just a random idea)

Yeh, I think that'd be a great idea CC @tdsmith @apjanke

@xu-cheng I don't think this needs to block this PR or even the feature shipping, though, given these are all run in forked background processes.

Would be great to have a TODO comment for the time being.

I agree. Having a dedicated blacklisted entry point for things used from shell completion sounds like a good solution, that doesn't depend on “magic” like testing stdout for a TTY.

MikeMcQuaid · 2016-04-11T10:11:52Z

Updated:

Don't report any formulae names from private taps (will rely on Cache Tap#private? checks. #67 being merged to be fast)

UniqMartin · 2016-04-11T13:53:20Z

Library/Homebrew/formula_installer.rb

@@ -202,7 +202,10 @@ def install

    oh1 "Installing #{Tty.green}#{formula.full_name}#{Tty.reset}" if show_header?

-    report_analytics_event("install", formula.full_name)
+    unless exception.formula.tap.private?


Looks like this should have been just formula.tap.private? for the tests not to fail.

Also tap can be nil.

xu-cheng · 2016-04-12T09:01:16Z

Library/Homebrew/utils/analytics.rb

-def report_analytics_exception(exception, options={})
-  if exception.is_a? BuildError
+def report_analytics_exception(exception, options = {})
+  if exception.is_a? BuildError && !exception.formula.tap.private?


exception.is_a?(BuildError) && exception.formula.tap && !exception.formula.tap.private?

MikeMcQuaid · 2016-04-12T09:10:54Z

Updated based on feedback. Think this is good to 🚢 along with #67 now when they are both 💚.

xu-cheng · 2016-04-12T09:24:50Z

Library/Homebrew/utils/analytics.sh

@@ -0,0 +1,71 @@
+setup-analytics() {
+  [[ -z "$HOMEBREW_AUTO_UPDATE" ]] && return
+  [[ -n "$HOMEBREW_NO_AUTO_UPDATE" ]] && return


You mean HOMEBREW_ANALYTICS not UPDATE right?

My code examples shouldn't always be copied verbatim. 😉 Good thing it was noticed before it could create any confusion. 🙇

- add `HOMEBREW_PRODUCT` global variable - only differentiate between `/usr/local` and `non-/usr/local` Homebrew prefixes to avoid sharing sensitive user information - note if e.g. build errors are occurring under CI - Add `HOMEBREW_NO_ANALYTICS` variable (this will be how people opt-out when this is enabled for everyone) - Add `HOMEBREW_ANALYTICS_DEBUG` variable to output all the analytics that are sent - Move Bash analytics code to `Library/Homebrew/utils/analytics.sh` - Add documentation for our analytics and why/what/when/how and opt-out - Only official Homebrew commands are reported - Ruby analytics are now reported in a forked, background process

xu-cheng reviewed Apr 9, 2016
View reviewed changes

MikeMcQuaid force-pushed the analytics-tweaks branch from 6a65e5b to 7cb6c73 Compare April 9, 2016 14:37

MikeMcQuaid force-pushed the analytics-tweaks branch from 7cb6c73 to 840b6ab Compare April 9, 2016 14:50

xu-cheng reviewed Apr 9, 2016
View reviewed changes

MikeMcQuaid force-pushed the analytics-tweaks branch 2 times, most recently from f5b9715 to 0bc7d3a Compare April 10, 2016 15:03

MikeMcQuaid force-pushed the analytics-tweaks branch from 0bc7d3a to 80bb958 Compare April 10, 2016 15:14

UniqMartin reviewed Apr 10, 2016
View reviewed changes

tdsmith reviewed Apr 10, 2016
View reviewed changes

MikeMcQuaid force-pushed the analytics-tweaks branch 2 times, most recently from 4f325ac to f21db5c Compare April 11, 2016 09:02

MikeMcQuaid force-pushed the analytics-tweaks branch 3 times, most recently from 91e62bf to cd587a5 Compare April 11, 2016 09:06

xu-cheng reviewed Apr 11, 2016
View reviewed changes

MikeMcQuaid force-pushed the analytics-tweaks branch from cd587a5 to bf67e93 Compare April 11, 2016 10:09

MikeMcQuaid mentioned this pull request Apr 11, 2016

Cache Tap#private? checks. #67

Merged

4 tasks

UniqMartin reviewed Apr 11, 2016
View reviewed changes

MikeMcQuaid force-pushed the analytics-tweaks branch from bf67e93 to cc81840 Compare April 12, 2016 08:49

xu-cheng reviewed Apr 12, 2016
View reviewed changes

MikeMcQuaid force-pushed the analytics-tweaks branch from cc81840 to 4d98592 Compare April 12, 2016 09:10

xu-cheng reviewed Apr 12, 2016
View reviewed changes

MikeMcQuaid force-pushed the analytics-tweaks branch 2 times, most recently from cf5356b to b102233 Compare April 12, 2016 09:31

MikeMcQuaid merged commit 0c85113 into master Apr 12, 2016

MikeMcQuaid deleted the analytics-tweaks branch April 12, 2016 10:02

UniqMartin added this to the Analytics deployment milestone Apr 12, 2016

Homebrew locked and limited conversation to collaborators May 3, 2018

	# Don't report non-official commands.
	if ! [[ -f "$HOMEBREW_LIBRARY/Homebrew/cmd/$HOMEBREW_COMMAND.rb" \|\|
	-f "$HOMEBREW_LIBRARY/Homebrew/cmd/$HOMEBREW_COMMAND.sh" \|\|
	-f "$HOMEBREW_LIBRARY/Homebrew/dev-cmd/$HOMEBREW_COMMAND.rb" \|\|
	-f "$HOMEBREW_LIBRARY/Homebrew/dev-cmd/$HOMEBREW_COMMAND.sh" ]]
	then
	return
	fi

Homebrew (opt-in) Analytics tweaks. #57

Homebrew (opt-in) Analytics tweaks. #57

Conversation

MikeMcQuaid commented Apr 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Apr 9, 2016

MikeMcQuaid commented Apr 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdsmith commented Apr 9, 2016

MikeMcQuaid commented Apr 9, 2016

MikeMcQuaid commented Apr 10, 2016

MikeMcQuaid commented Apr 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdsmith commented Apr 10, 2016

MikeMcQuaid commented Apr 11, 2016

MikeMcQuaid commented Apr 11, 2016

MikeMcQuaid commented Apr 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Apr 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Apr 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment