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

Start logging the time to index a project #372

Merged
merged 5 commits into from Mar 25, 2019

Conversation

Projects
None yet
3 participants
@rafeca
Copy link
Contributor

commented Mar 21, 2019

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

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

  • 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).
  • 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

#371

/cc @telliott27

@rafeca rafeca requested a review from jasonrudolph Mar 21, 2019

@rafeca rafeca force-pushed the log-time-to-index branch from a010802 to 8a64a2b Mar 21, 2019

@rafeca rafeca force-pushed the log-time-to-index branch from 8a64a2b to de6c9db Mar 21, 2019

@rafeca rafeca force-pushed the log-time-to-index branch from de6c9db to 720a8f4 Mar 21, 2019

@rafeca rafeca requested a review from annthurium Mar 21, 2019

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

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. ⚡️

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. 👍

@jasonrudolph
Copy link
Member

left a comment

I noted a few minor suggestions in docs/events.md that I think will make things read a bit more naturally. I hope that helps.

I looked over the code, and I have just a couple questions:

  • Can you describe how you'll verify that these changes are having the desired effect?
  • Would it make sense to add a test or two for this new functionality?
Show resolved Hide resolved docs/events.md Outdated
Show resolved Hide resolved docs/events.md Outdated
Show resolved Hide resolved docs/events.md Outdated
@@ -0,0 +1,35 @@

This comment has been minimized.

Copy link
@jasonrudolph

jasonrudolph Mar 22, 2019

Member

☝️ Can we remove this extra line?

Fix typos in events spec
Co-Authored-By: rafeca <rafeca@gmail.com>
@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

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 😅).

@rafeca rafeca merged commit fcb39fd into master Mar 25, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rafeca rafeca deleted the log-time-to-index branch Mar 25, 2019

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.