-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Adding Options for Request Id Policy. #6370
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,25 +3,64 @@ | |
|
|
||
| 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 | ||
| * 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 behaviour. | ||
| */ | ||
| 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. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be |
||
| * | ||
| * @param requestIdPolicyOptions with given {@link RetryPolicyOptions}. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix javadoc - |
||
| */ | ||
| public RequestIdPolicy(RequestIdPolicyOptions requestIdPolicyOptions) { | ||
| this.requestIdPolicyOptions = requestIdPolicyOptions; | ||
| } | ||
|
|
||
|
|
||
| @Override | ||
| public Mono<HttpResponse> process(HttpPipelineCallContext context, HttpPipelineNextPolicy next) { | ||
| String requestId = context.getHttpRequest().getHeaders().getValue(REQUEST_ID_HEADER); | ||
| if (requestId == null) { | ||
| context.getHttpRequest().getHeaders().put(REQUEST_ID_HEADER, UUID.randomUUID().toString()); | ||
| if (!Objects.isNull(requestIdPolicyOptions)) { | ||
| HttpHeaders httpHeaders = requestIdPolicyOptions.getIdHeaderSupplier().get(); | ||
| if (!Objects.isNull(httpHeaders)) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A random header id should be added if |
||
| httpHeaders.stream().forEach(httpHeader -> { | ||
| String requestIdHeaderValue = context.getHttpRequest().getHeaders().getValue(httpHeader.getName()); | ||
| if (requestIdHeaderValue == null) { | ||
| context.getHttpRequest().getHeaders().put(httpHeader.getName(), httpHeader.getValue()); | ||
| } | ||
| }); | ||
| }else { | ||
| if (requestId == null) { | ||
| context.getHttpRequest().getHeaders().put(REQUEST_ID_HEADER, UUID.randomUUID().toString()); | ||
| } | ||
| } | ||
| } else { | ||
| if (requestId == null) { | ||
| context.getHttpRequest().getHeaders().put(REQUEST_ID_HEADER, UUID.randomUUID().toString()); | ||
| } | ||
| } | ||
| return next.process(); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| // 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 { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need a separate type for this? Can the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 final Supplier<HttpHeaders> idHeaderSupplier; | ||
|
|
||
| /** | ||
| * 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) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. name suggestion: |
||
| this.idHeaderSupplier = Objects.requireNonNull(idHeaderSupplier, | ||
| "'idHeaderSupplier' cannot be null."); | ||
| } | ||
|
|
||
| /** | ||
| * Get the provided {@link Supplier} which dynamically generate id for various request id headers. | ||
| * | ||
hemanttanwar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * @return {@link Supplier} to provide dynamically generated id for various request id headers | ||
| */ | ||
| public Supplier<HttpHeaders> getIdHeaderSupplier() { | ||
| return idHeaderSupplier; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.