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

Fix the payload of core timing events #109

Merged
merged 1 commit into from Mar 22, 2019

Conversation

Projects
None yet
1 participant
@rafeca
Copy link
Contributor

commented Mar 21, 2019

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions
  • If you add/remove/modify any event please update the documentation

Description of the Change

This PR changes the format of the timing events to send an ec field instead of a category field, which is more aligned with the rest of events.

Alternate Designs

Leave it as it is 馃槃

Benefits

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

Possible Drawbacks

It's a change 馃槄

Applicable Issues

N/A

/cc @telliott27

Fix the payload of core timing events
They were sending a `category` field, I've changed it to send an `ec`
field which is more aligned with the rest of events

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

@@ -30,25 +30,21 @@ Timing events log the duration that a specific action took plus some metadata th

#### Window load time ([more info](https://atom.io/docs/api/v1.35.1/AtomEnvironment#instance-getWindowLoadTime))

* **eventType**: `core`
* **eventType**: `load`

This comment has been minimized.

Copy link
@rafeca

rafeca Mar 21, 2019

Author Contributor

I haven't changed the behaviour here (the event type was already load before ,as seen in this comment), it just wrongly documented (the redirection of using the sendTiming method with different order of arguments made it really confusing...

@rafeca rafeca self-assigned this Mar 21, 2019

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

I'm gonna land this PR since the sooner that we have the correct payload of the events the better... If the code review arises anything to fix from this PR I'll do it in a follow-up PR

@rafeca rafeca merged commit ffd7116 into master Mar 22, 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-of-timings branch Mar 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.