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

Integrate metrics #1562

Merged
merged 24 commits into from Jul 9, 2018

Conversation

Projects
None yet
3 participants
@annthurium
Copy link
Contributor

annthurium commented Jul 3, 2018

Addresses #1546

This pull request adds the ability to send 3 types of usage metrics from the GitHub package to GitHub's internal analytics pipeline:

events (for whatever we want)
counters (for incrementing every time a thing happens)
timers (for latency events)

Approach

  • Added a ReporterProxy class. This is necessary because we can't use the real reporter until the metrics package has loaded, and there's no guarantee that will happen before the GitHub package loads.

  • Export functions to add metrics. Functions can be conveniently imported/used at any layer of our codebase.

  • Since we can put metrics instrumentation at multiple levels of the codebase, where should we track? It depends on the question you're trying to answer with the data.

    • if the question is "how many times is x button clicked?" instrument the UI.
    • If the question is "how many times is this action performed" instrument as close to the API as possible (where we shell out to git, where we call graphql.)

Metrics to track

  • All Git operations (excluding noisy ones that don't give us much info in terms of user behavior)
  • Co author functionality
  • Authentication requests made for the GitHub package
  • GitHub package version
  • Amend actions
  • opening/closing the Git / GitHub panes
  • create pull request actions

We came up with these during the editor tools mini summit, and i'm totally open to revisiting this list now.

Test plan

  • unit tests for reporterProxy
  • unit tests for all the places we are incrementing metrics
  • manually testing this is hard. We don't actually send any metrics to the back end in dev mode, so I can't look at network activity to verify. There isn't an interface exposed to look directly at what's in the StatsStore from the Github package, because we're calling it via a services interface. I put in some console.log statements to verify that the Reporter class is being called as I expect, when I perform actions that are instrumented.

Future work

  • instrument UI for partial staging.
  • measure latency for git actions
@annthurium

This comment has been minimized.

Copy link
Contributor Author

annthurium commented Jul 3, 2018

@smashwilson: I'd love some "30% feedback" on this WIP pull request.

Specifically:

  • Are there issues with this approach that I might not be aware of?
  • are there other metrics we should be tracking that are not on the proposed list?

@annthurium annthurium requested a review from smashwilson Jul 3, 2018

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 5, 2018

Coverage Status

Coverage increased (+4.5%) to 77.791% when pulling 348ec0d on tt-18-jul-integrate-telemetry into 380a503 on master.

@@ -473,7 +474,10 @@ export default class GitShellOutStrategy {

if (amend) { args.push('--amend'); }
if (allowEmpty) { args.push('--allow-empty'); }
return this.gpgExec(args, {writeOperation: true});
const returnValue = await this.gpgExec(args, {writeOperation: true});

This comment has been minimized.

@annthurium

annthurium Jul 5, 2018

Author Contributor

actually, the more I think about it, the more I think we should just log all the git commands in th exec function instead of instrumenting individual commands.

if (err) { reject(err); } else { resolve(); }
});
});
let returnPromise;

This comment has been minimized.

@annthurium

annthurium Jul 6, 2018

Author Contributor

@smashwilson, does this look right to you?

I was having trouble getting the inner promise to return for the test. Other than adding the counter, this should behave identically.

This comment has been minimized.

@smashwilson

smashwilson Jul 6, 2018

Member

It looks like openExternal uses a callback instead of returning a Promise, at least on macOS:

callback Function (optional) macOS - If specified will perform the open asynchronously.

(From what I can tell, on other platforms, the callback will be called synchronously instead.)

Returns Boolean - Whether an application was available to open the URL. If callback is specified, always returns true.

Try putting the incrementCounter() call inside the if statement instead:

return new Promise((resolve, reject) => {
  shell.openExternal(createPrUrl, {}, err => {
    if (err) {
      reject(err);
    } else {
      incrementCounter('create-pull-request');
      resolve();
    }
  });
});

This comment has been minimized.

@annthurium

annthurium Jul 6, 2018

