Start logging the time to index a project

rafeca commented 5 years ago

Description of the Change

This PR makes use of the atom/metrics package to be able to start logging the time it takes to index a project by the fuzzy finder.

The format of the data that's being logged is the following:

eventType: fuzzy-finder-v1
metadata

field value

ec time-to-crawl

el Crawler type (ripgrep or fs)

ev Number of crawled files

field	value
`ec`	`time-to-crawl`
`el`	Crawler type (`ripgrep` or `fs`)
`ev`	Number of crawled files

Sample payload

{
  "eventType": "fuzzy-finder-v1",
  "durationInMilliseconds": 539,
  "metadata": {
    "ec": "time-to-crawl",
    "el": "ripgrep",
    "ev": 49,
    "cd2": "x64",
    "cd3": "x64",
    "cm1": 180,
    "cm2": 83,
    "sr": "1680x1050",
    "vp": "1680x591",
    "aiid": "dev"
  },
  "date": "2019-03-21T17:05:32.706Z"
}

Test plan

[x] Check that atom/telemetry sends the correct payload to GitHub's backend. To do so, I've had to edit the code of the atom/telemetry package (more specifically, I've manually modified node_modules/telemetry-github/out/index.js to set the variable this.isDevMode to false (code). I've also had to increase how often Atom sends metrics, since by default it only sends them every 24 hours. To do so, I've set the variables DailyStatsReportIntervalInMs and ReportingLoopIntervalInMs located in the previous file to something like 15000 (code).
[x] Check that the data gets propagated correctly to our analytics pipeline.

Alternate Designs

Do not log this?

Benefits

Knowing the time it takes to crawl the project by the fuzzy finder on real users will help us decide how much effort should be put on improving its performance.

Possible Drawbacks

We're logging a little bit more data from users.

Applicable Issues

https://github.com/atom/fuzzy-finder/issues/371

/cc @telliott27

rafeca commented 5 years ago

One limitation of this implementation is that if the user opts out from metrics, the metrics reporter service won't initialize and the queue that keeps the events that haven't been sent will grow indefinitely...

This issue is already happening in the welcome package (code), while the github package has a fix for it by adding quite a bit of logic.

IMHO this should not be the responsibility of every package that wants to consume the metrics service: packages shouldn't care about whether users have or not opted in into metrics and ideally the metrics service should always return a reporter which will log or not depending on the user preferences.

I can implement this solution as part of atom/metrics if people think that this is the right approach 😃

jasonrudolph commented 5 years ago

Sample payload

Thanks for including the sample payload. Seeing this sample made it much easier for me to get a quick feel for these changes. :zap:

IMHO this should not be the responsibility of every package that wants to consume the metrics service: packages shouldn't care about whether users have or not opted in into metrics and ideally the metrics service should always return a reporter which will log or not depending on the user preferences.

I agree. :+1:

rafeca commented 5 years ago

Can you describe how you'll verify that these changes are having the desired effect?

I've added some extra information in the PR summary 😄

Would it make sense to add a test or two for this new functionality?

Yes! I just added a couple of tests (and they actually made me find an issue when queueing events 😅).

atom / fuzzy-finder