Skip to content

bot filtering #79

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 7 commits into from
Jun 27, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
4 changes: 2 additions & 2 deletions OptimizelySDK.Tests/DecisionServiceTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -388,13 +388,13 @@ public void TestGetVariationWithBucketingId()
{"device_type", "iPhone"},
{"company", "Optimizely"},
{"location", "San Francisco"},
{DecisionService.RESERVED_ATTRIBUTE_KEY_BUCKETING_ID, testBucketingIdVariation}
{DecisionService.BUCKETING_ID_ATTRIBUTE, testBucketingIdVariation}
};

var invalidUserAttributesWithBucketingId = new UserAttributes
{
{"company", "Optimizely"},
{DecisionService.RESERVED_ATTRIBUTE_KEY_BUCKETING_ID, testBucketingIdControl}
{DecisionService.BUCKETING_ID_ATTRIBUTE, testBucketingIdControl}
};

var optlyObject = new Optimizely(TestData.Datafile, new ValidEventDispatcher(), LoggerMock.Object);
Expand Down
493 changes: 480 additions & 13 deletions OptimizelySDK.Tests/EventTests/EventBuilderTest.cs

Large diffs are not rendered by default.

5 changes: 2 additions & 3 deletions OptimizelySDK.Tests/OptimizelyTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -515,8 +515,7 @@ public void TestTrackInvalidAttributes()

Optimizely.Track("purchase", TestUserId, attributes);

//LoggerMock.Verify(l => l.Log(LogLevel.ERROR, "Provided attributes are in an invalid format."), Times.Once);
ErrorHandlerMock.Verify(e => e.HandleError(It.IsAny<InvalidAttributeException>()), Times.Once);
LoggerMock.Verify(l => l.Log(LogLevel.ERROR, @"Attribute key ""abc"" is not in datafile."), Times.Once);
}

[Test]
Expand Down Expand Up @@ -1111,7 +1110,7 @@ public void TestGetVariationBucketingIdAttribute()
{ "device_type", "iPhone" },
{ "company", "Optimizely" },
{ "location", "San Francisco" },
{ DecisionService.RESERVED_ATTRIBUTE_KEY_BUCKETING_ID, testBucketingIdVariation }
{ DecisionService.BUCKETING_ID_ATTRIBUTE, testBucketingIdVariation }
};

// confirm that a valid variation is bucketed without the bucketing ID
Expand Down
54 changes: 53 additions & 1 deletion OptimizelySDK.Tests/ProjectConfigTest.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 @@ -22,6 +22,8 @@
using OptimizelySDK.Exceptions;
using NUnit.Framework;
using OptimizelySDK.Entity;
using Newtonsoft.Json;
using OptimizelySDK.Event.Builder;

namespace OptimizelySDK.Tests
{
Expand Down Expand Up @@ -810,5 +812,55 @@ public void TestVariationFeatureEnabledProperty()
var variation = Config.GetVariationFromKey("test_experiment", "control");
Assert.IsFalse(variation.IsFeatureEnabled);
}

[Test]
public void TestBotFilteringValues()
{
// Verify that bot filtering value is true as defined in Config data.
Assert.True(Config.BotFiltering.GetValueOrDefault());

// Remove botFilering node and verify returned value in null.
JObject projConfig = JObject.Parse(TestData.Datafile);
if (projConfig.TryGetValue("botFiltering", out JToken token))
{
projConfig.Property("botFiltering").Remove();
var configWithoutBotFilter = ProjectConfig.Create(JsonConvert.SerializeObject(projConfig),
LoggerMock.Object, ErrorHandlerMock.Object);

// Verify that bot filtering is null when not defined in datafile.
Assert.Null(configWithoutBotFilter.BotFiltering);
}
}

