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 telemetry to the Chrome extension #7370

Merged
merged 1 commit into from
Jun 3, 2016

Conversation

Rob--W
Copy link
Member

@Rob--W Rob--W commented May 29, 2016

This PR adds telemetry to the Chrome extension - see #7312. To make sure that this system behaves exactly as expected (i.e. not too much logging, and not too few logging), I've added an extensive set of unit tests at both the extension and the server. Just as a sanity check, I've also included a manual test at the bottom. I've successfully tested it in 51.0.2704.63 and 34.0.1847.137.

Privacy policy: https://github.com/Rob--W/pdfjs-telemetry#privacy-policy

Unit tests (offline):

node test/chromium/test-telemetry.js

Server tests (requires that Nginx is installed):

git clone https://github.com/Rob--W/pdfjs-telemetry.git
cd pdfjs-telemetry/
python testserver.py TestHttp TestHttps

Integration test (extension + server):

  • Build the extension
  • Edit build/chromium/telemetry.js and remove the check for
    chrome.runtime.id.
  • Start Chrome (preferably a new profile):
    chromium --user-data-dir=/tmp/pdftest --no-first-run
  • Open chrome://net-internals#events
  • Visit chrome://extensions and enable Developer mode.
  • Load unpacked extension, select build/chromium.
  • Go to the chrome://net-internals tab and filter on pdfjs.robwu.nl.
  • Click on URL_REQUEST and verify that the server replied with 204.
  • Reload the extension.
  • Verify that chrome://net-internals did not contain a new log request.

/cc @timvandermeij


This change is Reviewable

// Another hour passes and the request should not be rate-limited any more.
window.Date.test_now_value += 1 * 36E5;
telemetryScript.runInNewContext(window);
// Only one request should be sent because of rate-limiting.
Copy link
Member Author

Choose a reason for hiding this comment

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

I'll remove this comment (copy-paste error).

@yurydelendik
Copy link
Contributor

Looks good from client side. A server functionality was not checked, but per IRC conversation installed locally extension also pings main server. Good to go after nits addressed.

Previously, Rob--W (Rob Wu) wrote…

Add opt-out telemetry to the Chrome extension

This PR adds telemetry to the Chrome extension - see #7312. To make sure that this system behaves exactly as expected (i.e. not too much logging, and not too few logging), I've added an extensive set of unit tests at both the extension and the server. Just as a sanity check, I've also included a manual test at the bottom. I've successfully tested it in 51.0.2704.63 and 34.0.1847.137.

Privacy policy: https://github.com/Rob--W/pdfjs-telemetry#privacy-policy

Unit tests (offline):


node test/chromium/test-telemetry.js

Server tests (requires that Nginx is installed):


git clone https://github.com/Rob--W/pdfjs-telemetry.git

cd pdfjs-telemetry/

python testserver.py TestHttp TestHttps

Integration test (extension + server):

  • Build the extension

  • Edit build/chromium/telemetry.js and remove the check for

    chrome.runtime.id.

  • Start Chrome (preferably a new profile):

    chromium --user-data-dir=/tmp/pdftest --no-first-run

  • Open chrome://net-internals#events

  • Visit chrome://extensions and enable Developer mode.

  • Load unpacked extension, select build/chromium.

  • Go to the chrome://net-internals tab and filter on pdfjs.robwu.nl.

  • Click on URL_REQUEST and verify that the server replied with 204.

  • Reload the extension.

  • Verify that chrome://net-internals did not contain a new log request.

/cc @timvandermeij


Reviewed 5 of 5 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


test/chromium/test-telemetry.js, line 231 [r1] (raw file):

      'Extension-Version': '1.0.0',
    }]);

Nit: rm spaces


Comments from Reviewable

Privacy policy: https://github.com/Rob--W/pdfjs-telemetry#privacy-policy

Unit tests (offline):

```
node test/chromium/test-telemetry.js
```

Server tests (requires that Nginx is installed):

```
git clone https://github.com/Rob--W/pdfjs-telemetry.git
cd pdfjs-telemetry/
python testserver.py TestHttp TestHttps
```

Integration test (extension + server):

- Build the extension
- Edit build/chromium/telemetry.js and remove the check for
  chrome.runtime.id.
- Start Chrome (preferably a new profile):
  chromium --user-data-dir=/tmp/pdftest --no-first-run
- Open chrome://net-internals#events
- Visit chrome://extensions and enable Developer mode.
- Load unpacked extension, select build/chromium.
- Go to the chrome://net-internals tab and filter on pdfjs.robwu.nl.
- Click on URL_REQUEST and verify that the server replied with 204.
- Reload the extension.
- Verify that chrome://net-internals did not contain a new log request.
@Rob--W Rob--W force-pushed the crx-telemetry-7312 branch from 2f2f560 to 724308c Compare June 3, 2016 18:37
@yurydelendik yurydelendik merged commit f0585f5 into mozilla:master Jun 3, 2016
@yurydelendik
Copy link
Contributor

Thank you for the patch

Rob--W added a commit to Rob--W/pdf.js that referenced this pull request Jun 3, 2016
Rob--W added a commit that referenced this pull request Jun 3, 2016
Fix typo in telemetry.js, follow-up to #7370
@Rob--W Rob--W mentioned this pull request Aug 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants