Skip to content

fix(api): Only track attributes with valid attribute types. #103

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

Merged
merged 5 commits into from
Sep 20, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 109 additions & 0 deletions OptimizelySDK.Tests/EventTests/EventBuilderTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,115 @@ public void TestCreateImpressionEventWithTypedAttributes()
Assert.IsTrue(TestData.CompareObjects(expectedLogEvent, logEvent));
}

[Test]
public void TestCreateImpressionEventRemovesInvalidAttributesFromPayload()
{
var guid = Guid.NewGuid();
var timeStamp = TestData.SecondsSince1970();

var payloadParams = new Dictionary<string, object>
{
{ "visitors", new object[]
{
new Dictionary<string, object>()
{
{ "snapshots", new object[]
{
new Dictionary<string, object>
{
{ "decisions", new object[]
{
new Dictionary<string, object>
{
{"campaign_id", "7719770039" },
{"experiment_id", "7716830082" },
{"variation_id", "7722370027" }
}
}
},
{ "events", new object[]
{
new Dictionary<string, object>
{
{"entity_id", "7719770039" },
{"timestamp", timeStamp },
{"uuid", guid },
{"key", "campaign_activated" }
}
}
}
}
}
},
{"attributes", new object[]
{
new Dictionary<string, object>
{
{"entity_id", "7723280020" },
{"key", "device_type" },
{"type", "custom" },
{"value", "iPhone"}
},
new Dictionary<string, object>
{
{"entity_id", "323434545" },
{"key", "boolean_key" },
{"type", "custom" },
{"value", true}
},
new Dictionary<string, object>
{
{"entity_id", "808797686" },
{"key", "double_key" },
{"type", "custom" },
{"value", 3.14}
},
new Dictionary<string, object>
{
{"entity_id", ControlAttributes.BOT_FILTERING_ATTRIBUTE},
{"key", ControlAttributes.BOT_FILTERING_ATTRIBUTE},
{"type", "custom" },
{"value", true }
}
}
},
{ "visitor_id", TestUserId }
}
}
},
{"project_id", "7720880029" },
{"account_id", "1592310167" },
{"client_name", "csharp-sdk" },
{"client_version", Optimizely.SDK_VERSION },
{"revision", "15" },
{"anonymize_ip", false}
};

var expectedLogEvent = new LogEvent("https://logx.optimizely.com/v1/events",
payloadParams,
"POST",
new Dictionary<string, string>
{
{ "Content-Type", "application/json" }
});

var userAttributes = new UserAttributes
{
{"device_type", "iPhone" },
{"boolean_key", true },
{"double_key", 3.14 },
{ "", "Android" },
{ "null", null },
{ "objects", new object() },
{ "arrays", new string[] { "a", "b", "c" } },
};

var logEvent = EventBuilder.CreateImpressionEvent(Config, Config.GetExperimentFromKey("test_experiment"), "7722370027", TestUserId, userAttributes);
TestData.ChangeGUIDAndTimeStamp(logEvent.Params, timeStamp, guid);

Assert.IsTrue(TestData.CompareObjects(expectedLogEvent, logEvent));
}

[Test]
public void TestCreateConversionEventNoAttributesNoValue()
{
Expand Down
15 changes: 6 additions & 9 deletions OptimizelySDK.Tests/OptimizelyTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -401,9 +401,12 @@ public void TestActivateWithAttributes()
EventBuilderMock.Verify(b => b.CreateImpressionEvent(It.IsAny<ProjectConfig>(), Config.GetExperimentFromKey("test_experiment"),
"7722370027", "test_user", OptimizelyHelper.UserAttributes), Times.Once);


LoggerMock.Verify(l => l.Log(It.IsAny<LogLevel>(), It.IsAny<string>()), Times.Exactly(6));
LoggerMock.Verify(l => l.Log(LogLevel.DEBUG, "User \"test_user\" is not in the forced variation map."), Times.Once);
LoggerMock.Verify(l => l.Log(LogLevel.DEBUG, "Assigned bucket [3037] to user [test_user] with bucketing ID [test_user]."), Times.Once);
LoggerMock.Verify(l => l.Log(LogLevel.INFO, "User [test_user] is in variation [control] of experiment [test_experiment]."), Times.Once);
LoggerMock.Verify(l => l.Log(LogLevel.INFO, "This decision will not be saved since the UserProfileService is null."));
LoggerMock.Verify(l => l.Log(LogLevel.INFO, "Activating user test_user in experiment test_experiment."), Times.Once);
LoggerMock.Verify(l => l.Log(LogLevel.DEBUG, @"Dispatching impression event to URL logx.optimizely.com/decision with params {""param1"":""val1""}."), Times.Once);

Expand All @@ -423,17 +426,14 @@ public void TestActivateWithNullAttributes()
var variation = (Variation)optly.Invoke("Activate", "test_experiment", "test_user", OptimizelyHelper.NullUserAttributes);

EventBuilderMock.Verify(b => b.CreateImpressionEvent(It.IsAny<ProjectConfig>(), Config.GetExperimentFromKey("test_experiment"),
"7722370027", "test_user", OptimizelyHelper.UserAttributes), Times.Once);
"7722370027", "test_user", OptimizelyHelper.NullUserAttributes), Times.Once);

LoggerMock.Verify(l => l.Log(It.IsAny<LogLevel>(), It.IsAny<string>()), Times.Exactly(9));
LoggerMock.Verify(l => l.Log(It.IsAny<LogLevel>(), It.IsAny<string>()), Times.Exactly(6));

//"User "test_user" is not in the forced variation map."
LoggerMock.Verify(l => l.Log(LogLevel.DEBUG, "User \"test_user\" is not in the forced variation map."), Times.Once);
LoggerMock.Verify(l => l.Log(LogLevel.DEBUG, "Assigned bucket [3037] to user [test_user] with bucketing ID [test_user]."), Times.Once);
LoggerMock.Verify(l => l.Log(LogLevel.INFO, "User [test_user] is in variation [control] of experiment [test_experiment]."), Times.Once);
LoggerMock.Verify(l => l.Log(LogLevel.ERROR, "[UserAttributes] Null value for key null_value removed and will not be sent to results."), Times.Once);
LoggerMock.Verify(l => l.Log(LogLevel.ERROR, "[UserAttributes] Null value for key wont_be_sent removed and will not be sent to results."), Times.Once);
LoggerMock.Verify(l => l.Log(LogLevel.ERROR, "[UserAttributes] Null value for key bad_food removed and will not be sent to results."), Times.Once);
LoggerMock.Verify(l => l.Log(LogLevel.INFO, "Activating user test_user in experiment test_experiment."), Times.Once);
LoggerMock.Verify(l => l.Log(LogLevel.DEBUG, @"Dispatching impression event to URL logx.optimizely.com/decision with params {""param1"":""val1""}."), Times.Once);

Expand Down Expand Up @@ -683,7 +683,7 @@ public void TestTrackWithNullAttributesWithNullEventValue()
{ "wont_send_null", null}
});

LoggerMock.Verify(l => l.Log(It.IsAny<LogLevel>(), It.IsAny<string>()), Times.Exactly(21));
LoggerMock.Verify(l => l.Log(It.IsAny<LogLevel>(), It.IsAny<string>()), Times.Exactly(18));

