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

Add analytics to cask installs #4620

Merged
merged 10 commits into from Aug 18, 2018

Conversation

Projects
None yet
8 participants
@brianmorton
Copy link
Contributor

brianmorton commented Aug 6, 2018

  • 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?

per Homebrew/homebrew-cask#47720

seems odd that it might be this simple... I welcome all feedback and help.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 6, 2018

I can confirm this will work as expected but I'd suggest @Homebrew/cask give final sign-off. I'd also suggest adding a build error report, too, so casks that are broken can be identified.

@brianmorton

This comment has been minimized.

Copy link
Contributor

brianmorton commented Aug 6, 2018

Thanks for the feedback @MikeMcQuaid , the HBC team's suggestions would be helpful.

First, let me say I am super inexperienced with this repo.

I looked at adding stuff for borked cask installs, however it seems because of the modular nature of most of the checks (shasum, downloads) that it would also be triggered by commands other than just failed installs, conflating numbers. The logic checks to see if it is borked is in the modules, so I can't see an elegant way to get around that. If there is a way to do this (or if just including bad numbers is acceptable) I would welcome the help.

I also looked at trying to include whether or not something was installed as a dependency (perhaps as cask_install_dependency?) but without explicit feedback on wanting this I was reluctant to include it form the get-go.

@brianmorton brianmorton referenced this pull request Aug 6, 2018

Closed

Analytics for casks #47720

@vitorgalvao

This comment has been minimized.

Copy link
Member

vitorgalvao commented Aug 6, 2018

I also looked at trying to include whether or not something was installed as a dependency

I don’t see that as necessary. If a cask has low download numbers the dependency installs will also be low; if a cask has high download numbers, the dependency needs to remain to support it. Dependency statistics will not be meaningful towards the goal of deciding what should be prioritised.

@commitay commitay added the cask label Aug 7, 2018

@brianmorton

This comment has been minimized.

Copy link
Contributor

brianmorton commented Aug 7, 2018

Any ideas around reporting for broken casks? Or shall we just skip, at least for now?

edit: I lean slightly towards including them for shasum and download errors... i think these commands would mostly be triggered by cask installs, and if triggered another way (such as audit) it would still be useful information. Thoughts?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 8, 2018

I think this is sufficient for now (or at least useful enough to be mergeable as-is).

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 8, 2018

@Homebrew/cask any objections to the code as-is being merged?

@claui

claui approved these changes Aug 8, 2018

Copy link
Member

claui left a comment

For good measure, a one-time user-facing message would have been nice (for perfect transparency, taking into account the public sensitivity for the topic and the popular misconception of anonymous analytics == tracking my every move).

With that being said, adding this particular trigger seems like more of a bugfix to me because it should have been there all along.

LGTM as is.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 8, 2018

For good measure, a one-time user-facing message would have been nice (for perfect transparency, taking into account the public sensitivity for the topic and the popular misconception of anonymous analytics == tracking my every move).

Yes, agreed with this. I think it'd be good to put something that's displayed (once) on the first brew cask run after this PR has been run. That's what we did with our other analytics. Then again, we've told people for formulae so it may not be necessary.

@brianmorton

This comment has been minimized.

Copy link
Contributor

brianmorton commented Aug 8, 2018

If you point me to an example of how it was done in the past I would be happy to update the PR.

@scpeters

This comment has been minimized.

Copy link
Contributor

scpeters commented Aug 9, 2018

@brianmorton I think #143 contains some relevant code for printing a message about analytics during brew update

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 9, 2018

Yeh, I'd suggest something like homebrew.cask.analyticsmessage with the similar git config approach.

@brianmorton

This comment has been minimized.

Copy link
Contributor

brianmorton commented Aug 14, 2018

I believe I have added the message.

Not the most graceful integration, but I think it should work.

As always, feedback is welcome/encouraged.

ENV["HOMEBREW_NO_ANALYTICS_THIS_RUN"] = "1"
# Use the shell's audible bell.
print "\a"

# Use an extra newline and bold to avoid this being missed.
ohai "Homebrew has enabled anonymous aggregate user behaviour analytics."
ohai "Homebrew and Homebrew Cask have enabled anonymous aggregate user behaviour analytics."

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 15, 2018

Member

Homebrew (and Cask) would be nicer; we need something < 81 characters.

@@ -22,16 +22,23 @@ def update_report
HOMEBREW_REPOSITORY.cd do
analytics_message_displayed = \
Utils.popen_read("git", "config", "--local", "--get", "homebrew.analyticsmessage").chuzzle
cask_analytics_message_displayed = \
Utils.popen_read("git", "config", "--local", "--get", "homebrew.cask.analyticsmessage").chuzzle

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 15, 2018

