New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/info: display analytics data. #4830

Merged
merged 1 commit into from Sep 6, 2018

Conversation

Projects
None yet
7 participants
@MikeMcQuaid
Copy link
Member

MikeMcQuaid commented Sep 5, 2018

cmd/info: display analytics data.
When users don't have HOMEBREW_NO_ANALYTICS or
HOMEBREW_NO_GITHUB_API set let's display some analytics data in
brew info. This should be useful for both maintainers and for users of
Homebrew.

Note this by default combines all installs across options for a single
number; for formulae with lots of options it's a bit overwhelming to
print the installs per-option. However, for HOMEBREW_DEVELOPERs print
the full output.

Sample non-developer output:

$ brew info wget
wget: stable 1.19.5 (bottled), HEAD
Internet file retriever
https://www.gnu.org/software/wget/
/usr/local/Cellar/wget/1.19.5 (49 files, 3.7MB) *
  Built from source on 2018-09-03 at 20:46:32
From: https://github.com/Homebrew/homebrew-core/blob/master/Formula/wget.rb
==> Dependencies
Build: pkg-config ✔
Required: libidn2 ✔, openssl ✔
Optional: pcre ✔, libmetalink ✘, gpgme ✘
==> Options
--with-debug
	Build with debug support
--with-gpgme
	Build with gpgme support
--with-libmetalink
	Build with libmetalink support
--with-pcre
	Build with pcre support
--HEAD
	Install HEAD version
==> Analytics
install: 84638 (30d), 353800 (90d), 1372775 (365d)
install_on_request: 77926 (30d), 291305 (90d), 1044898 (365d)
build_error: 11 (30d)

Sample developer output:

$ brew info wget
wget: stable 1.19.5 (bottled), HEAD
Internet file retriever
https://www.gnu.org/software/wget/
/usr/local/Cellar/wget/1.19.5 (49 files, 3.7MB) *
  Built from source on 2018-09-03 at 20:46:32
From: https://github.com/Homebrew/homebrew-core/blob/master/Formula/wget.rb
==> Dependencies
Build: pkg-config ✔
Required: libidn2 ✔, openssl ✔
Optional: pcre ✔, libmetalink ✘, gpgme ✘
==> Options
--with-debug
	Build with debug support
--with-gpgme
	Build with gpgme support
--with-libmetalink
	Build with libmetalink support
--with-pcre
	Build with pcre support
--HEAD
	Install HEAD version
