Add Progress Listener Invocation methods to Asynchronous and Synchronous code paths #5044

anirudh9391 · 2024-03-26T02:28:31Z

Asynchronous request Path to include Progress Listener invocations to track transactions

Motivation and Context

This is a parity feature gap between 1.x and 2.x

Modifications

Added callbacks :

requestPrepared
requestBytesSent
responseHeaderReceived
responseBytesReceived

in asynchronous code path.

Testing

Unit Tests

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
[ x] New feature (non-breaking change which adds functionality)

Checklist

[x ] I have read the CONTRIBUTING document
[x ] Local run of mvn install succeeds
[ x] My code follows the code style of this project
[x ] My change requires a change to the Javadoc documentation
I have updated the Javadoc documentation accordingly
[ x] I have added tests to cover my changes
[x ] All new and existing tests passed
I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

[ x] I confirm that this pull request can be released under the Apache 2 license

…Snapshots

This reverts commit 99febb6.

This reverts commit 276fb70.

This reverts commit 7b24093.

…istener

…ure context objects. Added progress listener method to RequestOverrideConfiguration

…istener

…add tests

…istener

...e/src/main/java/software/amazon/awssdk/core/internal/metrics/BytesSentTrackingPublisher.java

...ftware/amazon/awssdk/core/internal/http/pipeline/stages/PreExecutionUpdateProgressStage.java

.../awssdk/core/internal/http/pipeline/stages/AsyncExecutionFailureExceptionReportingStage.java

...ava/software/amazon/awssdk/core/internal/http/pipeline/stages/MakeAsyncHttpRequestStage.java

.../java/software/amazon/awssdk/core/internal/http/pipeline/stages/ProgressUpdateStageTest.java

...c/test/java/software/amazon/awssdk/core/internal/metrics/BytesReadTrackingPublisherTest.java

…istener

…ad and Download operations in a wya in which BytesReadTrackingPublisher can be reused for upload and download

…istener

…ogress-listener" This reverts commit 2d9cfe7, reversing changes made to fa580e4.

...ava/software/amazon/awssdk/core/internal/http/pipeline/stages/MakeAsyncHttpRequestStage.java

...src/main/java/software/amazon/awssdk/core/internal/util/UploadProgressUpdaterInvocation.java

...src/main/java/software/amazon/awssdk/core/internal/metrics/BytesReadTrackingInputStream.java

...sdk-core/src/main/java/software/amazon/awssdk/core/internal/util/ProgressUpdaterInvoker.java

...ain/java/software/amazon/awssdk/core/internal/progress/listener/LoggingProgressListener.java

zoewangg · 2024-07-19T23:32:42Z

...e/src/main/java/software/amazon/awssdk/core/internal/metrics/BytesReadTrackingPublisher.java

 */
 @SdkInternalApi
