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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add metrics collection for committing partial changes, undoing the last commit, and opening an expanded commit msg editor #1685

Merged
merged 13 commits into from Sep 11, 2018

Conversation

@kuychaco
Copy link
Member

@kuychaco kuychaco commented Sep 8, 2018

Description of the Change

This PR introduces some of the additional metrics discussed in #1591.

So far I've added events for the following:

  • undo last commit
    • I use this feature A LOT. would be interesting to see how frequently others use it.
  • commit, with metadata indicating:
    • if the commit included partial changes (that is, unstaged changes remained uncommitted in the working directory)
      • are people taking advantage of how easy it is to stage/unstage subsets of changes in Atom? what percentage of total commits commit partial changes? As we make changes to our diff view and implement other discoverability/usability improvements, how will this be affected?
    • the number of co-authors for the commit
      • what percentage of users are committing with co-authors? as we ship more features that promote collaboration how does this change?

Other thoughts and considerations:

Expanded commit message editor usage
I considered using the "verbatim" option passed to Repository#commit to report whether the commit was made from an expanded commit editor or the small commit box in the Git Panel because I know that is what dictates its value, but it seemed icky to be relying on that inference. It also felt not great to pass down an additional option such as expandedCommitMessageEditor to the repository model strictly for collecting metrics on it. If we decide this is something we really want to instrument, it's better and more direct to do so where the expanded editor is opened (such as CommitController#openCommitMessageEditor).

Gathering data on button use vs. keyboard shortcuts vs. context menu use
It could be interesting and useful to gather data on how users are initiating operations in our package. This info could yield insights about how discoverable each of these entry points are. That said, the way that keybindings and context-menus are set up in Atom is based on commands which result in functions being called. We can easily instrument the function calls in this package as well as button presses, but I don't think there's an easy/straight-forward way to instrument the keyboard shortcut or context menu usage. @smashwilson @annthurium 馃挱s?

Alternate Designs

To gather information on partial-staging I considered tracking staging/unstaging events. At the end of the day though, I think what we care most about is how frequently users are taking advantage of the fact that you can easily make commits with partial changes using our UI. Tracking all of the staging/unstaging events seems like it would be a lot of noise with little additional value. Open to thoughts and suggestions though!

Benefits

We'll have interesting insights into user behavior, and be able to ask/answer questions such as those above in italics. This information could potentially help us identify UX issues, inform project decisions and priorities, and track the impact of future work and features introduced.

Possible Drawbacks

This PR simply introduces additional metrics collection. No drawbacks that I can think of.

Applicable Issues

#1591

TODO:

  • Possibly instrument commit editor expansion
@kuychaco kuychaco requested a review from annthurium Sep 8, 2018
@coveralls
Copy link

@coveralls coveralls commented Sep 8, 2018

Coverage Status

Coverage increased (+0.08%) to 80.245% when pulling ceb99a3 on ku-mo-metrics into 51dac31 on master.

Copy link
Contributor

@annthurium annthurium left a comment

overall, looks good! Thanks for diving in on this. Left a few comments in the code.

