Update RequestIdPolicy.java #6214
Conversation
Both the new and legacy request headers are supported in order to enable traceability.
| @@ -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); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Can one of the admins verify this patch? |
| @@ -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); | |||
There was a problem hiding this comment.
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.
|
@hemanttanwar I believe you were working on this. Please confirm that your changes support this functionality, and if necessary update this PR with the latest status. |
|
Thanks for the response the changes were great and we can close this PR |
|
This work ws done in another PR #6602 |
Currently
RequestIdPolicyaddsx-ms-client-request-idheader which is a legacy header.The service seems to ignore this header, because it expects the new format header
client-request-id.The proposed change appends the new header in addition to the legacy one in order to maintain a backwards compatibility.
This way tracing will start working.