-public final class BytesReadTrackingPublisher implements Publisher<ByteBuffer> {
+public final class BytesReadTrackingPublisher implements SdkHttpContentPublisher {


Is there any reason we changed this class to implement SdkHttpContentPublisher?

Yes, this is to enable the ByteReadTrackingPublisher to track the AsyncBody publisher of type SdkHttpContentPublisher :

aws-sdk-java-v2/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/MakeAsyncHttpRequestStage.java

Line 138 in f6f2c25

requestProvider = ProgressListenerUtils.wrapRequestProviderWithByteTrackingIfProgressListenerAttached(requestProvider,

. The same class, ByteReadTrackingPublisher is used to track requests and response

...c/main/java/software/amazon/awssdk/core/internal/util/DownloadProgressUpdaterInvocation.java

...dk-core/src/main/java/software/amazon/awssdk/core/internal/http/RequestExecutionContext.java

zoewangg · 2024-07-19T23:40:21Z

.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/util/ProgressListenerUtils.java

+                ContentStreamProvider.fromInputStream(new BytesReadTrackingInputStream(
+                    AbortableInputStream.create(contentStreamProvider.newStream()),
+                    new AtomicLong(0L),
+                    new UploadProgressUpdaterInvocation(progressUpdater)));


This is a bit hard to read, let's create variables.

Looks like the existing implementation passes RESPONSE_BYTES_READ to BytesReadTrackingInputStream and now we are passing 0L, is this expected?

RESPONSE_BYTES_READ is used to account for "The running count of bytes in the response body that have been read by the client." Here, we are using it for an upload context but the same BytesReadTrackingInputStream is made use of. Hence, the 0.

I think this will break existing code because we will later read context.executionAttributes().getAttribute(SdkInternalExecutionAttribute.RESPONSE_BYTES_READ) to get the counter

…istener

zoewangg · 2024-08-07T16:36:40Z

.../amazon/awssdk/core/internal/http/pipeline/stages/BeforeExecutionProgressReportingStage.java

+        if (ProgressListenerUtils.progressListenerAttached(context.originalRequest())) {
+            Long requestContentLength =
+                (context.requestProvider() != null && context.requestProvider().contentLength().isPresent()) ?
+                context.requestProvider().contentLength().get() : null;


It seems requestProvider() returns AsyncRequestBody which is for async code path, what about sync?

Yes, this is only to track the bytes published in the Async request code path; I still have not found a way to track the sync req. Wil put that out in the next pr

zoewangg · 2024-08-07T16:51:56Z

...re/src/main/java/software/amazon/awssdk/core/internal/progress/listener/ProgressUpdater.java

-
-    public SdkExchangeProgress requestBodyProgress() {
-        return requestBodyProgress;
+@SdkPublicApi


Should be internal API

zoewangg · 2024-08-07T16:54:48Z

...src/main/java/software/amazon/awssdk/core/internal/metrics/BytesReadTrackingInputStream.java

@@ -68,6 +72,10 @@ public int read(byte[] b) throws IOException {
    private void updateBytesRead(long read) {
        if (read > 0) {
            bytesRead.addAndGet(read);
+
+            if (progressUpdaterInvoker != null) {
+                progressUpdaterInvoker.incrementBytesTransferred(bytesRead.get());


Shouldn't it be incrementBytesTransferred(read)?

zoewangg · 2024-08-07T17:06:21Z

...main/java/software/amazon/awssdk/core/internal/progress/listener/DeafultProgressUpdater.java

+ * ProgressUpdater exposes methods that invokes listener methods to update and store request progress state
+ */
+@SdkInternalApi
+public class DeafultProgressUpdater implements ProgressUpdater {


nit: typo, DefaultProgressUpdater

zoewangg · 2024-08-07T17:07:09Z

...rc/main/java/software/amazon/awssdk/core/internal/progress/listener/NoOpProgressUpdater.java

+    public SdkExchangeProgress requestBodyProgress() {
+        return null;
+    }
+
+    public SdkExchangeProgress responseBodyProgress() {
+        return null;
+    }


Unused methods

Removed this

zoewangg · 2024-08-07T17:23:48Z

...c/main/java/software/amazon/awssdk/core/internal/util/ResponseProgressUpdaterInvocation.java

+import software.amazon.awssdk.core.internal.progress.listener.ProgressUpdater;
+
+@SdkInternalApi
+public class ResponseProgressUpdaterInvocation implements ProgressUpdaterInvoker {


ResponseProgressUpdaterInvoker?

Changed, good suggestion

zoewangg · 2024-08-07T17:24:05Z

core/sdk-core/src/main/java/software/amazon/awssdk/core/progress/listener/ProgressListener.java

@@ -189,8 +190,7 @@ default void responseBytesReceived(Context.ResponseBytesReceived context) {
    /**
    * For Expect: 100-continue embedded requests, the service returning anything other than 100 continue
    * indicates a request failure. This method captures the error in the payload
-    * After this, either executionFailure or requestHeaderSent will always be invoked depending on
-    * whether the error type is retryable or not
+    * After this it will either be an executionFailure or a request retry


Unfinished sentence

i meant it as a finishing sentence, modified it to be clearer.

zoewangg · 2024-08-07T17:25:17Z

...c/test/java/software/amazon/awssdk/core/internal/metrics/BytesSentTrackingPublisherTest.java

+public class BytesSentTrackingPublisherTest {
+
+    @Test
+    public void test_updatesBytesSent() {


test_ prefix is really not needed. Let's follow our unit tests naming pattern

zoewangg · 2024-08-07T17:29:21Z

.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/util/ProgressListenerUtils.java

+                ContentStreamProvider.fromInputStream(new BytesReadTrackingInputStream(
+                    AbortableInputStream.create(contentStreamProvider.newStream()),
+                    new AtomicLong(0L),
+                    new UploadProgressUpdaterInvocation(progressUpdater)));


I think this will break existing code because we will later read context.executionAttributes().getAttribute(SdkInternalExecutionAttribute.RESPONSE_BYTES_READ) to get the counter

zoewangg · 2024-08-07T17:31:26Z

.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/util/ProgressListenerUtils.java

+
+    public static SdkHttpContentPublisher wrapRequestProviderWithByteTrackingIfProgressListenerAttached(
+        SdkHttpContentPublisher requestProvider, ProgressUpdater progressUpdater) {
+        return new BytesReadTrackingPublisher(requestProvider, new AtomicLong(0L),


Same here, new AtomicLong(0L), will break existing code path because the SDK relies on SdkInternalExecutionAttribute.RESPONSE_BYTES_READ to contain the counter

hmm.. Im not modifying the context attribute SdkInternalExecutionAttribute.RESPONSE_BYTES_READ here. While wrapping the request provider(while sending the request) with BytesReadTracking publisher im setting bytes read to 0, as we have not yet made the request at that point. This should not modify what we set on the response

sonarcloud · 2024-08-13T18:02:22Z

Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

anirudh9391 added 30 commits December 4, 2023 15:27

Adding interfaces for Progress Listener and state capturing Progress …

588b102

…Snapshots

Modified Indentation

da26d3e

Added new change script

99febb6

Revert "Added new change script"

7e70224

This reverts commit 99febb6.

Added new change script

5e9057e

Moved ProgressSnapshot to its own folder

e607d6c

Added sdkResponse to Progress Snapshot

3d26526

Change totalTransferSize to totalBytes

48a6617

Modify return types of start and elapsedTime to Optional

b096765

Progress Listener Interface definition

b558e6b

Address PR comments on Progress Listener

07f2bfb

Fix checkstyle issues

d2a3253

Fix checkstyle issues

eaf9470

Implement Default Progress Snapshot

276fb70

Revert "Implement Default Progress Snapshot"

7b24093

This reverts commit 276fb70.

Revert "Revert "Implement Default Progress Snapshot""

a943002

This reverts commit 7b24093.

Remove changes to pom

1d56ce2

Merge branch 'feature/anirudkr-progress-listener' into dev/progress-l…

7c1978b

…istener

Add parameterized unit tests

2e66a9a

fix failing tests

3ffbca2

Defined ProgressListener invoker methods, along with success and fail…

2dfdef7

…ure context objects. Added progress listener method to RequestOverrideConfiguration

Merge branch 'feature/anirudkr-progress-listener' into dev/progress-l…

713f6c1

…istener

Address PR comments

ba7b059

Remove SDK Preview API annotations

dbdd27d

Fixed a typo with error logging

29259df

Rectify typo in adding listeners to RequestOverrideConfiguration and …

16e06b7

…add tests

Added Progress Updater Class to invoke listener methods

54caa47

Merge branch 'feature/anirudkr-progress-listener' into dev/progress-l…

ef05f6b

…istener

ProgressUpdater class

631f674

Define ProgressUpdater and LoggingProgressListener

700bd5e

zoewangg reviewed Apr 19, 2024

View reviewed changes

anirudh9391 added 20 commits April 22, 2024 12:09

Merge branch 'feature/anirudkr-progress-listener' into dev/progress-l…

6509782

…istener

Merge branch 'feature/anirudkr-progress-listener' into dev/progress-l…

944a76d

…istener

WIP

d5f1258

Merge branch 'master' into feature/anirudkr-progress-listener

582c724

Merge branch 'feature/anirudkr-progress-listener' into dev/progress-l…

55a9bd8

…istener

Fix PR comments, added an interface and abstract class to denote Uplo…

8756570

…ad and Download operations in a wya in which BytesReadTrackingPublisher can be reused for upload and download

Merge branch 'feature/anirudkr-progress-listener' into dev/progress-l…

06f2c2d

…istener

Remove incorrect modification of test class

a604bd3

Repurposed BytesReadTrackingPublisher

be2f663

Merge branch 'master' into dev/progress-listener

a04b6eb

Added testing for catching attempt failures

dea870b

Added Tests for async exception reporting

e0e5e23

Fixed checkstyle

89b9f0a

Modified method call signatures

13a8461

Added comments to update BytesReadTrackingPublishee class

fa580e4

Merge branch 'feature/anirudkr-progress-listener' into dev/progress-l…

2d9cfe7

…istener

Revert "Merge branch 'feature/anirudkr-progress-listener' into dev/pr…

b40b3e1

…ogress-listener" This reverts commit 2d9cfe7, reversing changes made to fa580e4.

Improve code quality

ab9445d

Progress Listener for sync codepath

3eb5af9

Add response content length

f6f2c25

anirudh9391 changed the title ~~Add Progress Listener Invocation methods to Asynchronous code paths~~ Add Progress Listener Invocation methods to Asynchronous and Synchronous code paths Jul 16, 2024

zoewangg reviewed Jul 19, 2024

View reviewed changes

anirudh9391 added 3 commits July 24, 2024 23:03

Merge branch 'master' into dev/progress-listener

5239d3c

Add a No-Op Progress Listener to avoid null checks

e807dab

Merge branch 'feature/anirudkr-progress-listener' into dev/progress-l…

3d472c1

…istener

zoewangg reviewed Aug 7, 2024

View reviewed changes

Address Pr feedback. Class renaming and method refactoring

bb99253

Pass Response BytesRead as param for wrapping around bytetracking

430f198

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Progress Listener Invocation methods to Asynchronous and Synchronous code paths #5044

Add Progress Listener Invocation methods to Asynchronous and Synchronous code paths #5044

anirudh9391 commented Mar 26, 2024

zoewangg Jul 19, 2024

anirudh9391 Jul 29, 2024

zoewangg Jul 19, 2024

zoewangg Jul 19, 2024

anirudh9391 Jul 29, 2024

zoewangg Aug 7, 2024

zoewangg Aug 7, 2024

anirudh9391 Aug 13, 2024

zoewangg Aug 7, 2024

anirudh9391 Aug 13, 2024

zoewangg Aug 7, 2024

anirudh9391 Aug 13, 2024

zoewangg Aug 7, 2024

anirudh9391 Aug 13, 2024

zoewangg Aug 7, 2024

anirudh9391 Aug 13, 2024

zoewangg Aug 7, 2024

anirudh9391 Aug 13, 2024

zoewangg Aug 7, 2024

anirudh9391 Aug 13, 2024

zoewangg Aug 7, 2024

anirudh9391 Aug 13, 2024

zoewangg Aug 7, 2024

zoewangg Aug 7, 2024

anirudh9391 Aug 13, 2024

sonarcloud bot commented Aug 13, 2024

Add Progress Listener Invocation methods to Asynchronous and Synchronous code paths #5044

Are you sure you want to change the base?

Add Progress Listener Invocation methods to Asynchronous and Synchronous code paths #5044

Conversation

anirudh9391 commented Mar 26, 2024

Motivation and Context

Modifications

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

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

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

sonarcloud bot commented Aug 13, 2024

Quality Gate failed