[Test]
public void TestGetAttributeIdWithReservedPrefix()
{
// Verify that attribute key is returned for reserved attribute key.
Assert.AreEqual(Config.GetAttributeId(EventBuilder.USER_AGENT_ATTRIBUTE), EventBuilder.USER_AGENT_ATTRIBUTE);

// Verify that attribute Id is returned for attribute key with reserved prefix that does not exist in datafile.
Assert.AreEqual(Config.GetAttributeId(EventBuilder.USER_AGENT_ATTRIBUTE), EventBuilder.USER_AGENT_ATTRIBUTE);

Choose a reason for hiding this comment

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

nit. This test is no different from the previous test. Let's have a separate example. Like lets test for $opt_reserved_prefix_attribute


// Create config file copy with additional resered prefix attribute.
string reservedPrefixAttrKey = "$opt_user_defined_attribute";
JObject projConfig = JObject.Parse(TestData.Datafile);
var attributes = (JArray)projConfig["attributes"];

var reservedAttr = new Entity.Attribute { Id = "7723348204", Key = reservedPrefixAttrKey };
attributes.Add((JObject)JToken.FromObject(reservedAttr));

// Verify that attribute Id is returned and warning is logged for attribute key with reserved prefix that exists in datafile.
var reservedAttrConfig = ProjectConfig.Create(JsonConvert.SerializeObject(projConfig), LoggerMock.Object, ErrorHandlerMock.Object);
Assert.AreEqual(reservedAttrConfig.GetAttributeId(reservedPrefixAttrKey), reservedAttrConfig.GetAttribute(reservedPrefixAttrKey).Id);
LoggerMock.Verify(l => l.Log(LogLevel.WARN, $@"Attribute {reservedPrefixAttrKey} unexpectedly has reserved prefix {ProjectConfig.RESERVED_ATTRIBUTE_PREFIX}; using attribute ID instead of reserved attribute name."));
}

[Test]
public void TestGetAttributeIdWithInvalidAttributeKey()
{
// Verify that null is returned when provided attribute key is invalid.
Assert.Null(Config.GetAttributeId("invalid_attribute"));
LoggerMock.Verify(l => l.Log(LogLevel.ERROR, @"Attribute key ""invalid_attribute"" is not in datafile."));
}
}
}
3 changes: 2 additions & 1 deletion OptimizelySDK.Tests/TestData.json
Original file line number Diff line number Diff line change
Expand Up @@ -634,5 +634,6 @@
"key": "purchase"
}],
"revision": "15",
"anonymizeIP": "false"
"anonymizeIP": "false",

Choose a reason for hiding this comment

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

This will always be a boolean value

"botFiltering": "true"

Choose a reason for hiding this comment

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

This will always be a boolean value.

}

Choose a reason for hiding this comment

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

nit. Add new line at end of file.

6 changes: 3 additions & 3 deletions OptimizelySDK/Bucketing/DecisionService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ namespace OptimizelySDK.Bucketing
/// </summary>
public class DecisionService
{
public const string RESERVED_ATTRIBUTE_KEY_BUCKETING_ID = "$opt_bucketing_id";
public const string BUCKETING_ID_ATTRIBUTE = "$opt_bucketing_id";

private Bucketer Bucketer;
private IErrorHandler ErrorHandler;
Expand Down Expand Up @@ -401,9 +401,9 @@ private string GetBucketingId(string userId, UserAttributes filteredAttributes)
string bucketingId = userId;

// If the bucketing ID key is defined in attributes, then use that in place of the userID for the murmur hash key
if (filteredAttributes != null && filteredAttributes.ContainsKey(RESERVED_ATTRIBUTE_KEY_BUCKETING_ID))
if (filteredAttributes != null && filteredAttributes.ContainsKey(BUCKETING_ID_ATTRIBUTE))
{
bucketingId = filteredAttributes[RESERVED_ATTRIBUTE_KEY_BUCKETING_ID];
bucketingId = filteredAttributes[BUCKETING_ID_ATTRIBUTE];
Logger.Log(LogLevel.DEBUG, string.Format("Setting the bucketing ID to \"{0}\"", bucketingId));
}

Expand Down
44 changes: 20 additions & 24 deletions OptimizelySDK/Event/Builder/EventBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@ public class EventBuilder

private const string ACTIVATE_EVENT_KEY = "campaign_activated";

public const string RESERVED_ATTRIBUTE_KEY_BUCKETING_ID_EVENT_PARAM_KEY = "optimizely_bucketing_id";
public const string BOT_FILTERING_ATTRIBUTE = "$opt_bot_filtering";

public const string USER_AGENT_ATTRIBUTE = "$opt_user_agent";

Choose a reason for hiding this comment

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

nit. Like in other SDKs, lets consolidate all of the "special" attributes i.e. bucketing ID, bot filtering and user agent attributes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done.


private static readonly Dictionary<string, string> HTTP_HEADERS = new Dictionary<string, string>
{
Expand Down Expand Up @@ -103,37 +105,31 @@ private Dictionary<string, object> GetCommonParams(ProjectConfig config, string

foreach (var userAttribute in userAttributes.Where(a => !string.IsNullOrEmpty(a.Key)))
{
if (string.Equals(userAttribute.Key, DecisionService.RESERVED_ATTRIBUTE_KEY_BUCKETING_ID))
var attributeId = config.GetAttributeId(userAttribute.Key);
if (!string.IsNullOrEmpty(attributeId))
{
var userFeature = new Dictionary<string, object>
userFeatures.Add(new Dictionary<string, object>
{
{ "entity_id", DecisionService.RESERVED_ATTRIBUTE_KEY_BUCKETING_ID },
{ "key", RESERVED_ATTRIBUTE_KEY_BUCKETING_ID_EVENT_PARAM_KEY },
{ "entity_id", attributeId },
{ "key", userAttribute.Key },
{ "type", CUSTOM_ATTRIBUTE_FEATURE_TYPE },
{ "value", userAttribute.Value}
};
userFeatures.Add(userFeature);
}
else
{
var attributeEntity = config.GetAttribute(userAttribute.Key);
if (attributeEntity != null && attributeEntity.Key != null)
{
var userFeature = new Dictionary<string, object>
{
{ "entity_id", attributeEntity.Id },
{ "key", attributeEntity.Key },
{ "type", CUSTOM_ATTRIBUTE_FEATURE_TYPE },
{ "value", userAttribute.Value}
};
userFeatures.Add(userFeature);
}
});
}
}


if (config.BotFiltering.HasValue)
{
userFeatures.Add(new Dictionary<string, object>
{
{ "entity_id", BOT_FILTERING_ATTRIBUTE },
{ "key", BOT_FILTERING_ATTRIBUTE },
{ "type", CUSTOM_ATTRIBUTE_FEATURE_TYPE },
{ "value", config.BotFiltering}
});
}

visitor["attributes"] = userFeatures;

return comonParams;
}

Expand Down
1 change: 1 addition & 0 deletions OptimizelySDK/Logger/ILogger.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ public enum LogLevel
DEBUG,
INFO,
ERROR,
WARN,

Choose a reason for hiding this comment

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

WARN should go above ERROR IMO

Choose a reason for hiding this comment

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

Also, update year

}

public interface ILogger
Expand Down
36 changes: 35 additions & 1 deletion OptimizelySDK/ProjectConfig.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 @@ -25,6 +25,8 @@ namespace OptimizelySDK
{
public class ProjectConfig
{
public const string RESERVED_ATTRIBUTE_PREFIX = "$opt_";

/// <summary>
/// Version of the datafile.
/// </summary>
Expand Down Expand Up @@ -53,6 +55,11 @@ public class ProjectConfig
/// </summary>
public bool AnonymizeIP { get; set; }

/// <summary>
/// Bot filtering flag.
/// </summary>
public bool? BotFiltering { get; set; }

//========================= Mappings ===========================

/// <summary>
Expand Down Expand Up @@ -552,5 +559,32 @@ public Rollout GetRolloutFromId(string rolloutId)
ErrorHandler.HandleError(new Exceptions.InvalidRolloutException("Provided rollout is not in datafile."));
return new Rollout();
}

/// <summary>
/// Get attribute ID for the provided attribute key
/// </summary>
/// <param name="attributeKey">Key of the Attribute</param>
/// <returns>Attribute ID corresponding to the provided attribute key. Attribute key if it is a reserved attribute</returns>
public string GetAttributeId(string attributeKey)

Choose a reason for hiding this comment

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

Please add unit tests for this new module

Copy link
Contributor

@mfahadahmed mfahadahmed Jun 13, 2018

Choose a reason for hiding this comment

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

Already covered here:

public void TestGetAttributeIdWithReservedPrefix()

public void TestGetAttributeIdWithInvalidAttributeKey()

Choose a reason for hiding this comment

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

Oh, good. GitHub does not render large files by default and hence missed it.

{

var hasReservedPrefix = attributeKey.StartsWith(RESERVED_ATTRIBUTE_PREFIX);

if (_AttributeKeyMap.ContainsKey(attributeKey))
{
var attribute = _AttributeKeyMap[attributeKey];
if (hasReservedPrefix)
Logger.Log(LogLevel.WARN, $@"Attribute {attributeKey} unexpectedly has reserved prefix {RESERVED_ATTRIBUTE_PREFIX}; using attribute ID instead of reserved attribute name.");

return attribute.Id;
}
else if (hasReservedPrefix)
{
return attributeKey;
}

Logger.Log(LogLevel.ERROR, $@"Attribute key ""{attributeKey}"" is not in datafile.");

Choose a reason for hiding this comment

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

nit. Just single pair of double quotes should be good right?

Copy link
Contributor

Choose a reason for hiding this comment

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

In verbatim strings, 2 double-quotes are used as an escape character like double backslashes in normal strings. The final string will contain attribute key in single double-quotes.

return null;
}
}
}