LoggerMock.Verify(l => l.Log(LogLevel.DEBUG, "User \"test_user\" is not in the forced variation map."), Times.Exactly(3));
LoggerMock.Verify(l => l.Log(LogLevel.DEBUG, "Assigned bucket [3037] to user [test_user] with bucketing ID [test_user]."));
Expand All @@ -698,9 +698,6 @@ public void TestTrackWithNullAttributesWithNullEventValue()
LoggerMock.Verify(l => l.Log(LogLevel.INFO, "This decision will not be saved since the UserProfileService is null."));
LoggerMock.Verify(l => l.Log(LogLevel.INFO, "Experiment \"paused_experiment\" is not running."));
LoggerMock.Verify(l => l.Log(LogLevel.INFO, "Not tracking user \"test_user\" for experiment \"paused_experiment\""));
LoggerMock.Verify(l => l.Log(LogLevel.ERROR, "[UserAttributes] Null value for key null_value removed and will not be sent to results."));
LoggerMock.Verify(l => l.Log(LogLevel.ERROR, "[UserAttributes] Null value for key wont_be_sent removed and will not be sent to results."));
LoggerMock.Verify(l => l.Log(LogLevel.ERROR, "[UserAttributes] Null value for key bad_food removed and will not be sent to results."));
LoggerMock.Verify(l => l.Log(LogLevel.ERROR, "[EventTags] Null value for key wont_send_null removed and will not be sent to results."));
LoggerMock.Verify(l => l.Log(LogLevel.INFO, "Tracking event purchase for user test_user."));
LoggerMock.Verify(l => l.Log(LogLevel.DEBUG, "Dispatching conversion event to URL logx.optimizely.com/track with params {\"param1\":\"val1\"}."));
Expand Down
44 changes: 43 additions & 1 deletion OptimizelySDK.Tests/UtilsTests/ValidatorTest.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2017, Optimizely
* Copyright 2017-2018, Optimizely
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -126,6 +126,7 @@ public void TestAreEventTagsValidValidEventTags()
}));
}

[Test]
public void TestAreEventTagsValidInvalidTags()
{
// Some of the tests cases are not applicable because C# is strongly typed.
Expand All @@ -136,5 +137,46 @@ public void TestAreEventTagsValidInvalidTags()
}));
}

[Test]
public void TestIsUserAttributeValidWithValidValues()
{
var userAttributes = new UserAttributes
{
{ "device_type", "Android" },
{ "is_firefox", true },
{ "num_users", 15 },
{ "pi_value", 3.14 }
};

foreach (var attribute in userAttributes)
Assert.True(Validator.IsUserAttributeValid(attribute));
}

[Test]
public void TestIsUserAttributeValidWithInvalidValues()
{
var invalidUserAttributes = new UserAttributes
{
{ "objects", new object() },
{ "arrays", new string[] { "a", "b", "c" } }
};

foreach (var attribute in invalidUserAttributes)
Assert.False(Validator.IsUserAttributeValid(attribute));
}

[Test]
public void TestIsUserAttributeValidWithEmptyKeyOrValue()
{
var validUserAttributes = new UserAttributes
{
{ "", "Android" },
{ "integer", 0 },
{ "string", string.Empty }
};

foreach (var attribute in validUserAttributes)
Assert.True(Validator.IsUserAttributeValid(attribute));
}
}
}
13 changes: 1 addition & 12 deletions OptimizelySDK/Entity/UserAttributes.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,6 @@ namespace OptimizelySDK.Entity
{
public class UserAttributes : Dictionary<string, object>
{
public UserAttributes FilterNullValues(ILogger logger)
{
UserAttributes answer = new UserAttributes();
foreach (KeyValuePair<string, object> pair in this) {
if (pair.Value != null) {
answer[pair.Key] = pair.Value;
} else {
logger.Log(LogLevel.ERROR, string.Format("[UserAttributes] Null value for key {0} removed and will not be sent to results.", pair.Key));
}
}
return answer;
}

}
}
22 changes: 13 additions & 9 deletions OptimizelySDK/Event/Builder/EventBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ private Dictionary<string, object> GetCommonParams(ProjectConfig config, string
{ "snapshots", new object[0]},
{ "visitor_id", userId },
{ "attributes", new object[0] }
};
};

comonParams[Params.VISITORS] = new object[] { visitor };
comonParams[Params.PROJECT_ID] = config.ProjectId;
Expand All @@ -97,20 +97,23 @@ private Dictionary<string, object> GetCommonParams(ProjectConfig config, string

var userFeatures = new List<Dictionary<string, object>>();

foreach (var userAttribute in userAttributes.Where(a => !string.IsNullOrEmpty(a.Key))) {
var attributeId = config.GetAttributeId(userAttribute.Key);
//Omit attribute values that are not supported by the log endpoint.
foreach (var validUserAttribute in userAttributes.Where(attribute => Validator.IsUserAttributeValid(attribute)))
{
var attributeId = config.GetAttributeId(validUserAttribute.Key);
if (!string.IsNullOrEmpty(attributeId)) {
userFeatures.Add(new Dictionary<string, object>
{
{ "entity_id", attributeId },
{ "key", userAttribute.Key },
{ "type", CUSTOM_ATTRIBUTE_FEATURE_TYPE },
{ "value", userAttribute.Value}
});
{ "entity_id", attributeId },
{ "key", validUserAttribute.Key },
{ "type", CUSTOM_ATTRIBUTE_FEATURE_TYPE },
{ "value", validUserAttribute.Value}
});
}
}

if (config.BotFiltering.HasValue) {
if (config.BotFiltering.HasValue)
{
userFeatures.Add(new Dictionary<string, object>
{
{ "entity_id", ControlAttributes.BOT_FILTERING_ATTRIBUTE },
Expand All @@ -121,6 +124,7 @@ private Dictionary<string, object> GetCommonParams(ProjectConfig config, string
}

visitor["attributes"] = userFeatures;

return comonParams;
}

Expand Down
9 changes: 0 additions & 9 deletions OptimizelySDK/Optimizely.cs
Original file line number Diff line number Diff line change
Expand Up @@ -195,10 +195,6 @@ public Variation Activate(string experimentKey, string userId, UserAttributes us
return null;
}

if (userAttributes != null) {
userAttributes = userAttributes.FilterNullValues(Logger);
}

SendImpressionEvent(experiment, variation, userId, userAttributes);

return variation;
Expand Down Expand Up @@ -270,11 +266,6 @@ public void Track(string eventKey, string userId, UserAttributes userAttributes
if (validExperimentIdToVariationMap.Count > 0)
{

if (userAttributes != null)
{
userAttributes = userAttributes.FilterNullValues(Logger);
}

if (eventTags != null)
{
eventTags = eventTags.FilterNullValues(Logger);
Expand Down
18 changes: 15 additions & 3 deletions OptimizelySDK/Utils/Validator.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2017, Optimizely
* Copyright 2017-2018, Optimizely
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -14,6 +14,7 @@
* limitations under the License.
*/
using OptimizelySDK.Entity;
using System;
using System.Collections.Generic;
using System.Linq;
using Attribute = OptimizelySDK.Entity.Attribute;
Expand All @@ -22,7 +23,6 @@ namespace OptimizelySDK.Utils
{
public static class Validator
{

/// <summary>
/// Validate the ProjectConfig JSON
/// </summary>
Expand Down Expand Up @@ -87,7 +87,7 @@ public static bool AreEventTagsValid(Dictionary<string, object> eventTags) {
public static bool IsFeatureFlagValid(ProjectConfig projectConfig, FeatureFlag featureFlag)
{
var experimentIds = featureFlag.ExperimentIds;

if (experimentIds == null || experimentIds.Count <= 1)
return true;

Expand All @@ -102,6 +102,18 @@ public static bool IsFeatureFlagValid(ProjectConfig projectConfig, FeatureFlag f

return true;
}

/// <summary>
/// Determine if given user attribute is valid.
/// </summary>
/// <param name="attribute">Attribute key and value pair</param>
/// <returns>true if attribute key is not null and value is one of the supported type, false otherwise</returns>
public static bool IsUserAttributeValid(KeyValuePair<string, object> attribute)
{
return (attribute.Key != null) &&
(attribute.Value is int || attribute.Value is string || attribute.Value is double
|| attribute.Value is bool || attribute.Value is float || attribute.Value is long);
}
}
}