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

empty all queues in `reporterProxy` once data has been sent. #1659

Merged
merged 1 commit into from Aug 22, 2018

Conversation

Projects
None yet
3 participants
@annthurium
Contributor

annthurium commented Aug 22, 2018

Description of the Change

Recently, I was working on some improvements to the welcome package in atom/welcome#67. I noticed that the welcome package, which also consumes the reporter service, empties all queues after the reporter has been set. I believe the queues are emptied to guard against the possibility of the queued data being sent again if the reporter service is re-set.

This change empties the queues in the github package's ReporterProxy after data has been sent. This change also adds unit tests to verify this new functionality works as expected.

Alternate Designs

Is re-sending the same data actually something we need to worry about?

I tried to reproduce a situation where data might be re-sent to see if this is really a problem. I put a setTimeout in the metrics package so that it loads in a deferred fashion, and performed some actions that are instrumented (staging and unstaging changes). This adds data to the queues before the reporter has been set. It appears that a new ReporterProxy class is created in memory each time Atom reloads, so old events sticking around isn't a problem. However, I'm not super confident that I'm testing this in the same conditions that would occur in the wild. Clearing the queues is a simple enough change that I'd just rather do this and put any lingering worries to rest about the accuracy of our data.

Benefits

Guard against the possibility that we are re-sending data that has already been sent, which keeps our metrics accurate.

Possible Drawbacks

I can't think of any but am open to feedback.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 22, 2018

Coverage Status

Coverage increased (+0.08%) to 80.204% when pulling 55831d8 on tt-18-aug-queues into 784c17b on master.

coveralls commented Aug 22, 2018

Coverage Status

Coverage increased (+0.08%) to 80.204% when pulling 55831d8 on tt-18-aug-queues into 784c17b on master.

@annthurium annthurium merged commit dfcfc64 into master Aug 22, 2018

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.08%) to 80.204%
Details

@annthurium annthurium deleted the tt-18-aug-queues branch Aug 22, 2018

@@ -109,6 +109,11 @@ describe('reporterProxy', function() {
assert.deepEqual(addTimingArgs[2], {gitHubPackageVersion: version});
assert.deepEqual(incrementCounterStub.lastCall.args, [counterName]);
assert.deepEqual(reporterProxy.events.length, 0);

This comment has been minimized.

@smashwilson

smashwilson Aug 23, 2018

Member

For future reference: you can do assert.lengthOf(thing, 0) to get a slightly more informative error message when it fails. 🌠

@smashwilson

smashwilson Aug 23, 2018

Member

For future reference: you can do assert.lengthOf(thing, 0) to get a slightly more informative error message when it fails. 🌠

This comment has been minimized.

@annthurium

annthurium Aug 23, 2018

Contributor

oh, thanks! I'll clean this up.

@annthurium

annthurium Aug 23, 2018

Contributor

oh, thanks! I'll clean this up.

This comment has been minimized.

@smashwilson

smashwilson Aug 23, 2018

Member

Haha that wasn't meant as "you have to fix this"! It was just a random note because I'm tired 👀

@smashwilson

smashwilson Aug 23, 2018

Member

Haha that wasn't meant as "you have to fix this"! It was just a random note because I'm tired 👀

This comment has been minimized.

@annthurium

annthurium Aug 23, 2018

Contributor

totally, didn't think you meant it that way, just want to get in the habit of using better assertions <3

@annthurium

annthurium Aug 23, 2018

Contributor

totally, didn't think you meant it that way, just want to get in the habit of using better assertions <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment