Permalink
Browse files

Flatten experiment variant to int for ga.js.

If experiment variant is not an int, inject JS comment with a warning.
  • Loading branch information...
hillsp authored and crowell committed Apr 18, 2016
1 parent 6506752 commit a2b10e49318ea9d8e2ecf26e2c562bc427a06e99
@@ -141,7 +141,15 @@ extern const char kContentExperimentsJsClientUrl[] =
// before ga.js loads you need to call this. The first argument is the
// variant id, the second is the experiment id.
extern const char kContentExperimentsSetChosenVariationSnippet[] =
"cxApi.setChosenVariation('%s', '%s');";
"cxApi.setChosenVariation(%d, '%s');";

// When using content experiments with ga.js, the variant ID must be numeric.
// If the user requests a non-numeric variant with ga.js, we inject this
// comment. The string is bracketed with newlines because otherwise it's
// invisible in a wall of JavaScript.
extern const char kContentExperimentsNonNumericVariantComment[] =
"\n/* mod_pagespeed cannot inject experiment variant '%s' "
"because it's not a number */\n";

// When using content experiments with analytics.js, after ga('create', ..._)
// and before ga('[...].send', 'pageview'), we need to insert:
@@ -246,13 +254,30 @@ InsertGAFilter::AnalyticsStatus InsertGAFilter::FindSnippetInScript(
return kUnusableSnippetFound;
}

GoogleString InsertGAFilter::AnalyticsJsExperimentSnippet() {
GoogleString InsertGAFilter::AnalyticsJsExperimentSnippet() const {
return StringPrintf(
kContentExperimentsSetExpAndVariantSnippet,
driver()->options()->content_experiment_id().c_str(),
driver()->options()->content_experiment_variant_id().c_str());
}

GoogleString InsertGAFilter::GaJsExperimentSnippet() const {
// ga.js requires a numeric variant id. Attempt to convert the string
// variant ID to int and use that.
const char* variant_id =
driver()->options()->content_experiment_variant_id().c_str();
int numeric_variant_id;
if (StringToInt(variant_id, &numeric_variant_id)) {
return StringPrintf(
kContentExperimentsSetChosenVariationSnippet, numeric_variant_id,
driver()->options()->content_experiment_id().c_str());
} else {
// Variant ID was non-numeric, so inject a warning.
return StringPrintf(kContentExperimentsNonNumericVariantComment,
variant_id);
}
}

// * If we've already inserted any GA snippet or if we found a GA snippet in the
// original page, don't do anything.
// * If we haven't found anything, and haven't inserted anything yet, insert the
@@ -294,11 +319,7 @@ void InsertGAFilter::EndDocument() {
driver()->AddAttribute(
cxapi, HtmlName::kSrc, kContentExperimentsJsClientUrl);
InsertNodeAtBodyEnd(cxapi);

experiment_snippet = StringPrintf(
kContentExperimentsSetChosenVariationSnippet,
driver()->options()->content_experiment_variant_id().c_str(),
driver()->options()->content_experiment_id().c_str());
experiment_snippet = GaJsExperimentSnippet();
} else {
experiment_snippet = StringPrintf(
kGAExperimentSnippet,
@@ -515,15 +536,11 @@ void InsertGAFilter::RewriteInlineScript(HtmlCharactersNode* characters) {
void InsertGAFilter::HandleEndScript(HtmlElement* script) {
if (!postponed_script_body_.empty()) {
DCHECK(script == script_element_);
GoogleString snippet_text = StringPrintf(
kContentExperimentsSetChosenVariationSnippet,
driver()->options()->content_experiment_variant_id().c_str(),
driver()->options()->content_experiment_id().c_str());

driver()->InsertScriptAfterCurrent(
kContentExperimentsJsClientUrl, true /* external */);
driver()->InsertScriptAfterCurrent(
StrCat(snippet_text, postponed_script_body_), false /* inline */);
StrCat(GaJsExperimentSnippet(), postponed_script_body_),
false /* inline */);
added_experiment_snippet_ = true;
postponed_script_body_.clear();
}
@@ -252,19 +252,24 @@ class InsertGAFilterTest : public RewriteTestBase {
}

void SetUpContentExperiment(bool use_analytics_js) {
SetUpContentExperiment(use_analytics_js, "456");
}

void SetUpContentExperiment(bool use_analytics_js,
const GoogleString& variant_id) {
NullMessageHandler handler;
RewriteOptions* options = rewrite_driver()->options()->Clone();
options->set_use_analytics_js(use_analytics_js);
options->set_running_experiment(true);
ASSERT_TRUE(options->AddExperimentSpec(
ASSERT_TRUE(options->AddExperimentSpec(StringPrintf(
"id=2;percent=10;slot=4;options="
"ContentExperimentID=123,"
"ContentExperimentVariantID=456", &handler));
"ContentExperimentVariantID=%s", variant_id.c_str()), &handler));
ASSERT_TRUE(options->AddExperimentSpec(
"id=7;percent=10;level=CoreFilters;slot=4;options="
"ContentExperimentID=123,"
"ContentExperimentVariantID=789", &handler));
options->SetExperimentState(2); // Expecting cxid=123, cxvid=456.
options->SetExperimentState(2); // Expecting cxid=123, cxvid=variant_id.

// Setting up experiments automatically enables AddInstrumentation.
// Turn it off so our output is easier to understand.
@@ -376,7 +381,25 @@ TEST_F(InsertGAFilterTest, ExperimentGaJsCx) {
"\"></script>").c_str(),
"",
StrCat(StringPrintf(kContentExperimentsSetChosenVariationSnippet,
"456", "123"),
456, "123"),
StringPrintf(kGAJsSnippet, kGaId, "test.com",
kGASpeedTracking)).c_str());
ValidateExpected("ga.js cx experiment", kHtmlInput, output);
}

TEST_F(InsertGAFilterTest, ExperimentGaJsCxString) {
// Show that an attempt to insert a ga.js snippet with a string variant ID
// results in a warning message.
const GoogleString& kVariantText("StringVariant");
SetUpContentExperiment(false, kVariantText);
GoogleString output = StringPrintf(
kHtmlOutputFormat,
StrCat("<script src=\"",
kContentExperimentsJsClientUrl,
"\"></script>").c_str(),
"",
StrCat(StringPrintf(kContentExperimentsNonNumericVariantComment,
kVariantText.c_str()),
StringPrintf(kGAJsSnippet, kGaId, "test.com",
kGASpeedTracking)).c_str());
ValidateExpected("ga.js cx experiment", kHtmlInput, output);
@@ -411,6 +434,21 @@ TEST_F(InsertGAFilterTest, ExperimentAnalyticsJsCx) {
ValidateExpected("analytics.js cx experiment", kHtmlInput, output);
}

TEST_F(InsertGAFilterTest, ExperimentAnalyticsJsCxString) {
// Show that we can insert an anlytics.js snippet that includes content
// experiment tracking where the variant is a string.
const GoogleString& kVariantText("StringVariant");
SetUpContentExperiment(true, kVariantText);
GoogleString output = StringPrintf(
kHtmlOutputFormat, "", "", StringPrintf(
kAnalyticsJsSnippet,
kGaId,
kAnalyticsJsIncreaseSiteSpeedTracking,
StringPrintf(kContentExperimentsSetExpAndVariantSnippet,
"123", kVariantText.c_str()).c_str()).c_str());
ValidateExpected("analytics.js cx experiment", kHtmlInput, output);
}

const char kHtmlInputWithGASnippetFormat[] =
"<head>\n<title>Something</title>\n"
"</head><body> Hello World!"
@@ -529,7 +567,7 @@ TEST_F(InsertGAFilterTest, SynchronousGAContentExperiment) {
"\"></script><script>",
StringPrintf(
kContentExperimentsSetChosenVariationSnippet,
"456", "123")).c_str(),
456, "123")).c_str(),
kGaId);
ValidateExpected("extend sync ga.js for content experiment", input, output);
}
@@ -568,7 +606,7 @@ TEST_F(InsertGAFilterTest, AsynchronousGAContentExperiment) {
"\"></script><script>",
StringPrintf(
kContentExperimentsSetChosenVariationSnippet,
"456", "123")).c_str(),
456, "123")).c_str(),
kGaId);
ValidateExpected("extend async ga.js for content experiment", input, output);
}
@@ -764,7 +802,7 @@ TEST_F(InsertGAFilterTest, ExistingGaJsContentExperimentNoCloseAnything) {
"\"></script>"
"<script>",
StringPrintf(kContentExperimentsSetChosenVariationSnippet,
"456", "123"),
456, "123"),
StringPrintf(kGAJsSnippet, kGaId, "test.com",
kGASpeedTracking)).c_str());

@@ -786,7 +824,7 @@ TEST_F(InsertGAFilterTest, AsynchronousGAContentExperimentFlush) {
"\"></script><script>",
StringPrintf(
kContentExperimentsSetChosenVariationSnippet,
"456", "123")).c_str(),
456, "123")).c_str(),
kGaId);

SetupWriter();
@@ -42,6 +42,7 @@ extern const char kAnalyticsJsSnippet[];
extern const char kAnalyticsJsIncreaseSiteSpeedTracking[];
extern const char kAnalyticsJsIncreaseSiteSpeedTrackingMinimal[];
extern const char kContentExperimentsJsClientUrl[];
extern const char kContentExperimentsNonNumericVariantComment[];
extern const char kContentExperimentsSetChosenVariationSnippet[];
extern const char kContentExperimentsSetExpAndVariantSnippet[];
extern const char kGASpeedTracking[];
@@ -112,9 +113,9 @@ class InsertGAFilter : public CommonFilter {
// snippet depends in part on whether we've already seen a ga.js library load.
AnalyticsStatus FindSnippetInScript(const GoogleString& s);

// Determine the snippet of JS we need to log a content experiment to
// analytics.js.
GoogleString AnalyticsJsExperimentSnippet();
// Determine the snippet of JS we need to log a content experiment.
GoogleString AnalyticsJsExperimentSnippet() const;
GoogleString GaJsExperimentSnippet() const;

// Note: logs a warning if we're running with analytics.js and have asked it
// to log to a custom variable (which isn't possible).

0 comments on commit a2b10e4

Please sign in to comment.