Refactor EdgeHitProcessor.processHit to clean up error handling by kevinlind · Pull Request #19 · adobe/aepsdk-edge-android

kevinlind · 2023-01-10T20:16:22Z

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

codecov · 2023-01-10T20:27:44Z

Codecov Report

Merging #19 (e244810) into dev-v2.0.0 (31fe7e6) will increase coverage by 0.23%.
The diff coverage is 88.52%.

@@               Coverage Diff                @@
##             dev-v2.0.0      #19      +/-   ##
================================================
+ Coverage         83.17%   83.40%   +0.23%     
- Complexity          380      386       +6     
================================================
  Files                29       29              
  Lines              1545     1566      +21     
  Branches            219      221       +2     
================================================
+ Hits               1285     1306      +21     
  Misses              161      161              
  Partials             99       99

Flag	Coverage Δ
functional-tests	`65.84% <60.66%> (-0.31%)`	⬇️
unit-tests	`79.31% <83.61%> (+0.09%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...n/java/com/adobe/marketing/mobile/EdgeRequest.java	`65.00% <57.14%> (-6.43%)`	⬇️
...a/com/adobe/marketing/mobile/EdgeHitProcessor.java	`85.59% <91.11%> (+3.78%)`	⬆️
...e/marketing/mobile/CompletionCallbacksManager.java	`96.88% <100.00%> (+0.45%)`	⬆️
...com/adobe/marketing/mobile/EdgeNetworkService.java	`73.82% <100.00%> (+0.42%)`	⬆️
...adobe/marketing/mobile/NetworkResponseHandler.java	`82.14% <100.00%> (ø)`

code/edge/src/main/java/com/adobe/marketing/mobile/EdgeHitProcessor.java

emdobrin

looks great, and thanks for splitting that method up 👍

code/edge/src/main/java/com/adobe/marketing/mobile/EdgeRequest.java

code/edge/src/main/java/com/adobe/marketing/mobile/EdgeHitProcessor.java

…ent requests

code/edge/src/test/java/com/adobe/marketing/mobile/EdgeNetworkServiceTest.java

code/edge/src/main/java/com/adobe/marketing/mobile/CompletionCallbacksManager.java

code/edge/src/androidTest/java/com/adobe/marketing/mobile/CompletionHandlerFunctionalTests.java

addb · 2023-01-20T21:24:27Z

code/edge/src/test/java/com/adobe/marketing/mobile/EdgeHitProcessorTests.java

+		// Tests that when a good hit is processed that a network request is made and the request returns 200
+
+		// setup
+		Map<String, Object> config = new HashMap<>();


Should we add test cases with null config map?

addb · 2023-01-20T21:31:25Z

code/edge/src/test/java/com/adobe/marketing/mobile/EdgeNetworkServiceTest.java

+		final String responseStr =
+			"<RS>{\"some\":\"thing1\\n\"}<LF>" +
+			"<RS>{\"some\":\"thing2\\n\"}<LF>" +
+			"<RS>{\"some\":\"thing3\\n\"}<LF>."; // note the trailing '.' character


Should we add comment saying "." is dropped with the exception?

addb · 2023-01-20T21:41:50Z

code/edge/src/main/java/com/adobe/marketing/mobile/EdgeNetworkService.java

 		while (scanner.hasNext()) {
 			final String jsonResult = scanner.next();
-			responseCallback.onResponse(jsonResult.substring(trimLength));
+			try {


Is it possible to avoid the exception all together by adding a check like this?

if (jsonResult.length() > trimlength) { responseCallback.onResponse(jsonResult.substring(trimLength)); }

I updated this to check the length instead of adding the try/catch block. If the jsonResult length is less than the record separator length, then I log a debug stating the response is ignored.

emdobrin · 2023-01-20T22:08:38Z

code/edge/src/main/java/com/adobe/marketing/mobile/CompletionCallbacksManager.java

+				StringWriter stringWriter = new StringWriter();
+				PrintWriter printWriter = new PrintWriter(stringWriter);
+				ex.printStackTrace(printWriter);
+
+				Log.warning(
+					LOG_TAG,
+					LOG_SOURCE,
+					"Exception thrown when invoking EdgeCallback.onComplete() for request event id %s: %s",
+					requestEventId,
+					stringWriter.toString()


Reading more about this it looks like it is recommended to use the platform level logging support for exceptions instead of printStackTrace.

@praveek any chance would could support for logging throwable in the Logging service as the platform provides?
https://developer.android.com/reference/android/util/Log#w(java.lang.String,%20java.lang.Throwable)

Alternatively we can look into using https://developer.android.com/reference/android/util/Log#getStackTraceString(java.lang.Throwable)

@emdobrin We can explore about adding these additional log methods after the 2.0 release. For now, I would prefer getStackTraceString to get the string representation of stack trace.

nit: Can we reword the log to Exception thrown when invoking completion callback for request event id
EdgeCallback.onComplete() is not very intuitive when a customer passes lambda.

@kevinlind can you make this update and also create a feature request for the logging service in aepsdk-core-android?

I've updated the log message as suggested. I also created an issue in Android Core to add LoggingService APIs to support throwable objects, adobe/aepsdk-core-android#349

…n processing network response

… configuration is empty

kevinlind · 2023-01-20T18:18:10Z

code/edge/src/main/java/com/adobe/marketing/mobile/CompletionCallbacksManager.java

+				PrintWriter printWriter = new PrintWriter(stringWriter);
+				ex.printStackTrace(printWriter);
+
+				Log.warning(


Log warning will look like the following, where an NPE is thrown from "throwUp" which is called from EdgeCallback.onComplete:

2023-01-20 10:16:15.788 16047-16192/com.adobe.marketing.edgetestapp W/AdobeExperienceSDK: Edge/CompletionCallbacksManager - Exception thrown when invoking EdgeCallback for request event id 00c61fbb-4c43-476e-9fd0-acc20a958b93: java.lang.NullPointerException: Catch me!
at com.adobe.marketing.tester.MainActivity.throwUp(MainActivity.java:239)
at com.adobe.marketing.tester.MainActivity.lambda$onSubmitSingleEvent$1$com-adobe-marketing-tester-MainActivity(MainActivity.java:232)
at com.adobe.marketing.tester.MainActivity$$ExternalSyntheticLambda4.onComplete(Unknown Source:6)
at com.adobe.marketing.mobile.CompletionCallbacksManager.unregisterCallback(CompletionCallbacksManager.java:97)
at com.adobe.marketing.mobile.EdgeHitProcessor$1.onComplete(EdgeHitProcessor.java:142)
at com.adobe.marketing.mobile.EdgeNetworkService.doRequest(EdgeNetworkService.java:230)
at com.adobe.marketing.mobile.EdgeHitProcessor.sendNetworkRequest(EdgeHitProcessor.java:175)
at com.adobe.marketing.mobile.EdgeHitProcessor.processExperienceEventHit(EdgeHitProcessor.java:267)
at com.adobe.marketing.mobile.EdgeHitProcessor.processHit(EdgeHitProcessor.java:97)
at com.adobe.marketing.mobile.services.PersistentHitQueue.lambda$processNextHit$2$com-adobe-marketing-mobile-services-PersistentHitQueue(PersistentHitQueue.java:118)
at com.adobe.marketing.mobile.services.PersistentHitQueue$$ExternalSyntheticLambda0.run(Unknown Source:2)
at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:463)
at java.util.concurrent.FutureTask.run(FutureTask.java:264)
at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:307)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1137)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:637)
at java.lang.Thread.run(Thread.java:1012)

kevinlind · 2023-01-25T03:56:39Z

code/edge/src/main/java/com/adobe/marketing/mobile/EdgeNetworkService.java

 		while (scanner.hasNext()) {
 			final String jsonResult = scanner.next();
-			responseCallback.onResponse(jsonResult.substring(trimLength));
+			try {


I updated this to check the length instead of adding the try/catch block. If the jsonResult length is less than the record separator length, then I log a debug stating the response is ignored.

kevinlind · 2023-01-25T04:40:30Z

code/edge/src/main/java/com/adobe/marketing/mobile/CompletionCallbacksManager.java

+				StringWriter stringWriter = new StringWriter();
+				PrintWriter printWriter = new PrintWriter(stringWriter);
+				ex.printStackTrace(printWriter);
+
+				Log.warning(
+					LOG_TAG,
+					LOG_SOURCE,
+					"Exception thrown when invoking EdgeCallback.onComplete() for request event id %s: %s",
+					requestEventId,
+					stringWriter.toString()


I've updated the log message as suggested. I also created an issue in Android Core to add LoggingService APIs to support throwable objects, adobe/aepsdk-core-android#349

kevinlind added 3 commits January 10, 2023 11:23

Refactor EdgeHitProcessor.processHit to improve error handling

6257af7

use DataReader when reading location hint scope from response

20cea49

Add additional unit tests to cover the added error handling.

b89aa1a

kevinlind requested a review from emdobrin January 10, 2023 20:16

addb reviewed Jan 10, 2023

View reviewed changes

code/edge/src/main/java/com/adobe/marketing/mobile/EdgeHitProcessor.java Show resolved Hide resolved

emdobrin reviewed Jan 11, 2023

View reviewed changes

code/edge/src/main/java/com/adobe/marketing/mobile/EdgeRequest.java Show resolved Hide resolved

code/edge/src/main/java/com/adobe/marketing/mobile/EdgeHitProcessor.java Outdated Show resolved Hide resolved

kevinlind added 3 commits January 11, 2023 15:04

Merge branch 'dev-v2.0.0' into mob-17882

d3ef983

Add event type in log message when config id is not present

f38b955

Catch any exceptions thrown from EdgeCallback when completing send ev…

99ec313

…ent requests

emdobrin approved these changes Jan 12, 2023

View reviewed changes

Consolidate creation of EdgeEndpoint code to helper method

655b753

kevinlind requested a review from emdobrin January 12, 2023 06:11

emdobrin reviewed Jan 12, 2023

View reviewed changes

kevinlind added 4 commits January 12, 2023 09:42

Fix typo in test case comment

c32da61

Merge branch 'dev-v2.0.0' into mob-17882

815c7db

Add stack trace to logs when an exception is caught in EdgeCallback.

e3f9aee

Specify onComplete method name in log warning

b2edeb5

kevinlind requested review from addb and emdobrin January 20, 2023 20:46

addb reviewed Jan 20, 2023

View reviewed changes

emdobrin reviewed Jan 20, 2023

View reviewed changes

kevinlind added 3 commits January 24, 2023 19:31

use android.util.Log.getStackTrackeString to print stack trace to logs

d16f282

Check string length instead of catching IndexOutOfBoundsException whe…

ec5d2da

…n processing network response

Pass null for configuration map to EdgeDataEntity in test cases where…

e244810

… configuration is empty

emdobrin approved these changes Jan 25, 2023

View reviewed changes

kevinlind commented Jan 25, 2023

View reviewed changes

kevinlind merged commit 8f074dc into adobe:dev-v2.0.0 Jan 25, 2023

kevinlind deleted the mob-17882 branch January 25, 2023 17:28

Conversation

kevinlind commented Jan 10, 2023

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

codecov bot commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

emdobrin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addb Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

praveek Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Jan 10, 2023 •

edited

Loading

addb Jan 20, 2023 •

edited

Loading

praveek Jan 24, 2023 •

edited

Loading