feat: Added ODP RestApi interface for sending Events to ODP #485

zashraf1985 · 2022-08-18T21:07:14Z

Summary

This module provides an internal service for ODP event REST api access.

Test plan

Manually tested thoroughly.
Added new unit tests.
All Existing Unit tests pass.
All existing Full stack compatibility tests pass.

Jira

OASIS-8387

jaeopt

Looks good. A couple of changes suggested.

jaeopt · 2022-08-22T23:55:31Z

core-api/src/main/java/com/optimizely/ab/odp/ODPEvent.java

+    private String type;
+    private String action;
+    private Map<String, String > identifiers;
+    private Map<String, String> data;


Suggested change

private Map<String, String> data;

private Map<String, Object> data;

It can support any type of data (String, Number, Boolean).

jaeopt · 2022-08-22T23:56:25Z

core-api/src/main/java/com/optimizely/ab/odp/ODPEvent.java

+        this.identifiers = identifiers;
+    }
+
+    public Map<String, String> getData() {


Suggested change

public Map<String, String> getData() {

public Map<String, Object> getData() {

jaeopt · 2022-08-22T23:56:36Z

core-api/src/main/java/com/optimizely/ab/odp/ODPEvent.java

+        return data;
+    }
+
+    public void setData(Map<String, String> data) {


Suggested change

public void setData(Map<String, String> data) {

public void setData(Map<String, Object> data) {

jaeopt · 2022-08-23T16:39:54Z

core-api/src/main/java/com/optimizely/ab/odp/serializer/ODPJsonSerializerFactory.java

+            case GSON_CONFIG_PARSER:
+                jsonSerializer = new GsonSerializer();
+                break;
+            case JACKSON_CONFIG_PARSER:


Here we see another set of repeated multi JSON parsers. Let's refactor them all for sharing later.

Its using the same JsonParserProvider to find out the default one. Its just exporting the implementation class separately. This is separate because it is a seralizer. I wanted to keep parser and serializers separate to avoid confusion. any more parsing or serialization done for ODP will go in to these same classes as added methods. Let me know what you think

Separating parser and serializer looks good. I was talking about the future refactoring discussed already to reuse them in {datafile parser, event serializer, segments parser, and odp event serializer}.

jaeopt · 2022-08-23T16:49:16Z

core-api/src/test/java/com/optimizely/ab/odp/serializer/ODPJsonSerializerFactoryTest.java

+    @Test
+    public void getCorrectSerializerWhenValidDefaultIsProvided() {
+        PropertyUtils.set("default_parser", "JSON_CONFIG_PARSER");
+        assertEquals(JsonSerializer.class, ODPJsonSerializerFactory.getSerializer().getClass());
+    }
+


Let's add other parsers too to avoid wrong mapping.

jaeopt · 2022-08-23T16:51:37Z

core-api/src/test/java/com/optimizely/ab/odp/serializer/ODPJsonSerializerTest.java

+
+        ObjectMapper mapper = new ObjectMapper();
+
+        String expectedResult = "[{\"type\":\"type-1\",\"action\":\"action-1\",\"identifiers\":{\"vuid-1-3\":\"fs-1-3\",\"vuid-1-1\":\"fs-1-1\",\"vuid-1-2\":\"fs-1-2\"},\"data\":{\"source\":\"java-sdk\",\"data-1\":\"data-value-1\"}},{\"type\":\"type-2\",\"action\":\"action-2\",\"identifiers\":{\"vuid-2-3\":\"fs-2-3\",\"vuid-2-2\":\"fs-2-2\",\"vuid-2-1\":\"fs-2-1\"},\"data\":{\"source\":\"java-sdk\",\"data-1\":\"data-value-2\"}},{\"type\":\"type-3\",\"action\":\"action-3\",\"identifiers\":{\"vuid-3-3\":\"fs-3-3\",\"vuid-3-2\":\"fs-3-2\",\"vuid-3-1\":\"fs-3-1\"},\"data\":{\"source\":\"java-sdk\",\"data-1\":\"data-value-3\"}}]";


Can we change "data" samples to <String: Any> instead of <String: String>?

jaeopt · 2022-08-23T17:02:38Z

core-httpclient-impl/src/main/java/com/optimizely/ab/odp/DefaultODPApiManager.java

+            return;
+        }
+
+        if (response.getStatusLine().getStatusCode() >= 400) {


Failures with recoverable errors (status=5xx, connection errors, etc) should be differentiated so that OdpEventManager can retry them later.

Now returning status code from the method so that consuming code can use it and retry when needed.

jaeopt · 2022-08-23T17:15:07Z

core-api/src/main/java/com/optimizely/ab/odp/ODPEvent.java

+    private Map<String, String > identifiers;
+    private Map<String, String> data;
+
+    public ODPEvent(String type, String action, Map<String, String> identifiers, Map<String, String> data) {


Suggested change

public ODPEvent(String type, String action, Map<String, String> identifiers, Map<String, String> data) {

public ODPEvent(String type, String action, Map<String, String> identifiers, Map<String, Object> data) {

It can support any type of data (String, Number, Boolean).

jaeopt · 2022-08-23T17:23:33Z

core-api/src/main/java/com/optimizely/ab/odp/ODPApiManager.java

 public interface ODPApiManager {
    String fetchQualifiedSegments(String apiKey, String apiEndpoint, String userKey, String userValue, List<String> segmentsToCheck);
+
+    void sendEvents(String apiKey, String apiEndpoint, List<ODPEvent> events);
 }


It looks like you plan to open these to public (and Android-SDK) to customize. If so, ODPEvent should be open too, but this is not desired. Let's discuss more.

Any methods that make any external http calls have to be passed in from outside so that we can provide separate implementations for java and android. For java sdk only, the core part is in the core-api and the http implementation has to be in core-httpclient-impl. For Android, we will re use all the ODP code int he core-api but write a new ODPApiManager implementation and pass from outside. This is why it's open. Let me know what you think?

@zashraf1985 I fully understand opening is required to support Android and other clients. My question was about opening "ODPEvent" type. We can consider to accept serialized String or map instead here, that's how we avoid internal type.

I would emphasize on keeping the ODPEvent type exposed here. It's only and Entity class which does not contain any business logic. Adding a string input here will increase the burden of unnecessary serialization on us. Keeping it a list of ODPEvent is easy to use and less error prone. What do you suggest?

@zashraf1985 I understand that it'll look nice to pass the structure, but opening ODPEvent may be expensive. Following our rule, we should rename it to "OptimizelyOdpEvent" like other public type, which also should be documented. Other SDKs also should be changed for consistency. A question is that it's worth to open it for the cost? Clients do not care about the contents, just serialized and dispatched.

I think you are right. The only responsibility of the httpclient should be to take the payload, send it and return status. The serialization should be handled in EventManager. I will change it to get the string payload. The serialization code is still useful for the ODPEventManager so i will leave it there with this PR.

jaeopt

A couple of more suggestions.

jaeopt · 2022-08-25T21:10:03Z

core-api/src/test/java/com/optimizely/ab/odp/serializer/ODPJsonSerializerTest.java

+        data.put("data-num", Integer.parseInt(index));
+        data.put("data-float", 2.33);
+        data.put("data-bool-true", true);
+        data.put("data-bool-false", false);


Let's add to this test, which is also supported.

I did not get this suggestion. Can you elaborate a bit?

"Let's add to this test". I missed the key word in my comment :)

I should not use "<null>", which is removed by github :) I meant to add a type checking for "null" type.

jaeopt · 2022-08-25T21:14:06Z

core-httpclient-impl/src/main/java/com/optimizely/ab/odp/DefaultODPApiManager.java

@@ -206,17 +205,19 @@ public void sendEvents(String apiKey, String apiEndpoint, List<ODPEvent> events)
            response = httpClient.execute(request);
        } catch (IOException e) {
            logger.error("Error retrieving response from event request", e);
-            return;
+            return null;


Should we retry on "null" response? How can we differentiate connection error (retry) from other permanent errors?

…e top level functions.

jaeopt

See my other clarifications in the previous reviews.

jaeopt · 2022-08-26T20:24:57Z

core-httpclient-impl/src/main/java/com/optimizely/ab/odp/DefaultODPApiManager.java

@@ -205,7 +205,7 @@ public Integer sendEvents(String apiKey, String apiEndpoint, List<ODPEvent> even
            response = httpClient.execute(request);
        } catch (IOException e) {
            logger.error("Error retrieving response from event request", e);
-            return null;
+            return 0;


Either null or 0 is fine with me. My question was that how we can tell if this failure should be retried or not at the upper level. Do you plan to retry all status of 0 and 50x?

yes! I will document it on the method too.

null means there was a non recoverable error and no retry is needed.

0 means an unexpected error and you should retry

Any other status can be handled as needed but my idea is to retry on 500+

jaeopt

LGTM

Added serialization support for odp events

4a69dcd

zashraf1985 requested a review from a team as a code owner August 18, 2022 21:07

optimizely-codereview-bot assigned zashraf1985 Aug 18, 2022

zashraf1985 added 3 commits August 18, 2022 14:09

Added missing headers

eedd973

Implemented sendEvents method.

4e2745b

did some refactoring and added unit tests.

a8f11a7

zashraf1985 removed their assignment Aug 19, 2022

optimizely-codereview-bot added 🤖 4 hrs 🤖 8 hrs 🤖 12 hrs 🤖 16 hrs and removed 🤖 4 hrs 🤖 8 hrs 🤖 12 hrs labels Aug 19, 2022

jaeopt requested changes Aug 23, 2022

View reviewed changes

optimizely-codereview-bot removed the 🤖 16 hrs label Aug 23, 2022

optimizely-codereview-bot assigned zashraf1985 Aug 23, 2022

zashraf1985 added 2 commits August 23, 2022 11:14

Incorporated review feedback.

6e3515d

Return statudCode from sendEvents

a86f66f

zashraf1985 removed their assignment Aug 23, 2022

optimizely-codereview-bot requested a review from jaeopt August 23, 2022 18:27

optimizely-codereview-bot added 🤖 4 hrs 🤖 8 hrs 🤖 12 hrs 🤖 16 hrs and removed 🤖 4 hrs 🤖 8 hrs 🤖 12 hrs labels Aug 23, 2022

jaeopt requested changes Aug 25, 2022

View reviewed changes

optimizely-codereview-bot removed the 🤖 16 hrs label Aug 25, 2022

optimizely-codereview-bot assigned zashraf1985 Aug 25, 2022

returning nonnull response so that we can add a check and retry on th…

3323a31

…e top level functions.

zashraf1985 removed their assignment Aug 26, 2022

optimizely-codereview-bot requested a review from jaeopt August 26, 2022 00:54

jaeopt requested changes Aug 26, 2022

View reviewed changes

optimizely-codereview-bot assigned zashraf1985 Aug 26, 2022

Incorporated more review feedback

543c81a

zashraf1985 removed their assignment Aug 26, 2022

optimizely-codereview-bot requested a review from jaeopt August 26, 2022 23:45

jaeopt approved these changes Aug 29, 2022

View reviewed changes

zashraf1985 merged commit 8b8a983 into master Aug 29, 2022

zashraf1985 deleted the zeeshan/ats-rest-api-manager branch August 29, 2022 18:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Added ODP RestApi interface for sending Events to ODP #485

feat: Added ODP RestApi interface for sending Events to ODP #485

zashraf1985 commented Aug 18, 2022

jaeopt left a comment

jaeopt Aug 22, 2022

zashraf1985 Aug 23, 2022

jaeopt Aug 22, 2022

zashraf1985 Aug 23, 2022

jaeopt Aug 22, 2022

zashraf1985 Aug 23, 2022

jaeopt Aug 23, 2022

zashraf1985 Aug 23, 2022

jaeopt Aug 25, 2022

jaeopt Aug 23, 2022

zashraf1985 Aug 23, 2022

jaeopt Aug 23, 2022

zashraf1985 Aug 23, 2022

jaeopt Aug 23, 2022

zashraf1985 Aug 23, 2022

jaeopt Aug 23, 2022

zashraf1985 Aug 23, 2022

jaeopt Aug 23, 2022

zashraf1985 Aug 23, 2022

jaeopt Aug 25, 2022 •

edited

Loading

zashraf1985 Aug 26, 2022

jaeopt Aug 26, 2022

zashraf1985 Aug 26, 2022

jaeopt left a comment

jaeopt Aug 25, 2022

zashraf1985 Aug 26, 2022

jaeopt Aug 26, 2022

jaeopt Aug 26, 2022 •

edited

Loading

jaeopt Aug 25, 2022

jaeopt left a comment

jaeopt Aug 26, 2022

zashraf1985 Aug 26, 2022 •

edited

Loading

jaeopt left a comment

	private Map<String, String> data;
	private Map<String, Object> data;

	public Map<String, String> getData() {
	public Map<String, Object> getData() {

	public void setData(Map<String, String> data) {
	public void setData(Map<String, Object> data) {


		ObjectMapper mapper = new ObjectMapper();

		String expectedResult = "[{\"type\":\"type-1\",\"action\":\"action-1\",\"identifiers\":{\"vuid-1-3\":\"fs-1-3\",\"vuid-1-1\":\"fs-1-1\",\"vuid-1-2\":\"fs-1-2\"},\"data\":{\"source\":\"java-sdk\",\"data-1\":\"data-value-1\"}},{\"type\":\"type-2\",\"action\":\"action-2\",\"identifiers\":{\"vuid-2-3\":\"fs-2-3\",\"vuid-2-2\":\"fs-2-2\",\"vuid-2-1\":\"fs-2-1\"},\"data\":{\"source\":\"java-sdk\",\"data-1\":\"data-value-2\"}},{\"type\":\"type-3\",\"action\":\"action-3\",\"identifiers\":{\"vuid-3-3\":\"fs-3-3\",\"vuid-3-2\":\"fs-3-2\",\"vuid-3-1\":\"fs-3-1\"},\"data\":{\"source\":\"java-sdk\",\"data-1\":\"data-value-3\"}}]";

	public ODPEvent(String type, String action, Map<String, String> identifiers, Map<String, String> data) {
	public ODPEvent(String type, String action, Map<String, String> identifiers, Map<String, Object> data) {

feat: Added ODP RestApi interface for sending Events to ODP #485

feat: Added ODP RestApi interface for sending Events to ODP #485

Conversation

zashraf1985 commented Aug 18, 2022

Summary

Test plan

Jira

jaeopt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeopt Aug 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeopt Aug 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zashraf1985 Aug 26, 2022 • edited Loading

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

jaeopt Aug 25, 2022 •

edited

Loading

jaeopt Aug 26, 2022 •

edited

Loading

zashraf1985 Aug 26, 2022 •

edited

Loading