Skip to content
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 opt-out analytics to CLI (implements #108) #187

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Add opt-out analytics to CLI (implements #108) #187

wants to merge 8 commits into from

Conversation

johnmccabe
Copy link
Contributor

@johnmccabe johnmccabe commented Oct 24, 2017

Description

This PR adds an opt-out analytics capture using Google Analytics.

Refer to the included analytics.md file for details of what is being captured at this point and how to opt out from the capture.

Note it does not capture function counts etc, pending the merge of openfaas/faas#315.

@alexellis - you will need to replace the trackingID and applicationName in analytics/google_analytics.go in order to direct events to a GA account associated with the OpenFaaS project.

analytics

Motivation and Context

How Has This Been Tested?

Have tested the resulting client as follows:

  • on OSX and Windows
  • unresponsive analytics endpoint results in timeout
  • closed endpoint results in a fail fast
  • opt-out envvar short circuits all capture
  • blocked endpoint (PiHole) fails fast
  • events are visible live in the Real-Time/Events dashboard
  • Data Studio Dashboard created from captured event data
  • Message displayed to stderr the first time the cli is run with analytics enabled

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@johnmccabe johnmccabe changed the title Add Opt-out analytics to CLI (implements #108) Add opt-out analytics to CLI (implements #108) Oct 24, 2017
@open-derek
Copy link

open-derek bot commented Oct 24, 2017

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off.
That's something we need before your Pull Request can be merged. Please see our contributing guide.

@open-derek open-derek bot added no-dco and removed no-dco labels Oct 24, 2017
@johnmccabe
Copy link
Contributor Author

@alexellis do you want the steps to setup the Custom Dimensions, application, trackingID and Dashboard in GA added to these docs or stored elsewhere?

@alexellis
Copy link
Member

This is brilliant 🥇

@johnmccabe
Copy link
Contributor Author

@alexellis ignoring the rebase, is there more you need to see here


Due to the speed with which most OpenFaaS API calls return, a timeout of 300 milliseconds is used to give the analytics submission time to complete.

## Opting out?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you link me to what brew/ VSCode says?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


If you wish to opt out of submitting analytics events you may do so by setting the `OPEN_FAAS_TELEMETRY` environment variable.

export OPEN_FAAS_TELEMETRY=0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our NS should be OPENFAAS_

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see where it was changed? The code review doesn't say "see outdated".. did you push yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed pushing the README it seems.

Language: language,
OS: runtime.GOOS,
ARCH: runtime.GOARCH,
EventCategory: "cli-success",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get whether /.dockerenv exists? Tells you whether you're in a container or not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/.dockerenv will tell us whether the environment is a Docker container.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm adding this at the moment, just need to setup the GA custom dimension and Google Data Suite dashboard to validate it's getting pushed through.

func (u UserSession) PostEvent(ch chan int) {
v, _ := query.Values(u)
req := &http.Request{
Method: "POST",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

http.MethodPost

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@johnmccabe
Copy link
Contributor Author

I've started addressing your comments @alexellis will get something pushed up ASAP, been a bit off the radar recently as you're likely aware.

This commit implements #108 adding an opt-out analytics capture using
Google Analytics.

Refer to the analytics.md file for details of what is being captured at
this point and how to opt out from the capture.

*Note* it does not capture function counts etc, pending the merge of
openfaas/faas#315.

@alexellis - you will need to replace the `trackingID` and
`applicationName` in `analytics/google_analytics.go` in order to direct
events to a GA account associated with the OpenFaaS project.

Signed-off-by: John McCabe <[email protected]>
Signed-off-by: John McCabe <[email protected]>
@@ -14,7 +14,7 @@ RUN test -z "$(gofmt -l $(find . -type f -name '*.go' -not -path "./vendor/*"))"
# ldflags -X injects commit version into binary

RUN license-check -path ./ --verbose=false \
&& go test $(go list ./... | grep -v /vendor/ | grep -v /template/|grep -v /build/) -cover \
&& OPEN_FAAS_TELEMETRY=0 go test $(go list ./... | grep -v /vendor/ | grep -v /template/|grep -v /build/) -cover \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it easier to put this as an ENV just above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, could do.

ApplicationName string `url:"an"`
ApplicationVersion string `url:"av"`
AnonymizeIP int `url:"aip"`
Language string `url:"cd1"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cd1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Custom Dimension 1, its a Google Analytics term, the backend GA setup doc I shared should explain.

AnonymizeIP: 1,
Language: language,
OS: runtime.GOOS,
ARCH: runtime.GOARCH,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a thought this may mis-report I think it's set at build-time. One thing it won't do is to show armv6/7/8 differences. For that an OS library like pstate etc might help - although we'd need one that doesn't use CGO.

@johnmccabe
Copy link
Contributor Author

Which part do you think is set at build? The runtime.* calls?

@alexellis
Copy link
Member

I'll double check but I thought it was hard-coded through the build-time runtime when cross-compiling. It didn't seem to have the ARM version for sure.

@johnmccabe
Copy link
Contributor Author

@alexellis is this worth you want to see implemented? I think it should be ok from a GDPR point of view, would need to add a privacy section to the docs however.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants