Add metrics #349
Add metrics #349
Conversation
Some of it commented out using //// for things that aren't supported in GHfVS - needs a tidy up when we decide what we're going to track. Also not used anywhere yet!
As it may not exist.
For some reason the UsageTracker isn't being found by MEF though.
Also only include metrics service if build is internal.
And increment launch count when package initialized.
Also add supporting for generating complex properties like UIState
…ty-events' into feature/metrics
…ty-events' into feature/metrics
| using ReactiveUI; | ||
| using Rothko; | ||
|
|
||
| #pragma warning disable CS0649 |
There was a problem hiding this comment.
Nope! Well spotted 👓! Will remove.
| this.cache = cache; | ||
| this.repositoryHosts = repositoryHosts; | ||
| this.userSettings = userSettings; | ||
| this.client = (IMetricsService)serviceProvider.GetService(typeof(IMetricsService)); |
There was a problem hiding this comment.
This is going to blow up miserably if the service is not found. You need to use TryGetService<IMetricsService> here.
Use a dictionary of metrics keys with setter methods as values, so that BuildUsageModel and ClearCounters use the same data and never go out of sync.
| public class UsageTracker : IUsageTracker | ||
| { | ||
| // Whenever you add a counter make sure it gets added to _both_ | ||
| // BuildUsageModel and ClearCounters |
There was a problem hiding this comment.
This is a sign that we need to refactor the code so that we don't accidentally forget things. I opened #359 to fix this, comments welcome!
There was a problem hiding this comment.
Yeah, this came from the GHfW codebase - the changes in your PR look good to me.
There was a problem hiding this comment.
Ah, yes, I forgot this is mostly GHfW code. Cool, feel free to merge it in then!
So use TryGetService. Also removed unnecessary braces.
| else | ||
| { | ||
| return Observable.Return(Unit.Default); | ||
| } |
There was a problem hiding this comment.
There's those pesky braces again...
There was a problem hiding this comment.
Again, these come from GHfW - do you want me to change them or should we be avoiding unnecessary differences with GHfW when the other code is identical?
There was a problem hiding this comment.
GHfW is not going to have any changes to this code, so it doesn't matter if it's identical or not (and it's not anymore anyway with the other PR coming in). I'd rather have consistency in the code base and make this ours.
To be very honest, I'd rather have
return client != null
? Observable.Defer(() =>
{
log.Info("User has enabled sending anonymized usage statistics to GitHub");
return client.SendOptIn();
})
: Observable.Return(Unit.Default);
because that's the form that we use the most in logic like this around the codebase, but that doesn't get on my nerves as much as those floating braces 😉
|
Make sure to test this without the script submodule to make sure we're not accidentally relying on internal things and breaking external contributors. |
List of metrics keys with setter methods
|
Also, if there's no |
|
/me kicks the ⚽ over |
|
Just checked this with |
|
This is looking fine and I'm 👍 for merging, but we can get a couple of tests for the clear counters and usage model building there, to make sure we're clearing everything that matters and that the built model has all the relevant keys? There might be tests already in the Desktop repo that we can reuse. |
Metrics may need to be sent before our plugin is fully loaded, so we can't use Rx.
…trics Conflicts: src/GitHub.App/Services/UsageTracker.cs
And remove accidentally committed file in there.
| { | ||
| var result = File.Exists(storePath) ? | ||
| SimpleJson.DeserializeObject<UsageStore>(File.ReadAllText(storePath)) : | ||
| new UsageStore { Model = new UsageModel() }; |
There was a problem hiding this comment.
This whole thing is going to need exception handling, there's multiple things that can go wrong here. I'd like to follow the same pattern that https://github.com/github/VisualStudio/blob/master/src/GitHub.VisualStudio/Services/ConnectionManager.cs has - delegates for IO methods that can be mocked later on by our unit tests, a separate method that ensures the cache file exists, and splitting up reading/writing from serializing/deserializing, since one can fail because of the OS but the latter can fail because we break the format (and we can catch that in unit tests)
Note: ConnectionManager is not checking for IO exceptions at the moment. It's never failed because that would require two VS instances running at the same time and accessing ConnectionManager at exactly the same time, which is unlikely to happen (most cache accesses are user-initiated when logging in, so a user would have to login at the same time in two VS instances, unlikely). However, metrics are timer based which means the likelihood of two VS instances racing for the same file access could potentially be higher, so we need to watch out for this. Any crashes here will crash VS.
As there can be >1 instance of VS running. There is still the possibility of race conditions here but as they would be unlikely and the impact would be minimal, I'm tempted to just accept the possibility rather than make the code more complex.
This is the same pattern used by `ConnectionManager` which allows us to pass in mocked rothko interfaces for testing, but ensure that the rothko dependency isn't loaded at runtime. However in GitHub.Exports we don't have and (possibly) don't want a dependency on rothko so we need to work out what to do there.
|
Merging this in. We need to do a build with this, deploy the central metrics branch and test a few metrics, so we can have some data to graph. |
Adds metrics to the extension.