Append User-Agent for Waiters operations #2039

Quanzzzz · 2020-09-16T21:37:25Z

Currently, there is no signs for Waiters in the User-Agent header, and to track the usage of waiters, we should append a specific user agent for waiters in the header.

Description

The codegen module is modified, adding the method appending userAgent for waiters in all the operations related to the waiters.

Testing

The generated dynamodb client is tested with the waiters. There are two WireMock tests verifying if the requests triggered by the waiters include the WAITERS user agent in them.

Screenshots (if appropriate)

Types of changes

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

License

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

millems · 2020-09-18T19:28:04Z

.../src/test/resources/software/amazon/awssdk/codegen/poet/waiters/query-sync-waiter-class.java

@@ -84,6 +89,15 @@ public void close() {
        return new DefaultBuilder();
    }

+    private <T extends QueryRequest> T applyWaitersUserAgent(T request) {
+        Consumer<AwsRequestOverrideConfiguration.Builder> userAgentApplier = b -> b.addApiName(ApiName.builder()
+                                                                                                      .version(VersionInfo.SDK_VERSION).name("WAITERS").build());


In metric publisher we do

ApiName.builder().name("hll").version("cw-mp").build();

With the mapper we do

ApiName.builder().version("ddb-enh").name("hll").build();

Waiters is kind of a high-level library. Should we do

ApiName.builder().name("hll").version("waiter").build();

?

Yea sure, I can change that. I was imitating the Paginator user agent so it looked like this.

millems · 2020-09-18T19:29:40Z

...es/dynamodb/src/test/java/software/amazon/awssdk/services/dynamodb/WaitersUserAgentTest.java

+        dynamoDbClient = DynamoDbClient.builder()
+                                       .credentialsProvider(StaticCredentialsProvider.create(AwsBasicCredentials.create("test",
+                                                                                                                        "test")))
+                                       .region(Region.US_WEST_2)
+                                       .overrideConfiguration(c -> c.addExecutionInterceptor(interceptor))
+                                       .build();
+
+        dynamoDbAsyncClient = DynamoDbAsyncClient.builder()
+                                                 .credentialsProvider(StaticCredentialsProvider.create(AwsBasicCredentials
+                                                                                                           .create("test",
+                                                                                                                   "test")))
+                                                 .region(Region.US_WEST_2)
+                                                 .overrideConfiguration(c -> c.addExecutionInterceptor(interceptor))
+                                                 .build();


Is this an integration test now?

No, these are just the clients with the interceptor. The integration tests are not here.

It looks like these are calling the real DynamoDB, though, not the wiremock server.

Oh I got what you mean, yea I can add the wireMock server.

millems · 2020-09-21T16:50:34Z

...es/dynamodb/src/test/java/software/amazon/awssdk/services/dynamodb/WaitersUserAgentTest.java

+    @Test
+    public void syncWaiters_shouldHaveWaitersUserAgent() {
+        stubFor(any(urlEqualTo("/")).willReturn(aResponse().withStatus(500)));
+
+        DynamoDbWaiter waiter = dynamoDbClient.waiter();
+        assertThatThrownBy(() -> waiter.waitUntilTableExists(DescribeTableRequest.builder().tableName("table").build())).isNotNull();
+
+        ArgumentCaptor<Context.BeforeTransmission> context = ArgumentCaptor.forClass(Context.BeforeTransmission.class);
+        Mockito.verify(interceptor, times(9)).beforeTransmission(context.capture(), Matchers.any());
+
+        assertTrue(context.getValue().httpRequest().headers().get("User-Agent").toString().contains("waiter"));
+    }


How long do these tests take to run?

The sync one takes around 7s, while the async one only takes 98ms, because the sync one does wait for the response. For the async one, the CompletableFuture is cancelled once the user agent is checked.

Should we use mock clients instead? codegen-generated-classes-test seems like a good place for this test. https://github.com/aws/aws-sdk-java-v2/tree/waiters-development/test/codegen-generated-classes-test/src/test/java/software/amazon/awssdk/services/waiters

nit, you can just use wiremock's verify method rather than the interceptor

verify(postRequestedFor(anyUrl()).withHeader("User-Agent", contains(...));

You could also throw an exception from the interceptor if you want to cancel the calls altogether and just validate the request.

@zoewangg I just tried testing with the mock clients and Mockito.verify(), and it took around 8 mins to get the response and finish the test, not sure what caused that but it seems the WireMock test is much more efficient.

Quanzzzz · 2020-09-21T21:30:43Z

Test updated with adding an exception in the beforeTransmission() of the interceptor. Current testing time is around 1.5s for sync and 49ms for async.

sonarqubecloud · 2020-09-21T21:55:03Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
1 Code Smell

100.0% Coverage
0.0% Duplication

The version of Java (1.8.0_252) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here

…07ad87ae0 Pull request: release <- staging/b006e8f5-1021-4408-8c65-fe607ad87ae0

Quanzzzz force-pushed the waiters-development branch 2 times, most recently from f78f4f4 to 0d520b0 Compare September 16, 2020 21:46

Quanzzzz changed the title ~~Append userAgent for Waiters operations~~ Append User-Agent for Waiters operations Sep 16, 2020

Quanzzzz requested review from zoewangg and removed request for zoewangg September 16, 2020 22:26

Quanzzzz force-pushed the waiters-development branch 2 times, most recently from e6f2589 to 7567d23 Compare September 17, 2020 02:19

Quanzzzz requested a review from millems September 17, 2020 02:19

Quanzzzz force-pushed the waiters-development branch 3 times, most recently from 64c4737 to 9be7b05 Compare September 18, 2020 16:54

millems reviewed Sep 18, 2020

View reviewed changes

Quanzzzz force-pushed the waiters-development branch 2 times, most recently from f41b9f2 to 1945470 Compare September 19, 2020 01:15

millems reviewed Sep 21, 2020

View reviewed changes

Append userAgent for Waiters operations

cfe4068

Quanzzzz force-pushed the waiters-development branch from b3ec6b6 to cfe4068 Compare September 21, 2020 21:28

millems approved these changes Sep 22, 2020

View reviewed changes

Quanzzzz merged commit 57a47d5 into aws:waiters-development Sep 22, 2020

aws-sdk-java-automation added a commit that referenced this pull request May 23, 2022

Merge pull request #2039 from aws/staging/b006e8f5-1021-4408-8c65-fe6…

2f2acfe

…07ad87ae0 Pull request: release <- staging/b006e8f5-1021-4408-8c65-fe607ad87ae0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Append User-Agent for Waiters operations #2039

Append User-Agent for Waiters operations #2039

Uh oh!

Quanzzzz commented Sep 16, 2020

Uh oh!

millems Sep 18, 2020

Uh oh!

Quanzzzz Sep 18, 2020

Uh oh!

millems Sep 18, 2020

Uh oh!

Quanzzzz Sep 18, 2020

Uh oh!

millems Sep 18, 2020

Uh oh!

Quanzzzz Sep 19, 2020

Uh oh!

millems Sep 21, 2020

Uh oh!

Quanzzzz Sep 21, 2020

Uh oh!

zoewangg Sep 21, 2020

Uh oh!

millems Sep 21, 2020

Uh oh!

Quanzzzz Sep 21, 2020 •

edited

Loading

Uh oh!

Quanzzzz commented Sep 21, 2020

Uh oh!

sonarqubecloud bot commented Sep 21, 2020

Uh oh!

Uh oh!

Append User-Agent for Waiters operations #2039

Append User-Agent for Waiters operations #2039

Uh oh!

Conversation

Quanzzzz commented Sep 16, 2020

Description

Testing

Screenshots (if appropriate)

Types of changes

License

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!

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!

Quanzzzz Sep 21, 2020 • 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!

Quanzzzz commented Sep 21, 2020

Uh oh!

sonarqubecloud bot commented Sep 21, 2020

Uh oh!

Uh oh!

Quanzzzz Sep 21, 2020 •

edited

Loading