Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update RequestIdPolicy.java #6214

Closed
wants to merge 1 commit into from
+7 −2
Closed

Update RequestIdPolicy.java #6214

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 7 additions & 2 deletions sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RequestIdPolicy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,18 @@
* the unique identifier for the request.
*/
public class RequestIdPolicy implements HttpPipelinePolicy {
private static final String REQUEST_ID_HEADER = "x-ms-client-request-id";
private static final String REQUEST_ID_HEADER = "client-request-id";
private static final String LEGACY_REQUEST_ID_HEADER = "x-ms-client-request-id";

@Override
public Mono<HttpResponse> process(HttpPipelineCallContext context, HttpPipelineNextPolicy next) {
String requestId = context.getHttpRequest().getHeaders().getValue(REQUEST_ID_HEADER);
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible that the request-id is set only in the legacy header? Do we need to check both here?

String requestId = context.getHttpRequest().getHeaders().getValue(REQUEST_ID_HEADER);	
String legacyRequestId = context.getHttpRequest().getHeaders().getValue(LEGACY_REQUEST_ID_HEADER);	

if (requestId == null && legacyRequestId == null) {
 ... generate random id
}

Also, if only one is set, we should add logic to set the other as well.

Copy link
Author

Choose a reason for hiding this comment

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

Based on the docs (https://docs.microsoft.com/en-us/rest/api/searchservice/common-http-request-and-response-headers-used-in-azure-search) and Swagger spec, Search uses client-request-id and return-client-request-id.

So it's not clear why to check the status of the legacy header. Instead, it should be always equal to the valid header header, regardless of the value it contains.

The options are:

  • requestId is null, and legacyRequestId is null, in which case both are instantiated with same value.
  • when both requestId and legacyRequestId have the same value, it's also not a problem.

So the problem arises where the values are not equal, in which case a decision needs to be made for which of legacyRequestId or requestId is to taken and replace the other.
This feels to me like a product question rather than a code question, because there may other considerations.

Copy link
Member

Choose a reason for hiding this comment

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

Since the changes are in azure-core, several other Azure services will be using this outside of Search. I am not sure if they all follow the same guidelines as Search.

Copy link
Member

@alzimmermsft alzimmermsft Nov 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Not related to the changes being made in this PR, but a general question around the policy, could we add in another constructor which allows for a Supplier<String> to be passed which will be used in-place of UUID.randomUUID() if set.

@JonathanGiles @mssfang

if (requestId == null) {
context.getHttpRequest().getHeaders().put(REQUEST_ID_HEADER, UUID.randomUUID().toString());
String randomUUID = UUID.randomUUID().toString();
context.getHttpRequest().getHeaders().put(REQUEST_ID_HEADER, randomUUID);

//also set the legacy header for backwards compatibility.
context.getHttpRequest().getHeaders().put(LEGACY_REQUEST_ID_HEADER, randomUUID);
}
return next.process();
}
Expand Down