rawls238 / react-experiments

React components for implementing UI experiments
319 stars 20 forks source link

Fix exposure logging + cleanup #4

Closed eytan closed 9 years ago

eytan commented 9 years ago

Logging conditional on whether there is a matching variable can cause downstream (errors in randomization)[https://github.com/HubSpot/react-experiments/issues/1]. This diff fixes that by not making logging dependent on having a matching variant. This diff also includes a number of changes that increase specificity, including:

Todo:

This reverts commit c85a224531d4e21d0220b75b8dc0b9934071c6f1.

eytan commented 9 years ago

@rawls238, does the list of TODOs seem reasonable to you? I was expecting that this diff might be a little contentious... After more further consideration, my plan would be to get rid of the experimentEnrollment object, as well as the isEnrolled property.

rawls238 commented 9 years ago

I think that the experimentEnrollment class is still needed as long as the Namespace class is still in use. The process of writing up a few changes to the overall interface in #1 may make it necessary to repurpose this class for different uses.

I think that the changes to logging make sense. We originally added the conditional logging because we had a few experiments where each variation was in a separate component (though, if everything was working correctly in the code, an exposure event would log for each variation since they were all in the same area on the screen on the same page). The idea was that having exposure events only log when the variation was actually rendered would theoretically allow us to be able to detect when something is wrong with one of the variations, since we would not see the right # of exposure logs for that variant.

This method opens up a huge can of worms (for instance, the problems that you mentioned) that I'm not sure how to prevent so just doing automatic exposure logging makes sense as one piece of the solution. I think a decent compromise may be to log an exposure event as done in this PR, but also log an experiment event (using logEvent) when the variation component is actually rendered to get the best of both worlds. I think it's important for this library to work when experiment variations may need to be defined across different components so we should aim to ensure that we can allow consumers of the library to do so regardless of how their existing code is structured and without making it easy to shoot yourself in the foot. (Let me know if you disagree with anything here / find any flaws in it.)

On that note, we should nuke utils.js. Also agree with renaming the experimentClass, but not sure we should add propTypes for the experiment prop since I still want to allow consumers of the library to be able to use their own experiment Classes.

eytan commented 9 years ago

OK @rawls238, here are my changes. Probably won't have much time this week to work on it much more, so you could either merge and fix up later, or take whatever parts you'd like and I can abandon the PR.

rawls238 commented 9 years ago

thanks for doing this @eytan, I've added some of these suggestions to https://github.com/HubSpot/react-experiments/pull/6 but for the most part since the library has effectively been rewritten most of these changes are now irrelevant so closing this