Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,56 @@

package com.azure.core.http.policy;

import com.azure.core.http.HttpHeaders;
import com.azure.core.http.HttpPipelineCallContext;
import com.azure.core.http.HttpPipelineNextPolicy;
import com.azure.core.http.HttpResponse;
import reactor.core.publisher.Mono;

import java.util.Objects;
import java.util.UUID;

/**
* 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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* the unique identifier for the request.
* @see RequestIdPolicyOptions
*/
public class RequestIdPolicy implements HttpPipelinePolicy {
private static final String REQUEST_ID_HEADER = "x-ms-client-request-id";

private final RequestIdPolicyOptions requestIdPolicyOptions;

/**
* Creates {@link RequestIdPolicy} with default {@link ExponentialBackoff} as {@link RetryStrategy}and use
Comment thread
hemanttanwar marked this conversation as resolved.
Outdated
* 'retry-after-ms' in {@link HttpResponse} header for calculating retry delay.
*/
public RequestIdPolicy() {
this.requestIdPolicyOptions = null;
}

/**
* 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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*
* @param requestIdPolicyOptions with given {@link RetryPolicyOptions}.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix javadoc - RequestIdPolicyOptions

* @throws NullPointerException if {@code RequestIdPolicyOptions} is {@code null}.
*/
public RequestIdPolicy(RequestIdPolicyOptions requestIdPolicyOptions) {
this.requestIdPolicyOptions = Objects.requireNonNull(requestIdPolicyOptions,
Comment thread
hemanttanwar marked this conversation as resolved.
Outdated
"'retryPolicyOptions' cannot be null.");
}


@Override
public Mono<HttpResponse> process(HttpPipelineCallContext context, HttpPipelineNextPolicy next) {
String requestId = context.getHttpRequest().getHeaders().getValue(REQUEST_ID_HEADER);
if (requestId == null) {
if (!Objects.isNull(requestIdPolicyOptions)) {
HttpHeaders httpHeaders = requestIdPolicyOptions.getIdHeaderSupplier().get();
if (!Objects.isNull(httpHeaders)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

httpHeaders.stream().forEach(httpHeader ->
context.getHttpRequest().getHeaders().put(httpHeader.getName(), httpHeader.getValue())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@alzimmermsft alzimmermsft Nov 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hemanttanwar hemanttanwar Nov 18, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
return next.process();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

package com.azure.core.http.policy;

import com.azure.core.annotation.Immutable;
import com.azure.core.http.HttpHeaders;

import java.util.Objects;
import java.util.function.Supplier;

/**
* Immutable Configuration options for {@link RequestIdPolicy}.
* User can provide a {@link Supplier} to generated id for various id headers to be used in
* {@link com.azure.core.http.HttpRequest}. Examples of such headers are 'x-ms-client-request-id
* or 'x-ms-correlation-request-id' etc.
*/
@Immutable
public class RequestIdPolicyOptions {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a separate type for this? Can the Supplier instead be passed to RequestIdPolicy constructor directly?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on this comment, I don't think this policy will require additional configuration outside of being able to set Request ID headers.


private volatile Supplier<HttpHeaders> idHeaderSupplier;
Comment thread
hemanttanwar marked this conversation as resolved.
Outdated

/**
* Creates {@link RequestIdPolicyOptions} with provided {@code idHeaderSupplier }.
*
* @param idHeaderSupplier {@link Supplier} to provide dynamically generated id for various request id headers.
* Examples of such headers are 'x-ms-client-request-id or 'x-ms-correlation-request-id' etc.
*/
public RequestIdPolicyOptions(Supplier<HttpHeaders> idHeaderSupplier) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name suggestion: requestIdSupplier

this.idHeaderSupplier = Objects.requireNonNull(idHeaderSupplier,
"'idHeaderSupplier' cannot be null.");
}

/**
*
Comment thread
hemanttanwar marked this conversation as resolved.
* @return {@link Supplier} to provide dynamically generated id for various request id headers
*/
public Supplier<HttpHeaders> getIdHeaderSupplier() {
return idHeaderSupplier;
}
}