==> Analytics
==> install (30d)
wget: 84516
wget --with-debug: 51
wget --with-libressl: 16
wget --with-pcre: 14
wget --with-pcre --with-libmetalink --with-gpgme: 12
wget --with-debug --with-pcre --with-libmetalink --with-gpgme: 8
wget --HEAD: 3
wget --HEAD --with-debug --with-libmetalink --with-gpgme: 3
wget --with-gpgme: 3
wget --with-libmetalink: 3
wget --with-pcre --with-libmetalink: 3
wget --with-debug --with-pcre: 2
wget --with-libmetalink --with-gpgme: 2
wget --with-pcre --with-gpgme: 2
==> install (90d)
wget: 353131
wget --with-debug: 188
wget --with-pcre: 138
wget --with-pcre --with-libmetalink --with-gpgme: 118
wget --with-libressl: 81
wget --with-debug --with-pcre --with-libmetalink --with-gpgme: 47
wget --with-pcre --with-libmetalink: 31
wget --HEAD: 13
wget --with-pcre --with-gpgme: 12
wget --with-gpgme: 11
wget --with-debug --with-pcre: 10
wget --with-libmetalink: 8
wget --HEAD --with-pcre --with-libmetalink --with-gpgme: 4
wget --with-debug --with-pcre --with-libmetalink: 4
wget --with-libmetalink --with-gpgme: 4
==> install (365d)
wget: 1369530
wget --with-pcre: 810
wget --with-debug: 649
wget --with-pcre --with-libmetalink --with-gpgme: 554
wget --with-libressl: 479
wget --with-debug --with-pcre --with-libmetalink --with-gpgme: 235
wget --with-pcre --with-libmetalink: 184
wget --with-gpgme: 67
wget --with-pcre --with-gpgme: 67
wget --with-debug --with-pcre: 65
wget --HEAD: 54
wget --with-libmetalink: 30
wget --with-libmetalink --with-gpgme: 27
wget --with-debug --with-pcre --with-libmetalink: 24
==> install_on_request (30d)
wget: 77827
wget --with-debug: 48
wget --with-pcre: 12
wget --with-pcre --with-libmetalink --with-gpgme: 11
wget --with-debug --with-pcre --with-libmetalink --with-gpgme: 8
wget --HEAD: 3
wget --HEAD --with-debug --with-libmetalink --with-gpgme: 3
wget --with-gpgme: 3
wget --with-libmetalink: 3
wget --with-debug --with-pcre: 2
wget --with-libmetalink --with-gpgme: 2
wget --with-pcre --with-gpgme: 2
wget --with-pcre --with-libmetalink: 2
==> install_on_request (90d)
wget: 290818
wget --with-debug: 157
wget --with-pcre --with-libmetalink --with-gpgme: 101
wget --with-pcre: 100
wget --with-debug --with-pcre --with-libmetalink --with-gpgme: 42
wget --with-pcre --with-libmetalink: 30
wget --HEAD: 13
wget --with-pcre --with-gpgme: 11
wget --with-gpgme: 10
wget --with-debug --with-pcre: 8
wget --with-libmetalink: 7
wget --HEAD --with-pcre --with-libmetalink --with-gpgme: 4
wget --with-debug --with-pcre --with-libmetalink: 4
==> install_on_request (365d)
wget: 1042845
wget --with-pcre: 504
wget --with-debug: 458
wget --with-pcre --with-libmetalink --with-gpgme: 432
wget --with-debug --with-pcre --with-libmetalink --with-gpgme: 201
wget --with-pcre --with-libmetalink: 158
wget --with-gpgme: 61
wget --HEAD: 54
wget --with-pcre --with-gpgme: 49
wget --with-debug --with-pcre: 47
wget --with-debug --with-pcre --with-libmetalink: 24
wget --with-libressl: 23
wget --with-libmetalink: 22
wget --with-libmetalink --with-gpgme: 20
==> build_error (30d)
wget: 9
wget --HEAD: 1
wget --with-debug: 1
  • 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 written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

@wafflebot wafflebot bot added the in progress label Sep 5, 2018

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:info-analytics branch from ed81369 to 19ba9df Sep 5, 2018

@colindean
Copy link
Member

colindean left a comment

LGTM but my refactor trigger finger is getting itchy on that lengthy method.


return if ENV["HOMEBREW_NO_ANALYTICS"]
return if ENV["HOMEBREW_NO_GITHUB_API"]
output, = curl_output("https://formulae.brew.sh/api/formula/#{f}.json")

This comment has been minimized.

@colindean

colindean Sep 5, 2018

Member

Should this be output, _ so it looks less like a typo and more like an intentionally ignored return component?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 5, 2018

Member

@colindean I don't think so but I have pulled it into another method 👍

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:info-analytics branch 2 times, most recently from f58d863 to 501235f Sep 5, 2018

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 5, 2018

Decided to provide longer output for HOMEBREW_DEVELOPERs to replace brew formula-analytics for most usage. CC @Homebrew/maintainers for your thoughts on this command and the output.

@zbeekman

This comment has been minimized.

Copy link
Member

zbeekman commented Sep 5, 2018

I love the new analytics displayed by default with brew info.