Author Contributor

that was the first thing I tried, and the promise never returned in the test.

I also tried using a .done() callback in the test and the promise still did not return.

Lemmesee if I can figure out a different approach.

@smashwilson
Copy link
Member

smashwilson left a comment

I'm excited to get this in 😄

Are there issues with this approach that I might not be aware of?

Not that I can think of!

We'll have to check that it works properly with snapshotting... it should be fine because the global reporter state isn't depending on anything that can't be required in a snapshot, but it's always good to verify.

are there other metrics we should be tracking that are not on the proposed list?

Haha, I'm sure there are a bunch 😉 I suspect we'll have a long tail that we add over time as we think of them. I'd love to measure real-world performance, for example. But it would probably be best to add them incrementally as we have concrete hypotheses to test rather than just throwing everything against the wall and seeing what's useful after.

For our initial ship I'm most curious to see git and GitHub tab opening counts... we suspect we have discoverability issues and those would be our best indications of that, and the best way we'd know if we improve.

@@ -42,6 +43,9 @@ export class LargeRepoError extends Error {
}
}

// ignored for the purposes of usage metrics tracking because they're noisy
const IGNORED_GIT_COMMANDS = ['cat-file', 'config', 'diff', 'for-each-ref', 'log', 'rev-parse', 'status'];

This comment has been minimized.

@smashwilson

smashwilson Jul 6, 2018

Member

Would it be more reliable to list the commands we do want to capture?

This comment has been minimized.

@annthurium

annthurium Jul 6, 2018

Author Contributor

Good question. My thinking on this was that

  1. I don't really know which commands are important for understanding user behavior. So logging all of them except really obviously noisy ones avoids having to be more specific about what we'd like to learn from this. Which could be good or bad depending on how you look at it.

  2. Using an "exclude" list rather than an "include" list means that we don't have to worry about forgetting to update the list when we add support for new git commands. (Although obviously if we add new noisy commands, we'd still need to update.)

