Collect and send custom attributes for capture feature #89

satsukies · 2024-07-22T02:32:07Z

I added an implementation that will be used to enhance the capture function provided after the next version.

Users can add attributes freely and their values via CustomAttributes class .
Attributes are divided into two groups. BuildEnvironment or RuntimeExtra.
- DeployGate instance keeps CustomAttriutes of each groups.
The attributes are sent automatically to DeployGate by SDK when the capture is created.
- When SDK received ACTION_COLLECT_DEVICE_STATES event, SDK will write values via content provider.

…tion

sdk/src/main/java/com/deploygate/sdk/DeployGate.java

satsukies · 2024-07-22T02:40:41Z

sdk/src/main/java/com/deploygate/sdk/DeployGate.java

@@ -1390,4 +1401,192 @@ public static String getDistributionUserName() {

        return sInstance.mDistributionUserName;
    }
+
+    public static boolean putBuildEnvironmentValue(String key, String value) {


We need to add comments each public method.

satsukies · 2024-07-22T03:07:43Z

@jmatsu
Could you check if my implementation plan has any problems or concerns before finish my work?
Thank you.

jmatsu

The approach basically looks good. Please check my comments.

sdk/src/main/java/com/deploygate/sdk/DeployGate.java

…ceived

satsukies · 2024-07-22T10:03:26Z

@jmatsu
Thank you for your feedback.
I made some changes after your feedback.

Added CustomAttributes class for keeping custom attributes.
Added to checking key and value following the feature specs.
Added tests.
Added implementation of writing custom attributes to content provider using received URI.

Could you please re-review this PR? Thank you.

jmatsu

The approach looks basically good. Please check my comments.

sdk/src/main/java/com/deploygate/sdk/DeployGate.java

sdk/src/main/java/com/deploygate/service/DeployGateEvent.java

sdk/src/main/java/com/deploygate/sdk/CustomAttributes.java

sample/src/main/java/com/deploygate/sample/App.java

sdk/src/main/java/com/deploygate/sdk/CustomAttributes.java

jmatsu · 2024-07-22T11:50:18Z

@satsukies
I'm sorry for missing another point. Please use https://github.com/DeployGate/deploygate-android-sdk/blob/250ed74e93320d10ae710633c34fc8e85007270a/sdk/src/main/java/com/deploygate/sdk/internal/Logger.java for new logging!

satsukies · 2024-07-23T01:13:18Z

sdkMock/src/test/java/com/deploygate/sdk/CustomAttributesInterfaceTest.java

use the same test located in sdk module to checking provide same methods each modules.

satsukies · 2024-07-23T01:58:23Z

@jmatsu
Thanks for your careful feedback!
Please re-review some fixes and whole codes.

jmatsu

Please be careful for synchronization, sdk-mock interfaces and AIDL operations.

sdk/src/test/java/com/deploygate/sdk/CustomAttributesInterfaceTest.java

jmatsu · 2024-07-23T02:27:11Z

sdk/src/main/java/com/deploygate/sdk/DeployGate.java

+                if (sBuildEnvironment != null && !sBuildEnvironment.isEmpty()) {
+                    cv.put(DeployGateEvent.ATTRIBUTE_KEY_BUILD_ENVIRONMENT, sBuildEnvironment.toJsonString());
+                }


Thank you for modifying these codes and I noticed a new thing. This code can be decomposed to like the following.

// Be not synchornized by `sLock` if (sBuildEnvironment != null) { // These multiple operations are not synchornized by `ConcurrentHashMap` if (sBuildEnvironment.isEmpty()) { doSomething(sBuildEnvironment.toJsonString()) } }

Synchronization issues are here so we need to fix it. And also, isEmpty check can be shrunken cuz it's just a JSON.

String buildEnvironmentJson = getBuildEnvironment().toJsonString(); if (!buildEnvironmentJson.equals("{}") { cv.put(DeployGateEvent.ATTRIBUTE_KEY_BUILD_ENVIRONMENT, buildEnvironmentJson); }

jmatsu · 2024-07-23T02:29:34Z

sdk/src/main/java/com/deploygate/sdk/DeployGate.java

+                cv.put(DeployGateEvent.ATTRIBUTE_KEY_EVENT_AT, System.currentTimeMillis());
+
+                ContentResolver cr = mApplicationContext.getContentResolver();
+                cr.insert(targetUri, cv);


Please introduce try-catch and suppress the error for safer operations.

jmatsu · 2024-07-23T02:30:48Z

sdkMock/src/main/java/com/deploygate/sdk/DeployGate.java

@@ -179,4 +179,12 @@ public static void composeComment(String defaultComment) {
    public static String getDistributionUserName() {
        return null;
    }
+
+    public static CustomAttributes getBuildEnvironment() {
+        return null;


Don't make here null. It's always non-null on sdk so this code means sdk-mock is unreliable.

This comment remind me of you said "must be always non-null for friendly interfaces" in previous review.
I'll fix it.

jmatsu · 2024-07-23T02:31:16Z

sdkMock/src/main/java/com/deploygate/sdk/CustomAttributes.java

+  }
+
+  String toJsonString() {
+    return null;


Don't make here null. It's always non-null on sdk so this code means sdk-mock is unreliable.

jmatsu · 2024-07-23T02:31:43Z

sdk/src/main/java/com/deploygate/sdk/DeployGate.java

@@ -1390,4 +1412,34 @@ public static String getDistributionUserName() {

        return sInstance.mDistributionUserName;
    }
+
+    public static CustomAttributes getBuildEnvironment() {


satsukies · 2024-07-23T12:51:15Z

sdk/src/main/java/com/deploygate/sdk/DeployGate.java

-    private static CustomAttributes sRuntimeExtra = null;
+    private static CustomAttributes sBuildEnvironment;
+    private static CustomAttributes sRuntimeExtra;
+    private static CustomAttributes sSdkRuntimeExtra;


This attrs will be used to collect data for valuable on the debugging.
It is always empty at this point.

satsukies · 2024-07-23T12:58:11Z

sdk/src/main/java/com/deploygate/sdk/CustomAttributes.java

@@ -15,7 +13,7 @@ public final class CustomAttributes {
  private static final Pattern VALID_KEY_PATTERN = Pattern.compile("^[a-z][_a-z0-9]{2,31}$");
  private static final int MAX_VALUE_LENGTH = 64;

-  private final ConcurrentHashMap<String, Object> attributes;
+  final ConcurrentHashMap<String, Object> attributes;


Visibility changed.
we can use attributes from Deploygate class directly, but users cannot use.

satsukies · 2024-07-23T12:59:58Z

sdk/src/main/java/com/deploygate/sdk/DeployGate.java

+                synchronized (getRuntimeExtra().attributes) {
+                    if (!getRuntimeExtra().attributes.isEmpty()) {
+                        for (Map.Entry<String, Object> attr : getRuntimeExtra().attributes.entrySet()) {
+                            String userKey = String.format("%s.%s", mHostApp.packageName, attr.getKey());


We need to append prefix to attribute keys created by user.

Please don't add the extra time-consuming processing. If we append the prefix in CustomAttributes, doesn't it work?

satsukies · 2024-07-23T13:01:19Z

sdk/src/test/java/com/deploygate/sdk/CustomAttributesTest.java

-    } catch (Exception e) {
-      Truth.assertWithMessage("Failed to parse JSON").fail();
-    }
+  public void size() {


add new testcase for checking behavior size() and isEmpty() methods.

satsukies · 2024-07-23T13:07:20Z

@jmatsu
Thanks for your review!
So sorry to request reviews many times, could you please re-review?

jmatsu

Please be careful about the device resources.

jmatsu · 2024-07-24T06:27:36Z

sdkMock/src/test/java/com/deploygate/sdk/CustomAttributesInterfaceTest.java

@@ -0,0 +1 @@
+../../../../../../../sdk/src/test/java/com/deploygate/sdk/CustomAttributesInterfaceTest.java


jmatsu · 2024-07-24T06:28:17Z

sdkMock/src/main/java/com/deploygate/sdk/DeployGate.java

@@ -5,6 +5,8 @@

 public class DeployGate {

+    private static final CustomAttributes sAttributes = new CustomAttributes();


sdk/src/test/java/com/deploygate/sdk/DeployGateInterfaceTest.java

sdk/src/test/java/com/deploygate/sdk/CustomAttributesTest.java

sdk/src/test/java/com/deploygate/sdk/CustomAttributesInterfaceTest.java

sdk/src/main/java/com/deploygate/sdk/DeployGate.java

jmatsu · 2024-07-24T06:53:03Z

sdk/src/main/java/com/deploygate/sdk/DeployGate.java

+                synchronized (getRuntimeExtra().attributes) {
+                    if (!getRuntimeExtra().attributes.isEmpty()) {
+                        for (Map.Entry<String, Object> attr : getRuntimeExtra().attributes.entrySet()) {
+                            String userKey = String.format("%s.%s", mHostApp.packageName, attr.getKey());


Please don't add the extra time-consuming processing. If we append the prefix in CustomAttributes, doesn't it work?

satsukies · 2024-07-25T02:39:57Z

We discussed in-person to plan for how to fix current codes.
Following the discussion, I worked on belows.

Separating params of sending attributes from SDK by 3 groups. BuildEnvironment, RuntimeExtras and SdkDeviceStatus.
Improve implementation of CustomAttributes to be thread-safe.

@jmatsu
Could you please check my codes? Thanks.

jmatsu

Almost LGTM. Could you please add one important testcase?

jmatsu · 2024-07-25T02:56:12Z

sdk/src/test/java/com/deploygate/sdk/DeployGateInterfaceTest.java

+
+    @Test
+    public void getBuildEnvironment() {
+        Truth.assertThat(DeployGate.getBuildEnvironment()).isNotNull();


jmatsu · 2024-07-25T02:56:31Z

sdk/src/test/java/com/deploygate/sdk/CustomAttributesTest.java

+  }
+
+  @Test()
+  public void not_exceed_max_size_multi_thread() {


Awesome 👏

jmatsu · 2024-07-25T02:58:21Z

sdk/src/test/java/com/deploygate/sdk/CustomAttributesTest.java

+import java.util.concurrent.Executors;
+
+@RunWith(AndroidJUnit4.class)
+public class CustomAttributesTest {


@satsukies Could you please add a testcase for getJSONString if it's empty? It must return {}.

Oh sorry.
I added testcase for getJSONString method in 063fc76 .

jmatsu · 2024-07-25T02:59:11Z

sdk/src/main/java/com/deploygate/sdk/CustomAttributes.java

+          Logger.w(TAG, "Attributes already reached max size. Ignored: " + key);
+          return false;
+        }
+      } catch (JSONException e) {


jmatsu

Great. Thank you for the tough work.

satsukies · 2024-07-25T04:01:31Z

Thanks for comment.
I added important testcase for getJSONString method.

satsukies · 2024-07-25T04:05:40Z

Thank you for the tough work.

You too, thank you for your review many times and careful feedback.

satsukies added 4 commits July 22, 2024 10:59

chore(sdk): add constant values for collect custom attributes

80d1bc1

feat(sdk): add interface for set/remove custom attributes from user

6be4756

feat(sdk): manage custom attributes with HashMap and implement valida…

58aa865

…tion

chore(sdk): bumpup version to 4.8.0

fd54751

satsukies self-assigned this Jul 22, 2024

satsukies commented Jul 22, 2024

View reviewed changes

sdk/src/main/java/com/deploygate/sdk/DeployGate.java Show resolved Hide resolved

satsukies commented Jul 22, 2024

View reviewed changes

fix(sdk): add import

e4a3d40

jmatsu reviewed Jul 22, 2024

View reviewed changes

sdk/src/main/java/com/deploygate/sdk/DeployGate.java Outdated Show resolved Hide resolved

sdk/src/main/java/com/deploygate/sdk/DeployGate.java Outdated Show resolved Hide resolved

sdk/src/main/java/com/deploygate/sdk/DeployGate.java Show resolved Hide resolved

satsukies added 7 commits July 22, 2024 15:33

fix(sdk): fix test failed

ce47071

test(sdk): add test for CustomAttributes

b94250d

fix(sdk): add CustomAttributes for capture feature

d8bcc73

test(sdk): add CustomAttributesTest#toJsonString()

c5c8f3c

feat(sdk): write custom attributes when collect device state event re…

9be4d92

…ceived

chore(sample): update sample code set custom attributes

480de52

fix(sdk): fix build failed due to missing some methods in sdkMock

6b56334

satsukies changed the title ~~WIP: Collect custom attributes for capture feature~~ Collect and send custom attributes for capture feature Jul 22, 2024

satsukies marked this pull request as ready for review July 22, 2024 09:54

satsukies requested a review from jmatsu July 22, 2024 09:54

jmatsu suggested changes Jul 22, 2024

View reviewed changes

satsukies added 7 commits July 23, 2024 00:56

fix(sdk): don't send values when it is null

11d51ab

fix(sdk): don't send package name from SDK

2b8ecee

fix(sdk): provide getter only

95dad51

fix(sdk): fix too general naming of some constant values

4889e37

fix(sdk): class became final and hide constructor from outside

823463e

fix(sdk): simplify redundant condition

71e158f

fix(sdk): improve put operation for thread safety

9bcb4f1

satsukies commented Jul 23, 2024

View reviewed changes

test(sdk): fix symbolic link

c829658

satsukies requested a review from jmatsu July 23, 2024 01:58

jmatsu suggested changes Jul 23, 2024

View reviewed changes

satsukies added 6 commits July 23, 2024 15:40

test(sdk): fix testcase method naming

1c5407d

fix(sdk): all public method always return non-null value

45ac031

test(sdk): add testcase checking behavior of isEmpty() and size()

45084e1

fix(sdk): use try-catch insert value via content resolver

4519aa2

fix(sdk): append prefix to keys when call toJsonString()

024b6cc

fix(sdk): use synchronized when access CustomAttributes

1627bff

satsukies commented Jul 23, 2024

View reviewed changes

satsukies requested a review from jmatsu July 23, 2024 13:07

jmatsu suggested changes Jul 24, 2024

View reviewed changes

satsukies added 4 commits July 24, 2024 19:00

test(sdk): use different values, check return type

6f0ebd8

fix(sdk): fix CustomAttributes class become thread-safe

7a510d6

fix(sdk): send attributes collect by SDK

7391f7f

chore(sdk): documentation

110a00e

jmatsu suggested changes Jul 25, 2024

View reviewed changes

test(sdk): add testcase of toJSONString()

063fc76

jmatsu approved these changes Jul 25, 2024

View reviewed changes

satsukies merged commit 0bf65d8 into master Jul 25, 2024
6 checks passed

satsukies deleted the feat/collect-device-stats branch July 25, 2024 04:05

		@@ -0,0 +1 @@
		../../../../../../../sdk/src/test/java/com/deploygate/sdk/CustomAttributesInterfaceTest.java

		@@ -5,6 +5,8 @@

		public class DeployGate {

		private static final CustomAttributes sAttributes = new CustomAttributes();

Collect and send custom attributes for capture feature #89

Collect and send custom attributes for capture feature #89

Conversation

satsukies commented Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

satsukies commented Jul 22, 2024

jmatsu left a comment

Choose a reason for hiding this comment

satsukies commented Jul 22, 2024 • edited Loading

jmatsu left a comment

Choose a reason for hiding this comment

jmatsu commented Jul 22, 2024

Choose a reason for hiding this comment

satsukies commented Jul 23, 2024

jmatsu 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

satsukies commented Jul 23, 2024

jmatsu 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

satsukies commented Jul 25, 2024

jmatsu 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

jmatsu left a comment

Choose a reason for hiding this comment

satsukies commented Jul 25, 2024

satsukies commented Jul 25, 2024

satsukies commented Jul 22, 2024 •

edited

Loading

satsukies commented Jul 22, 2024 •

edited

Loading