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(track): Batch decision payloads into the same array in the snapshot. #155

Merged
merged 1 commit into from Aug 21, 2018

Conversation

mikeproeng37
Copy link
Contributor

@mikeproeng37 mikeproeng37 commented Aug 21, 2018

Before it was many snapshots, each with exactly one decision and one conversion event. This is not accurate because decisions for the experiments attached to the same event should be in the same snapshot.

Only conversion events need to have this special logic, because impression events only come from activate and isFeatureEnabled API (and transitively, getEnabledFeatures), which all submit a single impression event for just one experiment.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 97.114% when pulling 2e8a62c on mng/fix-batch-event-payload into 7ab1195 on master.

@mikeproeng37
Copy link
Contributor Author

build

@mikeproeng37
Copy link
Contributor Author

build-e2e

Copy link
Contributor

@spencerwilson-optimizely spencerwilson-optimizely left a comment

Choose a reason for hiding this comment

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

Does getImpressionEventParams need this change too?

@@ -1272,16 +1272,7 @@ describe('lib/optimizely', function() {
'campaign_id': '4',
'experiment_id': '111127',
'variation_id': '111129'
}],
'events': [{
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this no longer in the snapshot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, it gets added on line 154. It's just that instead of having the event duplicated for every decision, we just send one event per snapshot.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh okay cool

@mikeproeng37
Copy link
Contributor Author

So impression does not need to change because at the moment we only ever activate one experiment at a time instead of batching multiple impressions together. track is special because it loops through each experiment the event is attached to and captures a decision for each.

@spencerwilson-optimizely
Copy link
Contributor

spencerwilson-optimizely commented Aug 21, 2018

Got it, thanks!

I found the PR name seems kinda misleading, is this name/description accurate?

fix(track): Send all conversion events in one snapshot

Before it was many snapshots, each with exactly one decision and one conversion event. This is wrong because TODO: REASON.

Only conversion events need to have this special logic, because impression events only come from the `activate` and `isFeatureEnabled` APIs (and transitively, `getEnabledFeatures`), which all submit a single impression event for just one experiment. 

edit: I got this wrong, it should be something like 'Send all decisions in the same snapshot as the conversion event' or something.

@@ -1272,16 +1272,7 @@ describe('lib/optimizely', function() {
'campaign_id': '4',
'experiment_id': '111127',
'variation_id': '111129'
}],
'events': [{
Copy link
Contributor

Choose a reason for hiding this comment

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

oh okay cool

@mikeproeng37 mikeproeng37 merged commit 5b379df into master Aug 21, 2018
@aliabbasrizvi aliabbasrizvi deleted the mng/fix-batch-event-payload branch August 30, 2018 19:07
@loganlinn loganlinn restored the mng/fix-batch-event-payload branch December 15, 2018 00:01
@mikeproeng37 mikeproeng37 deleted the mng/fix-batch-event-payload branch February 21, 2019 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants