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

Fix field names used by the welcome package metric events #73

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 includes three changes:

  • A fix to send the events that were queued correctly (they were sent incorrectly in a nested array).
  • A fix to use the correct field names for all the events.
  • An added doc to specify the format of the events for this package.

The changes of the metric events field names have been done to match the names that atom/metric uses (source), so our analytics pipeline can process them correctly.

Ideally the name of the fields would be fixed by atom/metrics so other packages do not have to worry about using the correct fields, but this can be tackled in the future.

Test plan

  • Check that the expected payload is sent 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

N/A

Benefits

  • Easier processing in our backends (we can reuse the processing for custom events).

Possible Drawbacks

N/A

Applicable Issues

N/A

@rafeca rafeca requested review from jasonrudolph and annthurium Mar 21, 2019

@rafeca rafeca self-assigned this Mar 21, 2019

@rafeca rafeca force-pushed the fix-payload-events branch from 623c339 to fc3422b Mar 21, 2019

@rafeca rafeca force-pushed the fix-payload-events branch from fc3422b to fe7414c Mar 21, 2019

[{category: 'welcome-v1', action: 'foo', label: 'bar', value: 'baz'}],
[{category: 'welcome-v1', action: 'foo2', label: 'bar2', value: 'baz2'}]
{category: 'welcome-v1', action: 'foo', label: 'bar', value: 'baz'},
{category: 'welcome-v1', action: 'foo2', label: 'bar2', value: 'baz2'}

This comment has been minimized.

Copy link
@rafeca

rafeca Mar 21, 2019

Author Contributor

This is why unit tests are tricky 😅

@jasonrudolph
Copy link
Member

left a comment

👋 Thanks for digging into this, @rafeca. ⚡️ I noted a few minor suggestions in docs/events.md that I think will help things read a bit more naturally. I hope that helps.

I looked over the code, and I just have one question: Can you describe how you'll verify that these changes are having the desired effect? (Note: This reminds me that we should update the pull request templates to prompt for a verification plan. 😅)

Show resolved Hide resolved docs/events.md Outdated
Show resolved Hide resolved docs/events.md Outdated
Show resolved Hide resolved docs/events.md Outdated

jasonrudolph and others added some commits Mar 22, 2019

Typo in events doc
Co-Authored-By: rafeca <rafeca@gmail.com>
Typo in events doc
Co-Authored-By: rafeca <rafeca@gmail.com>
while ((customEvent = this.queue.shift())) {
this.reporter.addCustomEvent(customEvent.category, customEvent)
this.reporter.addCustomEvent(this.eventType, customEvent)

This comment has been minimized.

Copy link
@annthurium

annthurium Mar 22, 2019

Contributor

so the reason we were passing category as eventType was to be consistent with what we were passing to google analytics. Now that google analytics doesn't exist any more, consistency isn't important there. This may break some of our existing dashboards, if the event type differs from the category, so that's something to be aware of.

This comment has been minimized.

Copy link
@rafeca

rafeca Mar 25, 2019

Author Contributor

The interesting part is that the welcome package was not passing a category before this PR (well it was passing a category field, but this is not understood by our pipelines, which expects ec), so I don't expect things to get broken (or at least more broken than currently 😝).

Fix typo
Co-Authored-By: rafeca <rafeca@gmail.com>
@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

I looked over the code, and I just have one question: Can you describe how you'll verify that these changes are having the desired effect? (Note: This reminds me that we should update the pull request templates to prompt for a verification plan. 😅)

I've added a test plan on this PR. I haven't added many details about how to check that the events get propagated internally but feel free to ping me if you're interested in knowing how do I check it 😃.

I completely agree about adding verification steps to the PR templates!

@rafeca rafeca merged commit c6ff095 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 fix-payload-events branch Mar 25, 2019

@jasonrudolph

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

I've added a test plan on this PR.

Looks good! ⚡️🙇

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.