const unstagedCount = Object.keys({...unstagedFiles, ...mergeConflictFiles}).length;
addEvent('commit', {
package: 'github',
partial: unstagedCount > 0,

This comment has been minimized.

@annthurium

annthurium Sep 10, 2018
Contributor

Since there are several UI options for partial staging, it would be interesting to instrument those as well. Not something you have to implement here, just something to consider for future iterations.

This comment has been minimized.

@kuychaco

kuychaco Sep 10, 2018
Author Member

Yeah, I had that thought but then a couple of considerations caused me to pause. Maybe you have thoughts that would help with un-pausing :).

Do you have specific questions you'd like answered by this information or examples of conclusions we might be able to draw?

These were the considerations that caused me to pause initially:

  • as mentioned in the description, tracking all of the staging/unstaging events seems like it could be a lot of noise with little additional value. For example, keyboard users might stage files by hitting enter a bunch of times for each line/hunk/file they want to stage... so for a given partial commit, there could be a ton of staging events. Maybe this could provide valuable information, but nothing came to mind for me immediately
  • the distinction between partial staging as opposed to staging everything can't necessarily be inferred from instrumenting the UI controls. For example, clicking the "Stage selection" button could result in partial staging of a file, or it could result in staging all changes if the user has selected all of the changes before clicking the button.

馃挱 welcome if you have any!

This comment has been minimized.

@annthurium

annthurium Sep 10, 2018
Contributor

I'm curious about how users are staging files. Our file staging is extremely complex because we have many ways of performing the same action. For example, we have stage and unstage all both in the staged / unstaged changes pane and in the diff view for individual files. Ditto for hunk/line staging and unstaging.

If all of these are heavily used, great. If not, we should consider simplifying so we don't have so many different ways of performing the same operation.

This comment has been minimized.

@smashwilson

smashwilson Sep 11, 2018
Member

For example, keyboard users might stage files by hitting enter a bunch of times for each line/hunk/file they want to stage... so for a given partial commit, there could be a ton of staging events.

We'd almost have to track a flag locally (somewhere in package state... ?) that recorded a bunch of aggregate information about a commit while it's being staged, then increment actual metrics when the commit is finalized here based on their final states.

For example, when a user clicks "Stage Selection" in the FilePatchItem, we know if the file still includes unstaged changes or not, so we can update a "commit includes partial file" flag to true or false; then, at commit time, we check the flag and send a partiallyStagedFile property on the final event.

I have no idea where those flags should live, though - we don't really have a model yet for "the commit under construction." Also the FilePatchItem is about to change drastically so... 馃檭

If all of these are heavily used, great. If not, we should consider simplifying so we don't have so many different ways of performing the same operation.

Yeah, that's a great point. I'm not sure how we'd do that, though 馃

@@ -344,6 +355,7 @@ export default class Present extends State {
async () => {
try {
await this.git().reset('soft', 'HEAD~');
addEvent('undoLastCommit', {package: 'github'});

This comment has been minimized.

@annthurium

annthurium Sep 10, 2018
Contributor

nit: for the counter names, we're using dash separation. Could you make this undo-last-commit instead?

@@ -17,6 +17,7 @@ import Remote from '../remote';
import RemoteSet from '../remote-set';
import Commit from '../commit';
import OperationStates from '../operation-states';
import {addEvent} from '../../reporter-proxy';

This comment has been minimized.

@annthurium

annthurium Sep 10, 2018
Contributor

one thing I worry about is, we currently have some metrics tracking in git-shell-out-strategy. Which is basically as close to the "back end" as you can go. We also have tracking on the UI layer, which is useful for seeing how many users are clicking specific buttons and whatnot. If we are adding instrumentation to the middle layers of the application, how do we ensure we're not double counting events? Would it make more sense to move this instrumentation down a layer to keep it all in one place?

This comment has been minimized.

@kuychaco

kuychaco Sep 10, 2018
Author Member

This is a good question. @annthurium and I discussed it in person and I'll capture the conclusions here for posterity's sake --

While it's a great rule of thumb to keep instrumentation at the boundary layers of our application (UI or lowest layer), ultimately we want to insert tracking where the information we want to track lives. In this case, we want commit metadata including whether or not the commit is a partial commit, and for this we need to count the number of unstaged files. We have logic for determining the number of unstaged files in the Repository layer, and not in the GitShellOutStrategy (we try to keep GSOS methods very basic, simply fetching data from Git and making it available to the Repository layer).

So in the end, we have a bit of duplication in that we are collecting commit events in the Repository and incrementing a counter whenever there is a commit in the GitShellOutStrategy. But, we're collecting additional metadata in the Repository layer, and we're avoiding adding complexity to our GitShellOutStrategy with duplicated logic just for the sake of gathering metrics.

This comment has been minimized.

@smashwilson

smashwilson Sep 11, 2018
Member

Okay, cool 馃槃 I was wondering about what we should do about this.

I'm 馃憤 on putting our addEvent() calls where we have the most information about the metric we're trying to collect, as opposed to doing contortions to bring the information to the same semantic layer of the codebase. We can always git grep addEvent to see what we're collecting and where as a sanity check, but I feel like it's be pretty messy to drill enough options around purely to be able to count the right things. 馃

This comment has been minimized.

@smashwilson

smashwilson Sep 11, 2018
Member

Another thing we could do to keep a handle on the usefulness and non-redundancy of the metrics we're collecting might be to assert about them in our "integration"-style tests. That'd help us see a view of activity we'd see from the specific, holistic user behaviors that we're asserting against there.

stagedFiles: [],
unstagedFiles: [],
mergeConflictFiles: [],
stagedFiles: {},

This comment has been minimized.

@annthurium

annthurium Sep 10, 2018
Contributor

just for my own education: why did you change this function to return objects instead of arrays? (I'm sure there's a perfectly good reason, I just don't know what it is 馃槂 )

This comment has been minimized.

@kuychaco

kuychaco Sep 10, 2018
Author Member

heh, yeah good question. it's not immediately clear.

This was to fix an oversight. I checked the format of the results returned from this function and objects are expected rather than arrays. Probably doesn't make a difference for the error case here since they're all empty objects anyways, but consistency is good

This comment has been minimized.

@smashwilson

smashwilson Sep 11, 2018
Member

... Good catch 馃槃

@annthurium
Copy link
Contributor

@annthurium annthurium commented Sep 10, 2018

a few more thoughts on the questions you raised in the body of the pull request:

  • I agree that it would be icky to pass extra meta data about expandedCommitMessageEditor to the repository just for metrics tracking. It looks like the only way to toggle the expanded commit editor is through that button, though? You could instrument the button to get a sense of how often users are toggling it on. If it's very occasional, we can safely assume this is not a popular feature.

  • I haven't come up with a great strategy for instrumenting context menu actions as of yet. That would be a good thing to add. At a glance, perhaps we could leverage the ContextMenuInterceptor component and just track everything we're intercepting.

assert.isFalse(args[1].partial);
});

it.only('reports if the commit was an amend', async function() {

This comment has been minimized.

@annthurium

annthurium Sep 10, 2018
Contributor

I was like "why did code coverage decrease so much" and then I was like "ohh." I do the same thing myself at least once a week.

@kuychaco
Copy link
Member Author

@kuychaco kuychaco commented Sep 10, 2018

Regarding gathering metrics on expanded commit editor usage, if we do choose to do so

It looks like the only way to toggle the expanded commit editor is through that button, though? You could instrument the button to get a sense of how often users are toggling it on.

You can also use a command, so the optimal location to instrument would be the CommitController#openCommitMessageEditor method, which both the button and command call through to

If it's very occasional, we can safely assume this is not a popular feature.

Yeah, that and/or there鈥檚 potential to improve discoverability :)

coAuthors: coAuthors.map(author => {
return {email: author.getEmail(), name: author.getFullName()};
}),
};

This comment has been minimized.

@kuychaco

kuychaco Sep 10, 2018
Author Member

@smashwilson can you double check to see if I retained the logic you originally intended with this change?

This comment has been minimized.

@smashwilson

smashwilson Sep 11, 2018
Member

Yup, it looks good to me 馃憤

Default options to {}; if options contains coAuthors, translate it to an Array of email/name pairs.

@kuychaco kuychaco changed the title Add metrics collection for committing partial changes and undoing the last commit Add metrics collection for committing partial changes, undoing the last commit, and opening an expanded commit msg editor Sep 10, 2018
Copy link
Member

@smashwilson smashwilson left a comment

@@ -17,6 +17,7 @@ import Remote from '../remote';
import RemoteSet from '../remote-set';
import Commit from '../commit';
import OperationStates from '../operation-states';
import {addEvent} from '../../reporter-proxy';

This comment has been minimized.

@smashwilson

smashwilson Sep 11, 2018
Member

Okay, cool 馃槃 I was wondering about what we should do about this.

I'm 馃憤 on putting our addEvent() calls where we have the most information about the metric we're trying to collect, as opposed to doing contortions to bring the information to the same semantic layer of the codebase. We can always git grep addEvent to see what we're collecting and where as a sanity check, but I feel like it's be pretty messy to drill enough options around purely to be able to count the right things. 馃

coAuthors: coAuthors.map(author => {
return {email: author.getEmail(), name: author.getFullName()};
}),
};

This comment has been minimized.

@smashwilson

smashwilson Sep 11, 2018
Member

Yup, it looks good to me 馃憤

Default options to {}; if options contains coAuthors, translate it to an Array of email/name pairs.

const unstagedCount = Object.keys({...unstagedFiles, ...mergeConflictFiles}).length;
addEvent('commit', {
package: 'github',
partial: unstagedCount > 0,

This comment has been minimized.

@smashwilson

smashwilson Sep 11, 2018
Member

For example, keyboard users might stage files by hitting enter a bunch of times for each line/hunk/file they want to stage... so for a given partial commit, there could be a ton of staging events.

We'd almost have to track a flag locally (somewhere in package state... ?) that recorded a bunch of aggregate information about a commit while it's being staged, then increment actual metrics when the commit is finalized here based on their final states.

For example, when a user clicks "Stage Selection" in the FilePatchItem, we know if the file still includes unstaged changes or not, so we can update a "commit includes partial file" flag to true or false; then, at commit time, we check the flag and send a partiallyStagedFile property on the final event.

I have no idea where those flags should live, though - we don't really have a model yet for "the commit under construction." Also the FilePatchItem is about to change drastically so... 馃檭

If all of these are heavily used, great. If not, we should consider simplifying so we don't have so many different ways of performing the same operation.

Yeah, that's a great point. I'm not sure how we'd do that, though 馃

stagedFiles: [],
unstagedFiles: [],
mergeConflictFiles: [],
stagedFiles: {},

This comment has been minimized.

@smashwilson

smashwilson Sep 11, 2018
Member

... Good catch 馃槃

@@ -17,6 +17,7 @@ import Remote from '../remote';
import RemoteSet from '../remote-set';
import Commit from '../commit';
import OperationStates from '../operation-states';
import {addEvent} from '../../reporter-proxy';

This comment has been minimized.

@smashwilson

smashwilson Sep 11, 2018
Member

Another thing we could do to keep a handle on the usefulness and non-redundancy of the metrics we're collecting might be to assert about them in our "integration"-style tests. That'd help us see a view of activity we'd see from the specific, holistic user behaviors that we're asserting against there.

sinon.stub(reporterProxy, 'addEvent');
// open expanded commit message editor
await wrapper.find('CommitView').prop('toggleExpandedCommitMessageEditor')('message in box');
assert.deepEqual(reporterProxy.addEvent.callCount, 1);

This comment has been minimized.

@smashwilson

smashwilson Sep 11, 2018
Member

Nitpick: these assertions can be assert.strictEqual() instead, or even assert.isTrue(reporterProxy.addEvent.called).

This comment has been minimized.

@kuychaco

kuychaco Sep 11, 2018
Author Member

oh haha yeah, copy/paste oversight. Thanks for pointing this out!

assert.deepEqual(reporterProxy.addEvent.callCount, 1);
let args = reporterProxy.addEvent.lastCall.args;
assert.deepEqual(args[0], 'open-commit-message-editor');
assert.deepEqual(args[1], {package: 'github'});

This comment has been minimized.

@smashwilson

smashwilson Sep 11, 2018
Member

There is also assert.isTrue(reporterProxy.addEvent.calledWith('open-commit-message-editor', {package: 'github'})) to do these all at once! Although as I've found out the hard way those don't behave properly with things like Sets.

This comment has been minimized.

@kuychaco

kuychaco Sep 11, 2018
Author Member

oooh I like!


assert.deepEqual(reporterProxy.addEvent.callCount, 0);
try {
await repo.undoLastCommit();

This comment has been minimized.

@smashwilson

smashwilson Sep 11, 2018
Member

If you want to be sure this is a failure, you can replace this try-catch with:

await assert.isRejected(repo.undoLastCommit());

This comment has been minimized.

@kuychaco

kuychaco Sep 11, 2018
Author Member

I do want to be sure! :)

Thanks for all the great suggestions for cleaning up test assertions @smashwilson. I like the improved cleanliness and readability

@kuychaco kuychaco merged commit 8d407dd into master Sep 11, 2018
6 checks passed
6 checks passed
ci/circleci: beta Your tests passed on CircleCI!
Details
ci/circleci: dev Your tests passed on CircleCI!
Details
ci/circleci: stable 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 (+0.08%) to 80.245%
Details
@kuychaco kuychaco deleted the ku-mo-metrics branch Sep 11, 2018
@kuychaco kuychaco moved this from In Progress 馃敡 to Merged 鈽戯笍 in Feature Sprint : 27 August - 14 September 2018 : v0.20.0 Sep 18, 2018
@annthurium annthurium mentioned this pull request Oct 20, 2018
7 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can鈥檛 perform that action at this time.