Skip to content
This repository has been archived by the owner on Aug 12, 2019. It is now read-only.

upgrade to use new method available on metrics-reporter service #67

Merged
merged 2 commits into from
Aug 20, 2018

Conversation

annthurium
Copy link
Contributor

@annthurium annthurium commented Aug 20, 2018

Description of the Change

As part of the telemetry project, I added some new interfaces to the metrics service. For the sake of consistency and not having several apis that serve the same purpose, I'm porting callers of the sendEvent service to use the new addCustomEvent functionality instead.

Alternate Designs

I could have left the old interface sendEvent interface in place. However, this package looks to be the only caller of sendEvent. It feels lazy not to clean that up and completely deprecated the old interface.

Why Should This Be In Core?

The new user experience is already in the core of Atom. This pull request adds no new user-visible functionality to Atom. It's pure refactoring / cleanup.

Benefits

After this change is merged, there are no callers of metricsProvider.sendEvent which means that api can safely be removed in future versions of Atom. Having a smaller api surface area for Atom to support increases maintainability of Atom in the future.

Possible Drawbacks

Any change introduces the possibility of regressions or uncaught bugs. Other than that, I couldn't think of any drawbacks. If you think of any, I am super open to hearing them.

Test Plan

  • Update unit tests of welcome package to ensure that:
    • when reporterProxy.setReporter is called, events are sent to the real reporter and queued-up events are flushed.
  • Manual testing: in a dev mode window, configure atom to show welcome on startup. Add console logging to ensure that welcome events are sent to the reporter, and that these events have the shape that we expect.

screen shot 2018-08-20 at 2 14 22 pm

Note: `label` and `value` were undefined before, so these values are consistent with the shape of the event we were previously passing.

Copy link

@daviwil daviwil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@annthurium annthurium merged commit 55ecc3b into master Aug 20, 2018
@annthurium annthurium deleted the tt-18-aug-cleanup-reporter branch August 20, 2018 21:26
@annthurium
Copy link
Contributor Author

I think this is good to go, so I'm gonna go ahead and merge. If you have questions or concerns about this PR, please do leave a comment and I will follow up with you.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants