Skip to content

Add IP anonymization to event payload. #42

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.

Sign up for GitHub

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 3 commits into from
Dec 14, 2016
Merged

Add IP anonymization to event payload. #42

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ac3df12
Add IP anonymization to event payload.
Dec 14, 2016
6d7e344
Address feedback from vignesh.
Dec 14, 2016
91a6eef
Address some more nits.
Dec 14, 2016
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
12 changes: 9 additions & 3 deletions core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ public String toString() {
private final String projectId;
private final String revision;
private final String version;
private final boolean anonymizeIP;
Copy link
Contributor

@vraja2 vraja2 Dec 14, 2016

Choose a reason for hiding this comment

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

just for consistency, can you modify toString to include anonymizeIP as well?

Copy link
Contributor Author

@mikeproeng37 mikeproeng37 Dec 14, 2016

Choose a reason for hiding this comment

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

Will do

private final List<Group> groups;
private final List<Experiment> experiments;
private final List<Attribute> attributes;
Expand All @@ -78,19 +79,20 @@ public String toString() {

public ProjectConfig(String accountId, String projectId, String version, String revision, List<Group> groups,
List<Experiment> experiments, List<Attribute> attributes, List<EventType> eventType,
List<Audience> audiences) {
this(accountId, projectId, version, revision, groups, experiments, attributes, eventType, audiences,
List<Audience> audiences, boolean anonymizeIP) {
Copy link
Contributor

@vraja2 vraja2 Dec 14, 2016

Choose a reason for hiding this comment

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

we only need to add anonymizeIP to one ProjectConfig constructor. You can remove it here and call the other constructor with false as the default value.

Copy link
Contributor

@wangjoshuah wangjoshuah Dec 14, 2016

Choose a reason for hiding this comment

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

why not?

Copy link
Contributor Author

@mikeproeng37 mikeproeng37 Dec 14, 2016

Choose a reason for hiding this comment

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

Will do. This will ensure that we always have it initialized to false unless explicitly passed in to the constructor.

this(accountId, projectId, version, revision, groups, experiments, attributes, eventType, audiences, anonymizeIP,
null);
}

public ProjectConfig(String accountId, String projectId, String version, String revision, List<Group> groups,
List<Experiment> experiments, List<Attribute> attributes, List<EventType> eventType,
List<Audience> audiences, List<LiveVariable> liveVariables) {
List<Audience> audiences, boolean anonymizeIP, List<LiveVariable> liveVariables) {

this.accountId = accountId;
this.projectId = projectId;
this.version = version;
this.revision = revision;
this.anonymizeIP = anonymizeIP;

this.groups = Collections.unmodifiableList(groups);
List<Experiment> allExperiments = new ArrayList<Experiment>();
Expand Down Expand Up @@ -151,6 +153,10 @@ public String getRevision() {
return revision;
}

public boolean getAnonymizeIP() {
return anonymizeIP;
}

public List<Group> getGroups() {
return groups;
}
Expand Down
5 changes: 4 additions & 1 deletion core-api/src/main/java/com/optimizely/ab/config/parser/JsonConfigParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,16 @@ public ProjectConfig parseProjectConfig(@Nonnull String json) throws ConfigParse
List<Audience> audiences = parseAudiences(rootObject.getJSONArray("audiences"));
List<Group> groups = parseGroups(rootObject.getJSONArray("groups"));

boolean anonymizeIP = false;
List<LiveVariable> liveVariables = null;
if (version.equals(ProjectConfig.Version.V3.toString())) {
liveVariables = parseLiveVariables(rootObject.getJSONArray("variables"));

anonymizeIP = rootObject.getBoolean("anonymizeIP");
}

return new ProjectConfig(accountId, projectId, version, revision, groups, experiments, attributes, events,
audiences, liveVariables);
audiences, anonymizeIP, liveVariables);
} catch (Exception e) {
throw new ConfigParseException("Unable to parse datafile: " + json, e);
}
Expand Down
5 changes: 4 additions & 1 deletion core-api/src/main/java/com/optimizely/ab/config/parser/JsonSimpleConfigParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,16 @@ public ProjectConfig parseProjectConfig(@Nonnull String json) throws ConfigParse
List<Audience> audiences = parseAudiences((JSONArray)parser.parse(rootObject.get("audiences").toString()));
List<Group> groups = parseGroups((JSONArray)rootObject.get("groups"));

boolean anonymizeIP = false;
List<LiveVariable> liveVariables = null;
if (version.equals(ProjectConfig.Version.V3.toString())) {
liveVariables = parseLiveVariables((JSONArray)rootObject.get("variables"));

anonymizeIP = (Boolean)rootObject.get("anonymizeIP");
}

return new ProjectConfig(accountId, projectId, version, revision, groups, experiments, attributes, events,
audiences, liveVariables);
audiences, anonymizeIP, liveVariables);
} catch (Exception e) {
throw new ConfigParseException("Unable to parse datafile: " + json, e);
}
Expand Down
5 changes: 4 additions & 1 deletion core-api/src/main/java/com/optimizely/ab/config/parser/ProjectConfigGsonDeserializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,14 +72,17 @@ public ProjectConfig deserialize(JsonElement json, Type typeOfT, JsonDeserializa
List<Audience> audiences =
context.deserialize(jsonObject.get("audiences").getAsJsonArray(), audienceType);

boolean anonymizeIP = false;
// live variables should be null if using V1
List<LiveVariable> liveVariables = null;
if (version.equals(ProjectConfig.Version.V3.toString())) {
Type liveVariablesType = new TypeToken<List<LiveVariable>>() {}.getType();
liveVariables = context.deserialize(jsonObject.getAsJsonArray("variables"), liveVariablesType);

anonymizeIP = jsonObject.get("anonymizeIP").getAsBoolean();
}

return new ProjectConfig(accountId, projectId, version, revision, groups, experiments, attributes, events,
audiences, liveVariables);
audiences, anonymizeIP, liveVariables);
}
}
4 changes: 3 additions & 1 deletion core-api/src/main/java/com/optimizely/ab/config/parser/ProjectConfigJacksonDeserializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,15 @@ public ProjectConfig deserialize(JsonParser parser, DeserializationContext conte
List<Audience> audiences = mapper.readValue(node.get("audiences").toString(),
new TypeReference<List<Audience>>() {});

boolean anonymizeIP = false;
List<LiveVariable> liveVariables = null;
if (version.equals(ProjectConfig.Version.V3.toString())) {
liveVariables = mapper.readValue(node.get("variables").toString(),
new TypeReference<List<LiveVariable>>() {});
anonymizeIP = node.get("anonymizeIP").asBoolean();
}

return new ProjectConfig(accountId, projectId, version, revision, groups, experiments, attributes, events,
audiences, liveVariables);
audiences, anonymizeIP, liveVariables);
}
}
6 changes: 4 additions & 2 deletions core-api/src/main/java/com/optimizely/ab/event/internal/EventBuilderV2.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ public class EventBuilderV2 extends EventBuilder {

private static final Logger logger = LoggerFactory.getLogger(EventBuilderV2.class);

static final String IMPRESSION_ENDPOINT = "https://p13nlog.dz.optimizely.com/log/decision";
static final String CONVERSION_ENDPOINT = "https://p13nlog.dz.optimizely.com/log/event";
static final String IMPRESSION_ENDPOINT = "https://logx.optimizely.com/log/decision";
static final String CONVERSION_ENDPOINT = "https://logx.optimizely.com/log/event";

@VisibleForTesting
public final ClientEngine clientEngine;
Expand Down Expand Up @@ -95,6 +95,7 @@ public LogEvent createImpressionEvent(@Nonnull ProjectConfig projectConfig,
impressionPayload.setUserFeatures(createFeatures(attributes, projectConfig));
impressionPayload.setClientEngine(clientEngine);
impressionPayload.setClientVersion(clientVersion);
impressionPayload.setAnonymizeIP(projectConfig.getAnonymizeIP());

String payload = this.serializer.serialize(impressionPayload);
return new LogEvent(RequestMethod.POST, IMPRESSION_ENDPOINT, Collections.<String, String>emptyMap(), payload);
Expand Down Expand Up @@ -133,6 +134,7 @@ public LogEvent createConversionEvent(@Nonnull ProjectConfig projectConfig,

conversionPayload.setEventFeatures(Collections.<Feature>emptyList());
conversionPayload.setIsGlobalHoldback(false);
conversionPayload.setAnonymizeIP(projectConfig.getAnonymizeIP());
conversionPayload.setClientEngine(clientEngine);
conversionPayload.setClientVersion(clientVersion);

Expand Down
9 changes: 8 additions & 1 deletion core-api/src/main/java/com/optimizely/ab/event/internal/payload/Conversion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,13 @@ public class Conversion extends Event {
private List<EventMetric> eventMetrics;
private List<Feature> eventFeatures;
private boolean isGlobalHoldback;
private boolean anonymizeIP;

public Conversion() { }

public Conversion(String visitorId, long timestamp, String projectId, String accountId, List<Feature> userFeatures,
List<LayerState> layerStates, String eventEntityId, String eventName,
List<EventMetric> eventMetrics, List<Feature> eventFeatures, boolean isGlobalHoldback) {
List<EventMetric> eventMetrics, List<Feature> eventFeatures, boolean isGlobalHoldback, boolean anonymizeIP) {
this.visitorId = visitorId;
this.timestamp = timestamp;
this.projectId = projectId;
Expand All @@ -48,6 +49,7 @@ public Conversion(String visitorId, long timestamp, String projectId, String acc
this.eventMetrics = eventMetrics;
this.eventFeatures = eventFeatures;
this.isGlobalHoldback = isGlobalHoldback;
this.anonymizeIP = anonymizeIP;
}

public String getVisitorId() {
Expand Down Expand Up @@ -138,6 +140,10 @@ public void setIsGlobalHoldback(boolean globalHoldback) {
this.isGlobalHoldback = globalHoldback;
}

public boolean getAnonymizeIP() { return anonymizeIP; }

public void setAnonymizeIP(boolean anonymizeIP) { this.anonymizeIP = anonymizeIP; }

@Override
public boolean equals(Object other) {
if (!(other instanceof Conversion))
Expand Down Expand Up @@ -192,6 +198,7 @@ public String toString() {
", eventMetrics=" + eventMetrics +
", eventFeatures=" + eventFeatures +
", isGlobalHoldback=" + isGlobalHoldback +
", anonymizeIP=" + anonymizeIP +
Copy link
Contributor

@wangjoshuah wangjoshuah Dec 14, 2016

Choose a reason for hiding this comment

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

should we have constants for all these param values?

Copy link
Contributor Author

@mikeproeng37 mikeproeng37 Dec 14, 2016

Choose a reason for hiding this comment

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

Yeah we should at some point since we share these with Impression.java. Just don't think it's worth refactoring for this diff :)

", clientEngine='" + clientEngine +
", clientVersion='" + clientVersion + '}';
}
Expand Down
9 changes: 8 additions & 1 deletion core-api/src/main/java/com/optimizely/ab/event/internal/payload/Impression.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,12 @@ public class Impression extends Event {
private String layerId;
private String accountId;
private List<Feature> userFeatures;
private boolean anonymizeIP;

public Impression() { }

public Impression(String visitorId, long timestamp, boolean isGlobalHoldback, String projectId, Decision decision,
String layerId, String accountId, List<Feature> userFeatures) {
String layerId, String accountId, List<Feature> userFeatures, boolean anonymizeIP) {
this.visitorId = visitorId;
this.timestamp = timestamp;
this.isGlobalHoldback = isGlobalHoldback;
Expand All @@ -41,6 +42,7 @@ public Impression(String visitorId, long timestamp, boolean isGlobalHoldback, St
this.layerId = layerId;
this.accountId = accountId;
this.userFeatures = userFeatures;
this.anonymizeIP = anonymizeIP;
}

public String getVisitorId() {
Expand Down Expand Up @@ -107,6 +109,10 @@ public void setUserFeatures(List<Feature> userFeatures) {
this.userFeatures = userFeatures;
}

public boolean getAnonymizeIP() { return anonymizeIP; }
Copy link
Contributor

@vraja2 vraja2 Dec 14, 2016

Choose a reason for hiding this comment

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

nit: can you format this to be on multiple lines?


public void setAnonymizeIP(boolean anonymizeIP) { this.anonymizeIP = anonymizeIP; }
Copy link
Contributor

@vraja2 vraja2 Dec 14, 2016

Choose a reason for hiding this comment

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

nit: can you format this to be on multiple lines?


@Override
public boolean equals(Object other) {
if (!(other instanceof Impression))
Expand Down Expand Up @@ -147,6 +153,7 @@ public String toString() {
"visitorId='" + visitorId + '\'' +
", timestamp=" + timestamp +
", isGlobalHoldback=" + isGlobalHoldback +
", anonymizeIP=" + anonymizeIP +
", projectId='" + projectId + '\'' +
", decision=" + decision +
", layerId='" + layerId + '\'' +
Expand Down
2 changes: 2 additions & 0 deletions core-api/src/main/java/com/optimizely/ab/event/internal/serializer/JsonSimpleSerializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ private JSONObject serializeImpression(Impression impression) {
jsonObject.put("visitorId", impression.getVisitorId());
jsonObject.put("timestamp", impression.getTimestamp());
jsonObject.put("isGlobalHoldback", impression.getIsGlobalHoldback());
jsonObject.put("anonymizeIP", impression.getAnonymizeIP());
jsonObject.put("projectId", impression.getProjectId());
jsonObject.put("decision", serializeDecision(impression.getDecision()));
jsonObject.put("layerId", impression.getLayerId());
Expand All @@ -72,6 +73,7 @@ private JSONObject serializeConversion(Conversion conversion) {
jsonObject.put("eventMetrics", serializeEventMetrics(conversion.getEventMetrics()));
jsonObject.put("eventFeatures", serializeFeatures(conversion.getEventFeatures()));
jsonObject.put("isGlobalHoldback", conversion.getIsGlobalHoldback());
jsonObject.put("anonymizeIP", conversion.getAnonymizeIP());
jsonObject.put("clientEngine", conversion.getClientEngine());
jsonObject.put("clientVersion", conversion.getClientVersion());

Expand Down
12 changes: 6 additions & 6 deletions core-api/src/test/java/com/optimizely/ab/config/ProjectConfigTestUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ private static ProjectConfig generateValidProjectConfigV1() {
Collections.<TrafficAllocation>emptyList());
List<Group> groups = asList(randomPolicyGroup, overlappingPolicyGroup);

return new ProjectConfig("789", "1234", "1", "42", groups, experiments, attributes, events, audiences);
return new ProjectConfig("789", "1234", "1", "42", groups, experiments, attributes, events, audiences, true);
Copy link
Contributor

@vraja2 vraja2 Dec 14, 2016

Choose a reason for hiding this comment

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

don't need to modify this if you change the constructor

}

private static final ProjectConfig NO_AUDIENCE_PROJECT_CONFIG_V1 = generateNoAudienceProjectConfigV1();
Expand Down Expand Up @@ -176,7 +176,7 @@ private static ProjectConfig generateNoAudienceProjectConfigV1() {
new EventType("099", "clicked_purchase", multipleExperimentIds));

return new ProjectConfig("789", "1234", "1", "42", Collections.<Group>emptyList(), experiments, attributes,
events, Collections.<Audience>emptyList());
events, Collections.<Audience>emptyList(), true);
Copy link
Contributor

@vraja2 vraja2 Dec 14, 2016

Choose a reason for hiding this comment

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

don't need to modify

}

private static final ProjectConfig VALID_PROJECT_CONFIG_V2 = generateValidProjectConfigV2();
Expand Down Expand Up @@ -270,7 +270,7 @@ private static ProjectConfig generateValidProjectConfigV2() {
Collections.<TrafficAllocation>emptyList());
List<Group> groups = asList(randomPolicyGroup, overlappingPolicyGroup);

return new ProjectConfig("789", "1234", "2", "42", groups, experiments, attributes, events, audiences);
return new ProjectConfig("789", "1234", "2", "42", groups, experiments, attributes, events, audiences, true);
Copy link
Contributor

@vraja2 vraja2 Dec 14, 2016

Choose a reason for hiding this comment

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

don't need to modify

}

private static final ProjectConfig NO_AUDIENCE_PROJECT_CONFIG_V2 = generateNoAudienceProjectConfigV2();
Expand Down Expand Up @@ -307,7 +307,7 @@ private static ProjectConfig generateNoAudienceProjectConfigV2() {
new EventType("099", "clicked_purchase", multipleExperimentIds));

return new ProjectConfig("789", "1234", "2", "42", Collections.<Group>emptyList(), experiments, attributes,
events, Collections.<Audience>emptyList());
events, Collections.<Audience>emptyList(), true);
Copy link
Contributor

@vraja2 vraja2 Dec 14, 2016

Choose a reason for hiding this comment

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

don't need to modify

}

private static final ProjectConfig VALID_PROJECT_CONFIG_V3 = generateValidProjectConfigV3();
Expand Down Expand Up @@ -437,7 +437,7 @@ private static ProjectConfig generateValidProjectConfigV3() {
);

return new ProjectConfig("789", "1234", "3", "42", groups, experiments, attributes, events, audiences,
liveVariables);
true, liveVariables);
}

private static final ProjectConfig NO_AUDIENCE_PROJECT_CONFIG_V3 = generateNoAudienceProjectConfigV3();
Expand Down Expand Up @@ -474,7 +474,7 @@ private static ProjectConfig generateNoAudienceProjectConfigV3() {
new EventType("099", "clicked_purchase", multipleExperimentIds));

return new ProjectConfig("789", "1234", "3", "42", Collections.<Group>emptyList(), experiments, attributes,
events, Collections.<Audience>emptyList(), Collections.<LiveVariable>emptyList());
events, Collections.<Audience>emptyList(), true, Collections.<LiveVariable>emptyList());
}

private ProjectConfigTestUtils() { }
Expand Down
2 changes: 2 additions & 0 deletions core-api/src/test/java/com/optimizely/ab/event/internal/EventBuilderV2Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ public void createImpressionEvent() throws Exception {
assertThat(impression.getVisitorId(), is(userId));
assertThat((double)impression.getTimestamp(), closeTo((double)System.currentTimeMillis(), 60.0));
assertFalse(impression.getIsGlobalHoldback());
assertThat(impression.getAnonymizeIP(), is(projectConfig.getAnonymizeIP()));
Copy link
Contributor

@vraja2 vraja2 Dec 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

let's make this comparison explicit. assertFalse in this case since it's a v2 datafile. We should also pass in v2 datafile to the eventbuilder as well as a v3 datafile since for v2, anonymizeIP should be false by default, while v3 will vary

Copy link
Contributor Author

@mikeproeng37 mikeproeng37 Dec 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think testing for that is needed here. This tests that events are built correctly and not the fact that IPAnon should be false by default as that should be tested elsewhere. We should just make sure that the event value corresponds to what the project config shows. That said, I'll add a test for this case on the ProjectConfigTest

Copy link
Contributor

@vraja2 vraja2 Dec 14, 2016

Choose a reason for hiding this comment

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

thanks! that makes sense

assertThat(impression.getProjectId(), is(projectConfig.getProjectId()));
assertThat(impression.getDecision(), is(expectedDecision));
assertThat(impression.getLayerId(), is(activatedExperiment.getLayerId()));
Expand Down Expand Up @@ -207,6 +208,7 @@ public void createConversionEvent() throws Exception {
assertThat(conversion.getEventMetrics(), is(Collections.<EventMetric>emptyList()));
assertThat(conversion.getEventFeatures(), is(Collections.<Feature>emptyList()));
assertFalse(conversion.getIsGlobalHoldback());
assertThat(conversion.getAnonymizeIP(), is(projectConfig.getAnonymizeIP()));
assertThat(conversion.getClientEngine(), is(ClientEngine.JAVA_SDK.getClientEngineValue()));
assertThat(conversion.getClientVersion(), is(BuildVersionInfo.VERSION));
}
Expand Down
2 changes: 2 additions & 0 deletions core-api/src/test/java/com/optimizely/ab/event/internal/serializer/SerializerTestUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ static Impression generateImpression() {
impression.setDecision(decision);
impression.setUserFeatures(userFeatures);
impression.setClientVersion("0.1.1");
impression.setAnonymizeIP(true);

return impression;
}
Expand All @@ -91,6 +92,7 @@ static Conversion generateConversion() {
conversion.setEventFeatures(eventFeatures);
conversion.setIsGlobalHoldback(isGlobalHoldback);
conversion.setClientVersion("0.1.1");
conversion.setAnonymizeIP(true);

return conversion;
}
Expand Down
1 change: 1 addition & 0 deletions core-api/src/test/resources/config/valid-project-config-v3.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"projectId": "1234",
"version": "3",
"revision": "42",
"anonymizeIP": true,
"experiments": [
{
"id": "223",
Expand Down
1 change: 1 addition & 0 deletions core-api/src/test/resources/serializer/conversion.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
],
"eventFeatures": [],
"isGlobalHoldback": false,
"anonymizeIP": true,
"clientEngine": "java-sdk",
"clientVersion": "0.1.1"
}
1 change: 1 addition & 0 deletions core-api/src/test/resources/serializer/impression.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"visitorId": "testvisitor",
"timestamp": 12345,
"isGlobalHoldback": false,
"anonymizeIP": true,
"projectId": "1",
"decision": {
"variationId": "4",
Expand Down