Adding Options for Request Id Policy. #6370

hemanttanwar · 2019-11-15T21:03:28Z

Allow customers set the value in RequestId header with following headers names

"x-ms-client-request-id"
"x-ms-correlation-request-id"
"client-request-id"

Should use something like RequestIDOptions POJO to collect information from user. Reference PR https://github.com/Azure/azure-sdk-for-java/pull/6196/files

Coordinate with PR, since it is waiting for this change. : #6214
Related Issue : #6171

Additional context

This change is inline with overall strategy discussed here #6217

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RequestIdPolicy.java

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RequestIdPolicyOptions.java

srnagar · 2019-11-16T09:33:07Z

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RequestIdPolicy.java

-        if (requestId == null) {
+        if (!Objects.isNull(requestIdPolicyOptions)) {
+            HttpHeaders httpHeaders =  requestIdPolicyOptions.getIdHeaderSupplier().get();
+            if (!Objects.isNull(httpHeaders)) {


A random header id should be added if httpHeaders is null.

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RequestIdPolicyOptions.java

srnagar · 2019-11-16T09:42:18Z

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RequestIdPolicy.java


 /**
- * The pipeline policy that puts a UUID in the request header. Azure uses the request id as
+ * The pipeline policy that puts a UUID or user provided id in the request header. Azure uses the request id as


With this change, we can't guarantee that the request-id is a unique identifier. User-provided request ids may not be unique.

While it's true, it may also be possible that a customer generates their own UUID and may want to correlate it with the ID generated by their system.
I have no idea how the service implements it, but as a user I would expect the service to save the provided ID with the rest of the raw data I sent, so I can analyze it later. Therefore the SDK cannot guarantee uniqueness if user provides the ID, which is fine, if the persistence layer can support it.
I want to suggest that the code will generate a default ID, and allow the customer to override it based on their need.

srnagar · 2019-11-16T09:43:41Z

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RequestIdPolicy.java

+     * Creates a {@link RequestIdPolicy} with the provided {@link RequestIdPolicyOptions}.
+     * User can specify {@link java.util.function.Function} to dynamically generated id for various id headers.
+     *
+     * @param requestIdPolicyOptions with given {@link RetryPolicyOptions}.


Fix javadoc - RequestIdPolicyOptions

srnagar · 2019-11-16T09:44:36Z

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RequestIdPolicy.java

+
+    /**
+     * Creates a {@link RequestIdPolicy} with the provided {@link RequestIdPolicyOptions}.
+     * User can specify {@link java.util.function.Function} to dynamically generated id for various id headers.


This should be RequestIdPolicyOptions instead of Function. Also, import packages and use class names instead of fully qualified names.

srnagar · 2019-11-16T09:47:54Z

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RequestIdPolicy.java

+                );
+            }
+        } else {
            context.getHttpRequest().getHeaders().put(REQUEST_ID_HEADER, UUID.randomUUID().toString());


This will now overwrite the requestId that was already in context. Existing requestId should be preserved. Random id should be used only when the requestId in context is null.

srnagar · 2019-11-16T09:51:26Z

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RequestIdPolicy.java

+            HttpHeaders httpHeaders =  requestIdPolicyOptions.getIdHeaderSupplier().get();
+            if (!Objects.isNull(httpHeaders)) {
+                httpHeaders.stream().forEach(httpHeader ->
+                    context.getHttpRequest().getHeaders().put(httpHeader.getName(), httpHeader.getValue())


This should check for existing headers with the same name in the context. If it already exists, it should not be overwritten.

I think this is a good place to have a discussion, can take it offline if needed.

@JonathanGiles is there any guidelines around whether a pipeline policy should overwrite or only add if not exists?

This isn't our only policy that could potentially mutate the headers state.

I am also unsure about guideline on this. I did hear that every new request ( including retry ) to Azure service should use new Request ID. But at the same time it is "optional" in Azure Docs
https://docs.microsoft.com/en-us/rest/api/azure/devops/symbol/requests/create%20requests?view=azure-devops-rest-5.1

I just spoke to Jeffery on this and his comments are "Every retry should have the same ID. This way, we can tell how many retries occurred for a single logical operation."

alzimmermsft

Same questions as @srnagar

itye-msft · 2019-11-21T09:42:31Z

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RequestIdPolicy.java


 /**
- * The pipeline policy that puts a UUID in the request header. Azure uses the request id as
+ * The pipeline policy that puts a UUID or user provided id in the request header. Azure uses the request id as


While it's true, it may also be possible that a customer generates their own UUID and may want to correlate it with the ID generated by their system.
I have no idea how the service implements it, but as a user I would expect the service to save the provided ID with the rest of the raw data I sent, so I can analyze it later. Therefore the SDK cannot guarantee uniqueness if user provides the ID, which is fine, if the persistence layer can support it.
I want to suggest that the code will generate a default ID, and allow the customer to override it based on their need.

hemanttanwar · 2020-01-27T19:29:59Z

Closing this PR this work was done in a separate PR by adding a constructor public RequestIdPolicy(String requestIdHeaderName) . So closing this PR.

Adding Options for Request Id Policy.

c0b1fdf

hemanttanwar requested review from alzimmermsft, jianghaolu and srnagar as code owners November 15, 2019 21:03

hemanttanwar requested a review from mssfang November 15, 2019 21:03

srnagar reviewed Nov 16, 2019

View reviewed changes

alzimmermsft requested changes Nov 18, 2019

View reviewed changes

incorporating review comments

217b64f

itye-msft approved these changes Nov 21, 2019

View reviewed changes

hemanttanwar closed this Jan 27, 2020

Adding Options for Request Id Policy. #6370

Adding Options for Request Id Policy. #6370

Uh oh!

Conversation

hemanttanwar commented Nov 15, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

alzimmermsft Nov 18, 2019 • 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!

hemanttanwar Nov 18, 2019 • 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!

alzimmermsft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hemanttanwar commented Jan 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alzimmermsft Nov 18, 2019 •

edited

Loading

hemanttanwar Nov 18, 2019 •

edited

Loading