Member

How about just caskanalyticsmessage

This comment has been minimized.

@brianmorton

brianmorton Aug 15, 2018

Contributor

Sure, I was just prepending the other variable with cask to keep some consistency.

@brianmorton

This comment has been minimized.

Copy link
Contributor

brianmorton commented Aug 15, 2018

I have updated per PR per the requested changes.

@@ -22,16 +22,23 @@ def update_report
HOMEBREW_REPOSITORY.cd do
analytics_message_displayed = \
Utils.popen_read("git", "config", "--local", "--get", "homebrew.analyticsmessage").chuzzle
caskanalyticsmessage = \
Utils.popen_read("git", "config", "--local", "--get", "homebrew.cask.analyticsmessage").chuzzle

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 15, 2018

Member

I'd suggest homebrew.caskanalyticsmessage here

This comment has been minimized.

@brianmorton

brianmorton Aug 15, 2018

Contributor

Earlier ( #4620 (comment) ) you suggested the current implementation. Could you confirm that you would now like it changed?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 15, 2018

Member

Yes, please, sorry. I wasn't familiar with what the existing keys were at the time.

@@ -41,6 +48,7 @@ def update_report
# Consider the message possibly missed if not a TTY.
if $stdout.tty?
safe_system "git", "config", "--local", "--replace-all", "homebrew.analyticsmessage", "true"
safe_system "git", "config", "--local", "--replace-all", "homebrew.cask.analyticsmessage", "true"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 15, 2018

Member

I'd suggest homebrew.caskanalyticsmessage here

This comment has been minimized.

@brianmorton

brianmorton Aug 15, 2018

Contributor

Earlier ( #4620 (comment) ) you suggested the current implementation. Could you confirm that you would now like it changed?

@claui
Copy link
Member

claui left a comment

One more thing.

@@ -89,6 +89,8 @@ def install
install_artifacts
enable_accessibility_access

Utils::Analytics.report_event("cask_install", @cask.token)

This comment has been minimized.

@claui

claui Aug 15, 2018

Member

This will fail because we’re in the Hbc module, and Hbc::Utils::Analytics doesn’t exist.

Write ::Utils::Analytics instead of Utils::Analytics to fix this.

This comment has been minimized.

@claui

claui Aug 15, 2018

Member

For a quick test case that reproduces the error reliably, paste this into your terminal:

(
  set -e
  cd "$(mktemp -d)"
  git clone -b hbc_analytics --depth 1 https://github.com/brianmorton/brew
  cd brew
  mkdir -p Library/Taps/homebrew/homebrew-{core,cask}
  cat > empty.rb <<< \
    'cask("empty") { version :latest; sha256 :no_check; url "file:///dev/null"; container type: :naked }'
  unset HOMEBREW_NO_ANALYTICS
  export HOMEBREW_NO_AUTO_UPDATE=1
  pwd
  bin/brew cask reinstall empty.rb
)

This results in:

Error: uninitialized constant Hbc::Utils::Analytics

@claui

claui approved these changes Aug 16, 2018

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Aug 16, 2018

This needs a rebase.

brianmorton and others added some commits Aug 6, 2018

@claui claui force-pushed the brianmorton:hbc_analytics branch from 73f5d00 to 61db35f Aug 16, 2018

@claui claui removed their assignment Aug 16, 2018

@brianmorton

This comment has been minimized.

Copy link
Contributor

brianmorton commented Aug 17, 2018

I tried a rebase but it got gnarly and is not my forte. I merged and it seems fine... let me know if there's anything else.

analytics_disabled =
Utils.popen_read("git", "config", "--local", "--get", "homebrew.analyticsdisabled").chuzzle
if analytics_message_displayed != "true" &&
caskanalyticsmessage != "true" &&

This comment has been minimized.

@reitermarkus

reitermarkus Aug 18, 2018

Member

Name this cask_analytics_message_displayed.

ENV["HOMEBREW_NO_ANALYTICS_THIS_RUN"] = "1"
# Use the shell's audible bell.
print "\a"

# Use an extra newline and bold to avoid this being missed.
ohai "Homebrew has enabled anonymous aggregate user behaviour analytics."
ohai "Homebrew (and Cask) have enabled anonymous aggregate user behaviour analytics."

This comment has been minimized.

@reitermarkus

reitermarkus Aug 18, 2018

Member

Should this stay “has” since “and Cask” is in parentheses?

This comment has been minimized.

@reitermarkus

reitermarkus Aug 18, 2018

Member

This feels weird anyways since everything is in the Homebrew org. Maybe “Homebrew has enabled anonymous aggregate user behaviour analytics for formulae and casks.”?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 18, 2018

Member

@reitermarkus That's > 80 characters

This comment has been minimized.

@reitermarkus

reitermarkus Aug 19, 2018

Member

That's exactly why ohai shouldn't truncate text.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 19, 2018

Member

@reitermarkus That's unrelated to ohai; even a puts that's wrapped by the terminal at >80 characters is uglier than one that's been done by humans (e.g. is it wrapped in the middle of a word or not),

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Aug 18, 2018

@MikeMcQuaid MikeMcQuaid changed the title add anlytics to cask installs Add analytics to cask installs Aug 18, 2018

MikeMcQuaid added some commits Aug 18, 2018

@MikeMcQuaid MikeMcQuaid merged commit bab5b02 into Homebrew:master Aug 18, 2018

2 of 3 checks passed

codecov/patch 33.33% of diff hit (target 71.17%)
Details
codecov/project 71.15% (-0.02%) compared to b8e276a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in progress label Aug 18, 2018

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 18, 2018

Thanks so much for your contribution! Without people like you submitting PRs we couldn't run this project. You rock, @brianmorton!

@brianmorton

This comment has been minimized.

Copy link
Contributor

brianmorton commented Aug 18, 2018

Now, for the whole point of this PR - how do I get to see these analytics?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 18, 2018

Only Homebrew maintainers have access to them. You would need to build something like https://github.com/homebrew/homebrew-formula-analytics and https://github.com/homebrew/formulae.brew.sh to upload the cask analytics publicly somewhere.

@brianmorton

This comment has been minimized.

Copy link
Contributor

brianmorton commented Aug 20, 2018

Can you all not just add a row or two to the table at https://formulae.brew.sh/analytics/ ?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 20, 2018

Can you all not just

https://cantyoujust.no 😉

What I've mentioned above are the requirements. It's not as simple as just adding a line because then it would need to be downloaded manually, entered manually and updated manually.

@brianmorton

This comment has been minimized.

Copy link
Contributor

brianmorton commented Aug 20, 2018

Forgive me, but since you seem to guard the analytics and workflow more closely than I would have thought, the process becomes somewhat opaque.

To me it would have seemed there was an automated process for providing the existing analytics. In that line of reasoning, since the cask analytics are feeding in to the same system, it would seem to trivial to simply output a table row.

Based on your linking of the homebrew-formula-analytics repo, I am now guessing that somebody manually runs a CLI command or series of commands and manually uploads a file to that repo/site?

Also, respectfully, I think it would be difficult for me to update brew-formula-analytics.rb since you have indicated the analytics are restricted.

To that end, I am happy to do what I can to move this along so there are some type of publicly accessible cask analytics.

Please remember that this is all tangential to my original purpose of having cask analytics available to guide automation efforts. I am happy to continue submitting PRs and Getting Things Done as long as there is light at the end of this tunnel.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 20, 2018

Based on your linking of the homebrew-formula-analytics repo, I am now guessing that somebody manually runs a CLI command or series of commands and manually uploads a file to that repo/site?

It's manually run for maintainers to query the values. More recently a job has been added to homebrew-core (https://github.com/Homebrew/homebrew-core/blob/master/.travis.yml) which commits the values to formulae.brew.sh.

Also, respectfully, I think it would be difficult for me to update brew-formula-analytics.rb since you have indicated the analytics are restricted.

This is tricky, I agree. If the @Homebrew/cask maintainers are happy with you being temporarily granted access to the Homebrew Cask analytics to build the necessary functionality I would be OK with that too. I would say that given the naming homebrew-formula-analytics and formulae.brew.sh may not entirely lend themselves to just having Casks bolted on but I defer that to whoever does the work.

@vitorgalvao

This comment has been minimized.

Copy link
Member

vitorgalvao commented Aug 20, 2018

If the @Homebrew/cask maintainers are happy with you being temporarily granted access to the Homebrew Cask analytics to build the necessary functionality I would be OK with that too.

I am.

@claui

This comment has been minimized.

Copy link
Member

claui commented Aug 21, 2018

I’d be fine with that, too.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 21, 2018

Cool, thanks folks. @brianmorton can you send me your Google account email address?

@brianmorton

This comment has been minimized.

Copy link
Contributor

brianmorton commented Aug 21, 2018

Sent.

@lock lock bot added the outdated label Sep 20, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Sep 20, 2018

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