@@ -89,6 +93,7 @@ export default class GitShellOutStrategy {
async exec(args, options = GitShellOutStrategy.defaultExecArgs) {
/* eslint-disable no-console,no-control-regex */
const {stdin, useGitPromptServer, useGpgWrapper, useGpgAtomPrompt, writeOperation} = options;
const commandName = args[0];

This comment has been minimized.

@smashwilson
@@ -473,7 +482,7 @@ export default class GitShellOutStrategy {

if (amend) { args.push('--amend'); }
if (allowEmpty) { args.push('--allow-empty'); }
return this.gpgExec(args, {writeOperation: true});
return await this.gpgExec(args, {writeOperation: true});

This comment has been minimized.

@smashwilson

smashwilson Jul 6, 2018

Member

☝️ You shouldn't need this await.

This comment has been minimized.

@annthurium

annthurium Jul 6, 2018

Author Contributor

👍 oops. fixed.

// assume that the metrics package will load before the GitHub package.

// maybe this should be an object instead of a class.
// IDK.

This comment has been minimized.

@smashwilson

smashwilson Jul 6, 2018

Member

Haha, this is fine. One advantage of having a class - you can export it and test its internals independent of the global state.


export const reporterProxy = new ReporterProxy();

export const incrementCounter = function(counterName) {

This comment has been minimized.

@smashwilson

smashwilson Jul 6, 2018

Member

Any particular reason for the export const name = function() { ... } style? Elsewhere we're just doing export function name() { ... }, but this being JavaScript it's quite possible I'm missing some subtleties about the value of this or something between declaration styles 🤔

This comment has been minimized.

@annthurium

annthurium Jul 6, 2018

Author Contributor

I wasn't sure if export function name() { .. } retained the execution context of the module where the function was defined, which is necessary because I'm using closures. but I suppose I could bind if I need to. Consistency is good, I'll try to switch to the other style. Thanks!

}

// function that is called after the reporter is actually loaded, to
// set the reporter and send any data that have accumulated while it was loading.

This comment has been minimized.

@smashwilson

smashwilson Jul 6, 2018

Member

One question: what happens if the metrics package is disabled and this is never called? It'd be good not to endlessly accumulate events and slowly eat RAM without bound... but I'm not sure how we could know that this method won't be called.

This comment has been minimized.

@annthurium

annthurium Jul 6, 2018

Author Contributor

oh, gooooooood question. Hadn't thought about that edge case.

I guess I could set a timer, and if the metrics package fails to be activated within a reasonable timeframe, cast the events into the void? I can't think of a better way to handle this but I'm open to suggestions.

Do you know how common it is for users to straight up disable core packages like that? Now I'm curious.

@@ -131,6 +132,28 @@ describe('RemoteContainer', function() {
assert.strictEqual(qev.prop('error'), e);
});

it('increments a counter on login', function() {
const token = '1234';
sinon.stub(model, 'getScopes').resolves(GithubLoginModel.REQUIRED_SCOPES);

This comment has been minimized.

@smashwilson

smashwilson Jul 6, 2018

Member

Hmm I should probably add this as a helper or something - you have to stub it every time any test needs to touch the GithubLoginModel, which is annoying.


const wrapper = mount(buildApp());
wrapper.instance().handleLogin(token);
assert.isTrue(incrementCounterStub.calledOnceWith('github-login'));

This comment has been minimized.

@smashwilson

smashwilson Jul 6, 2018

Member

Reminder to self: look up how to set up Sinon with Chai expectations again so we can (hopefully) get more informative failure messages on these.

This comment has been minimized.

@annthurium

annthurium Jul 6, 2018

Author Contributor

jah, seems like a good issue for the cleanup sprint.

if (err) { reject(err); } else { resolve(); }
});
});
let returnPromise;

This comment has been minimized.

@smashwilson

smashwilson Jul 6, 2018

Member

It looks like openExternal uses a callback instead of returning a Promise, at least on macOS:

callback Function (optional) macOS - If specified will perform the open asynchronously.

(From what I can tell, on other platforms, the callback will be called synchronously instead.)

Returns Boolean - Whether an application was available to open the URL. If callback is specified, always returns true.

Try putting the incrementCounter() call inside the if statement instead:

return new Promise((resolve, reject) => {
  shell.openExternal(createPrUrl, {}, err => {
    if (err) {
      reject(err);
    } else {
      incrementCounter('create-pull-request');
      resolve();
    }
  });
});
@@ -49,6 +51,29 @@ describe('RemoteController', function() {
);
}

it('increments a counter when onCreatePr is called', async function() {
const wrapper = shallow(createApp());
sinon.stub(shell, 'openExternal').resolves();

This comment has been minimized.

@smashwilson

smashwilson Jul 6, 2018

Member

Ah, this is the bit where you were having trouble getting the Promise to resolve?

Because the real shell.openExternal doesn't return a Promise, you might have better luck with:

sinon.stub(shell, 'openExternal').callsArg(2);

And to report an error:

sinon.stub(shell, 'openExternal').callsArgWith(2, new Error('boom'));

This comment has been minimized.

@annthurium

annthurium Jul 6, 2018

Author Contributor

ah thanks, I'll try that!

annthurium and others added some commits Jul 6, 2018

🔥 unnecessary `await`
Co-Authored-By: Ash Wilson <smashwilson@gmail.com>
fix `onCreatePr` so that it returns a promise properly.
Co-Authored-By: Ash Wilson <smashwilson@gmail.com>
handle case where metrics package never loads
Co-Authored-By: Ash Wilson <smashwilson@gmail.com>
export functions in a more consistent style
Co-Authored-By: Ash Wilson <smashwilson@gmail.com>

@annthurium annthurium changed the title Integrate metrics [wip] Integrate metrics Jul 6, 2018

@annthurium annthurium merged commit babf618 into master Jul 9, 2018

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+4.5%) to 77.791%
Details

@annthurium annthurium deleted the tt-18-jul-integrate-telemetry branch Jul 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.