Add user agent info to ADT client, miscellaneous builder improvements #14237

timtay-microsoft · 2020-08-18T21:45:04Z

User agent looks like: "User-Agent=azsdk-java-azure-digitaltwins-core/1.0.0-beta.1 (1.8.0_201; Windows 10; 10.0)" when run from a windows 10 machine.

See inline below for my notes on the other small changes in this PR

User agent looks like: "User-Agent=azsdk-java-azure-digitaltwins-core/1.0.0-beta.1 (1.8.0_201; Windows 10; 10.0)" when run from a windows 10 machine

timtay-microsoft · 2020-08-18T21:46:15Z

...ns/azure-digitaltwins-core/src/main/java/com/azure/digitaltwins/core/DigitalTwinsClient.java

@@ -53,7 +54,7 @@ public DigitalTwinsServiceVersion getServiceVersion() {
    // this annotation lets users know this method makes a call to the service and whether it returns a single resource or a collection of resources
    @ServiceMethod(returns = ReturnType.SINGLE)
    // TODO This is just a temporary implementation for test purposes. This should be spruced up/replaced once this API is actually designed
-    public DigitalTwinsGetByIdResponse getDigitalTwin(String digitalTwinId) {
+    public Response<Object> getDigitalTwin(String digitalTwinId) {


This works in the sync client, but not in the async client. We'll still need the java autorest team to fix their bug so that the protocol layer actually returns the expected type

I may have missed it but what and why does it not work for async?

timtay-microsoft · 2020-08-18T21:47:43Z

...e-digitaltwins-core/src/main/java/com/azure/digitaltwins/core/DigitalTwinsClientBuilder.java

+    // of the header's value. These null values are equivalent to just constructing "new RetryPolicy()"
+    private static final String retryAfterHeader = null;
+    private static final ChronoUnit retryAfterTimeUnit = null;
+    private static final RetryPolicy DEFAULT_RETRY_POLICY = new RetryPolicy(retryAfterHeader, retryAfterTimeUnit);


Adding these explicitly even though they are default values so that if/when we add support for retry-after headers, it is easy to figure out that the Azure Core RetryPolicy class handles that for us as long as we tell it what our service sends as the header name and time unit

Doesn't RetryPolicy have null check for these values? Does it inform the calling application that it is using default values since null was supplied, or does it silently make the switch?

timtay-microsoft · 2020-08-18T21:48:06Z

...e-digitaltwins-core/src/main/java/com/azure/digitaltwins/core/DigitalTwinsClientBuilder.java

+
+    private Configuration configuration;
+
+    public DigitalTwinsClientBuilder()


Adding a default constructor so that we don't forget it exists, and also to assign some default values

timtay-microsoft · 2020-08-18T21:48:45Z

...e-digitaltwins-core/src/main/java/com/azure/digitaltwins/core/DigitalTwinsClientBuilder.java

        // Closest to API goes first, closest to wire goes last.
        List<HttpPipelinePolicy> policies = new ArrayList<>();

+        String clientName = properties.getOrDefault(SDK_NAME, "UnknownName");
+        String clientVersion = properties.getOrDefault(SDK_VERSION, "UnknownVersion");


These values come from azure-digital-twins.properties file that is added in this PR

timtay-microsoft · 2020-08-18T21:50:20Z

...e-digitaltwins-core/src/main/java/com/azure/digitaltwins/core/DigitalTwinsClientBuilder.java

+        if (buildConfiguration == null)
+        {
+            buildConfiguration = Configuration.getGlobalConfiguration().clone();
+        }


In the configurations client code, their builder never overwrites the values of the instance variables in the builder. Instead it creates local copies that can be altered instead

Do we know why? Is this more efficient, or is it the defined process?

timtay-microsoft · 2020-08-18T21:51:15Z

sdk/digitaltwins/azure-digitaltwins-core/src/main/resources/azure-digital-twins.properties

@@ -0,0 +1,2 @@
+name=${project.artifactId}
+version=${project.version}


"azure-digitaltwins-core" and "1.0.0-beta.1" as defined in the pom file right now

timtay-microsoft · 2020-08-18T21:54:55Z

...e-digitaltwins-core/src/main/java/com/azure/digitaltwins/core/DigitalTwinsClientBuilder.java

+
+    private final Map<String, String> properties;
+
+    private Configuration configuration;


Used for user agent string, and other functionality. See here

vinagesh · 2020-08-19T17:51:05Z

...e-digitaltwins-core/src/main/java/com/azure/digitaltwins/core/DigitalTwinsClientBuilder.java

+    // This is the name of the properties file in this repo that contains the default properties
+    private static final String DIGITAL_TWINS_PROPERTIES = "azure-digital-twins.properties";
+
+    // These are the keys to the above properties file that define the sdk's name and version for use in the user agent string


Call it 'client library'. I have heard that feedback from the SDK architects.

vinagesh · 2020-08-19T17:55:17Z

...e-digitaltwins-core/src/main/java/com/azure/digitaltwins/core/DigitalTwinsClientBuilder.java

        }

-        return new DigitalTwinsAsyncClient(this.httpPipeline, this.serviceVersion, this.endpoint);
+        return new DigitalTwinsAsyncClient(this.httpPipeline, serviceVersion, this.endpoint);


I'm not a java expert but why was 'this' removed?

In this case, removing the "this." means that I wanted to use the locally defined variable here instead of the instance variable. I've seen this pattern in the builders in track 2 where you don't assign the default value to the instance variables when building the pipeline. Instead, you create a local variable and assign its value to the default if the user never set the instance variable's value

abhipsaMisra · 2020-08-19T18:20:14Z

...e-digitaltwins-core/src/main/java/com/azure/digitaltwins/core/DigitalTwinsClientBuilder.java

        // Adds a "x-ms-client-request-id" header to each request. This header is useful for tracing requests through Azure ecosystems
        policies.add(new RequestIdPolicy());

-        // Only the RequestIdPolicy will take effect prior to the retry policy
+        // Only the RequestIdPolicy  and UserAgentPolicy will take effect prior to the retry policy since neither of those need
+        // to change in any way upon retry


abhipsaMisra · 2020-08-19T18:26:41Z

...e-digitaltwins-core/src/main/java/com/azure/digitaltwins/core/DigitalTwinsClientBuilder.java

-        // Custom policies, authentication policy, and add date policy all take place after the retry policy
+        // Custom policies, authentication policy, and add date policy all take place after the retry policy which means
+        // they will be applied once per retry. For instance, the AddDatePolicy will add a different date time header
+        // each time the retry is attempted.


Q - do we know if AddDatePolicy updates the date per retry attempt, or it updates the date for each http request made?

Since that policy is applied after the retry policy, each retry message will have a date time header that fits when the retry attempt occurred

I am not sure if I understand this correctly:

A client is created, date policy is set.

Make http request (1) with this client -> current date is added to the request - request is a success.

Make http request (2) with this client -> current date is added to the request - request fails -> retry kicks in -> new http request (3) is made.

Since the current date is sent with request (1), (2) and (3), how does retry matter here?
Or am I missing something?

For auth, I can understand, that the next time retrt happens, you also want the auth credentials to be refreshed; but I cannot understand why date policy is relevant with retry.

We discussed this offline, and I'm putting up another PR to address my confusing wording of this #14283

Add user agent info to ADT client, miscellaneous builder improvements

58b15d0

User agent looks like: "User-Agent=azsdk-java-azure-digitaltwins-core/1.0.0-beta.1 (1.8.0_201; Windows 10; 10.0)" when run from a windows 10 machine

timtay-microsoft added the Digital Twins label Aug 18, 2020

timtay-microsoft requested review from abhipsaMisra, azabbasi, barustum, bikamani, drwill-ms, prmathur-microsoft and vinagesh as code owners August 18, 2020 21:45

timtay-microsoft commented Aug 18, 2020

View reviewed changes

squash

d6b5431

timtay-microsoft commented Aug 18, 2020

View reviewed changes

timtay-microsoft added 2 commits August 18, 2020 15:20

squash

19271b6

squash

6af2a33

vinagesh reviewed Aug 19, 2020

View reviewed changes

vinagesh approved these changes Aug 19, 2020

View reviewed changes

squash

aefdb18

timtay-microsoft merged commit 63b2889 into master Aug 19, 2020

timtay-microsoft deleted the feature/adt/timtay/userAgent branch August 19, 2020 18:19

abhipsaMisra reviewed Aug 19, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add user agent info to ADT client, miscellaneous builder improvements #14237

Add user agent info to ADT client, miscellaneous builder improvements #14237

timtay-microsoft commented Aug 18, 2020 •

edited

Loading

timtay-microsoft Aug 18, 2020

bikamani Aug 19, 2020

timtay-microsoft Aug 18, 2020

abhipsaMisra Aug 19, 2020

timtay-microsoft Aug 18, 2020

timtay-microsoft Aug 18, 2020

timtay-microsoft Aug 18, 2020

abhipsaMisra Aug 19, 2020

timtay-microsoft Aug 18, 2020

timtay-microsoft Aug 18, 2020

vinagesh Aug 19, 2020

vinagesh Aug 19, 2020

timtay-microsoft Aug 19, 2020

abhipsaMisra Aug 19, 2020

abhipsaMisra Aug 19, 2020

timtay-microsoft Aug 19, 2020

abhipsaMisra Aug 19, 2020

timtay-microsoft Aug 19, 2020


		private Configuration configuration;

		public DigitalTwinsClientBuilder()

		@@ -0,0 +1,2 @@
		name=${project.artifactId}
		version=${project.version}


		private final Map<String, String> properties;

		private Configuration configuration;

Add user agent info to ADT client, miscellaneous builder improvements #14237

Add user agent info to ADT client, miscellaneous builder improvements #14237

Conversation

timtay-microsoft commented Aug 18, 2020 • 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

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

timtay-microsoft commented Aug 18, 2020 •

edited

Loading