I'm a bit wary of the verbosity when HOMEBREW_MAINTAINER=1 as I use brew info with great frequency and typically when comparing information from different packages. I'll have to change my workflow to not have HOMEBREW_MAINTAINER set by default if it will print this level of information for packages, and then turn it on when doing Homebrew work. Not a big deal, but I personally would prefer to not get the verbose analytics from brew info by default when HOMEBREW_MAINTAINER is set.

Just my $0.02, FWIW (not much 😄)

@fxcoudert

This comment has been minimized.

Copy link
Member

fxcoudert commented Sep 5, 2018

I'm don't particularly like the idea, sorry… We have two separate existing commands, which work well, and I am not sure people often use both at the same time (I know I don't).

  • For non-developers, I think the output is not particularly easy to read. I'm not sure either whether they actually need it, or what's the expected use case.
  • brew info currently runs in less than a second on my laptop, but brew formula-analytics can take several seconds. Also, brew info is (AFAIU) purely local, and I don't always have net access (or I have slow access): does it mean I have to wait for network timeout, or for slow network, for every brew info?
  • Developer info: why not, but it can be very long. And not as readable as brew formula-analytics output.
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 5, 2018

Not a big deal, but I personally would prefer to not get the verbose analytics from brew info by default when HOMEBREW_MAINTAINER is set.

How about if it was just with --analytics set? Alternatively, --no-analytics may make this a bit more discoverable for at least the default, non-maintainer values.

brew info currently runs in less than a second on my laptop, but brew formula-analytics can take several seconds.

This is dramatically faster as it's hitting a static file on a CDN rather than an API which is doing a real-time datastore query.

does it mean I have to wait for network timeout, or for slow network, for every brew info?

I would be surprised if the 4K file for e.g. wget (which is one of the larger ones) would take long even on a (very) slow network. In that situation you could set one of the variables that disables it, though.

And not as readable as brew formula-analytics output.

Would it be more useful if it was as readable as that output? That could be arranged.

@colindean

This comment has been minimized.

Copy link
Member

colindean commented Sep 5, 2018

does it mean I have to wait for network timeout, or for slow network, for every brew info?

Could the timeout for this fetch be set to something absurdly low, like 1 sec, so that it won't ever take more than that? Favoring speed of the core command over the completion of the remote request seems like a potential compromise.

Additionally, is there an opportunity for concurrency here? Kick off this analytics request at the start of the info_formula method and then get its value when it's time to display it, provided that it's available by then?

@sjackman

This comment has been minimized.

Copy link
Contributor

sjackman commented Sep 5, 2018

I agree with @zbeekman about the verbosity when HOMEBREW_DEVELOPER is set (as I always have it set). Perhaps brew info foo -v for verbose output.

@chdiza

This comment has been minimized.

Copy link
Contributor

chdiza commented Sep 5, 2018

does it mean I have to wait for network timeout, or for slow network, for every brew info?

I'm also worried about this.

end

def output_analytics(f)
return if ENV["HOMEBREW_NO_ANALYTICS"]

This comment has been minimized.

@javian

javian Sep 5, 2018

Member

Should we automatically exclude the people that opt out of submitting analytics data ? Getting access to the data might not be equivalent to not wanting to share your own.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 6, 2018

Could the timeout for this fetch be set to something absurdly low, like 1 sec, so that it won't ever take more than that? Favoring speed of the core command over the completion of the remote request seems like a potential compromise.

This seems like a good idea.

Perhaps brew info foo -v for verbose output.

👍

Should we automatically exclude the people that opt out of submitting analytics data ? Getting access to the data might not be equivalent to not wanting to share your own.

@javian Yeh, I don't think they are equivalent but I think it's reasonable to say if you're not willing to share we're not going to make it easy for you to access the fruits of other people's sharing.

cmd/info: display analytics data.
When users don't have `HOMEBREW_NO_ANALYTICS` or
`HOMEBREW_NO_GITHUB_API` set let's display some analytics data in
`brew info`. This should be useful for both maintainers and for users of
Homebrew.

Note this by default combines all installs across options for a single
number; for formulae with lots of options it's a bit overwhelming to
print the installs per-option. However, for `HOMEBREW_DEVELOPER`s print
the full output.

Sample non-developer output:

```console
$ brew info wget
wget: stable 1.19.5 (bottled), HEAD
Internet file retriever
https://www.gnu.org/software/wget/
/usr/local/Cellar/wget/1.19.5 (49 files, 3.7MB) *
  Built from source on 2018-09-03 at 20:46:32
From: https://github.com/Homebrew/homebrew-core/blob/master/Formula/wget.rb
==> Dependencies
Build: pkg-config ✔
Required: libidn2 ✔, openssl ✔
Optional: pcre ✔, libmetalink ✘, gpgme ✘
==> Options
--with-debug
	Build with debug support
--with-gpgme
	Build with gpgme support
--with-libmetalink
	Build with libmetalink support
--with-pcre
	Build with pcre support
--HEAD
	Install HEAD version
==> Analytics
install: 84638 (30d), 353800 (90d), 1372775 (365d)
install_on_request: 77926 (30d), 291305 (90d), 1044898 (365d)
build_error: 11 (30d)
```

Sample developer output:
```console
$ brew info wget
wget: stable 1.19.5 (bottled), HEAD
Internet file retriever
https://www.gnu.org/software/wget/
/usr/local/Cellar/wget/1.19.5 (49 files, 3.7MB) *
  Built from source on 2018-09-03 at 20:46:32
From: https://github.com/Homebrew/homebrew-core/blob/master/Formula/wget.rb
==> Dependencies
Build: pkg-config ✔
Required: libidn2 ✔, openssl ✔
Optional: pcre ✔, libmetalink ✘, gpgme ✘
==> Options
--with-debug
	Build with debug support
--with-gpgme
	Build with gpgme support
--with-libmetalink
	Build with libmetalink support
--with-pcre
	Build with pcre support
--HEAD
	Install HEAD version
==> Analytics
==> install (30d)
wget: 84516
wget --with-debug: 51
wget --with-libressl: 16
wget --with-pcre: 14
wget --with-pcre --with-libmetalink --with-gpgme: 12
wget --with-debug --with-pcre --with-libmetalink --with-gpgme: 8
wget --HEAD: 3
wget --HEAD --with-debug --with-libmetalink --with-gpgme: 3
wget --with-gpgme: 3
wget --with-libmetalink: 3
wget --with-pcre --with-libmetalink: 3
wget --with-debug --with-pcre: 2
wget --with-libmetalink --with-gpgme: 2
wget --with-pcre --with-gpgme: 2
==> install (90d)
wget: 353131
wget --with-debug: 188
wget --with-pcre: 138
wget --with-pcre --with-libmetalink --with-gpgme: 118
wget --with-libressl: 81
wget --with-debug --with-pcre --with-libmetalink --with-gpgme: 47
wget --with-pcre --with-libmetalink: 31
wget --HEAD: 13
wget --with-pcre --with-gpgme: 12
wget --with-gpgme: 11
wget --with-debug --with-pcre: 10
wget --with-libmetalink: 8
wget --HEAD --with-pcre --with-libmetalink --with-gpgme: 4
wget --with-debug --with-pcre --with-libmetalink: 4
wget --with-libmetalink --with-gpgme: 4
==> install (365d)
wget: 1369530
wget --with-pcre: 810
wget --with-debug: 649
wget --with-pcre --with-libmetalink --with-gpgme: 554
wget --with-libressl: 479
wget --with-debug --with-pcre --with-libmetalink --with-gpgme: 235
wget --with-pcre --with-libmetalink: 184
wget --with-gpgme: 67
wget --with-pcre --with-gpgme: 67
wget --with-debug --with-pcre: 65
wget --HEAD: 54
wget --with-libmetalink: 30
wget --with-libmetalink --with-gpgme: 27
wget --with-debug --with-pcre --with-libmetalink: 24
==> install_on_request (30d)
wget: 77827
wget --with-debug: 48
wget --with-pcre: 12
wget --with-pcre --with-libmetalink --with-gpgme: 11
wget --with-debug --with-pcre --with-libmetalink --with-gpgme: 8
wget --HEAD: 3
wget --HEAD --with-debug --with-libmetalink --with-gpgme: 3
wget --with-gpgme: 3
wget --with-libmetalink: 3
wget --with-debug --with-pcre: 2
wget --with-libmetalink --with-gpgme: 2
wget --with-pcre --with-gpgme: 2
wget --with-pcre --with-libmetalink: 2
==> install_on_request (90d)
wget: 290818
wget --with-debug: 157
wget --with-pcre --with-libmetalink --with-gpgme: 101
wget --with-pcre: 100
wget --with-debug --with-pcre --with-libmetalink --with-gpgme: 42
wget --with-pcre --with-libmetalink: 30
wget --HEAD: 13
wget --with-pcre --with-gpgme: 11
wget --with-gpgme: 10
wget --with-debug --with-pcre: 8
wget --with-libmetalink: 7
wget --HEAD --with-pcre --with-libmetalink --with-gpgme: 4
wget --with-debug --with-pcre --with-libmetalink: 4
==> install_on_request (365d)
wget: 1042845
wget --with-pcre: 504
wget --with-debug: 458
wget --with-pcre --with-libmetalink --with-gpgme: 432
wget --with-debug --with-pcre --with-libmetalink --with-gpgme: 201
wget --with-pcre --with-libmetalink: 158
wget --with-gpgme: 61
wget --HEAD: 54
wget --with-pcre --with-gpgme: 49
wget --with-debug --with-pcre: 47
wget --with-debug --with-pcre --with-libmetalink: 24
wget --with-libressl: 23
wget --with-libmetalink: 22
wget --with-libmetalink --with-gpgme: 20
==> build_error (30d)
wget: 9
wget --HEAD: 1
wget --with-debug: 1
```

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:info-analytics branch from 501235f to c60fe60 Sep 6, 2018

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 6, 2018

And not as readable as brew formula-analytics output.

@fxcoudert Also, to be more explicit, not as part of this PR but I am hoping to have it so maintainers do not need to run brew formula-analytics directly and it can just be used by bots to generate the data.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 6, 2018

I've added a 3 second timeout and made this require --verbose to get the full output. If you find issues with this or it's still too sluggish: shout here or in Slack and I'll address it. Thanks for the feedback folks 👍

@MikeMcQuaid MikeMcQuaid merged commit 3406162 into Homebrew:master Sep 6, 2018

2 of 3 checks passed

codecov/patch 47.83% of diff hit (target 71.34%)
Details
codecov/project 71.32% (-0.01%) compared to 6fe61a0
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in progress label Sep 6, 2018

@MikeMcQuaid MikeMcQuaid deleted the MikeMcQuaid:info-analytics branch Sep 6, 2018

@sjackman

This comment has been minimized.

Copy link
Contributor

sjackman commented Sep 6, 2018

I'm loving this feature. Thanks, Mike!

Looks like this PR doesn't currently report analytics for taps, though the analytics data is available for taps (https://formulae.brew.sh/analytics/). Was that a design choice?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 7, 2018

@sjackman Unfortunately the way the analytics data for taps and the API is structured means that it's too hard to obtain that information without querying all of the analytics data for a given time period (which is a considerably larger payload). That could be something that brew info -v does, though, if you're interested on giving it a go? It might be a bit weird that the data won't be present unless the formula is popular enough, though.

@sjackman

This comment has been minimized.

Copy link
Contributor

sjackman commented Sep 7, 2018

I'll put that feature on my radar. It does interest me.

@lock lock bot added the outdated label Oct 7, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Oct 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.