[feature] add a new http pipeline policy for special header `client-request-id` and `return-client-request-id`

[chunyu3]

Batch service will have special header client-request-id and return-client-request-id set in HTTP request for diagnostic instead of x-ms-client-request-id and x-ms-return-client-request-id which are used in other Azure Service for diagnostic.

Current in .Net Azure.Core pipeline, the ClientRequestIdPolicy is added into pipeline by default, so each http request has x-ms-client-request-id and x-ms-return-client-request-id see following: https://github.com/Azure/azure-sdk-for-net/blob/592196f5c96a5bfdb7ab29fd28fec2db6edffdcd/sdk/core/Azure.Core/src/Pipeline/HttpPipelineBuilder.cs#L148-L149

policies.Add(ClientRequestIdPolicy.Shared); 

Expected behavior: in each http request message, there are client-request-id and return-client-request-id but there are no x-ms-client-request-id and x-ms-return-client-request-id.

So there are two possible approach:

approach 1

update existing ClientRequestIdPolicy and let it accept a parameter to customize the head name, and in HttpPipelineBuilder will update the head name when add it.

approach 2

To recognize those two headers,

• SDK will instrument a policy similar as ClientRequestIdPolicy into HTTP pipeline, and pipeline will append those two headers for each http request.

• And HttpPipelineBuilder will not add ClientRequestIdPolicy by default.

Current Azure.Core does not have such policy. Can we add this new policy in Azure.Core and export it either mark it public or define it in Azure.Core.Shared?

expected new policy:

namespace Azure.Core.Pipeline
{
    internal class AzureClientRequestIdPolicy : HttpPipelineSynchronousPolicy
    {
        internal const string ClientRequestIdHeader = "client-request-id";
        internal const string EchoClientRequestId = "return-client-request-id";

        protected AzureClientRequestIdPolicy()
        {
        }

        public static AzureClientRequestIdPolicy Shared { get; } = new AzureClientRequestIdPolicy();

        public override void OnSendingRequest(HttpMessage message)
        {
            message.Request.Headers.SetValue(ClientRequestIdHeader, message.Request.ClientRequestId);
            message.Request.Headers.SetValue(EchoClientRequestId, "true");
        }
    }
}

[chunyu3]

Current existing similar policy ClientRequestIdPolicy https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/core/Azure.Core/src/Pipeline/Internal/ClientRequestIdPolicy.cs

But it is internal and the header name is not configurable. one possible solution is to update existing policy: make it header name configurable and move it to Azure.Core.Shared if we don't change it to public.

[jsquire]

@chunyu3 : Can you please help us understand why this should be in Core rather than specific to Batch? Is this something that we believe is a pattern that we'll see across Azure services? Do we have other examples of it in use currently?

[jsquire]

//cc: @annelo-msft

[chunyu3]

@chunyu3 : Can you please help us understand why this should be in Core rather than specific to Batch? Is this something that we believe is a pattern that we'll see across Azure services? Do we have other examples of it in use currently?

Hello @jsquire Current only Batch asked for this. It is not required for all Azure Services. And AFAS I know, the number of SDKs which need those specific headers is not high. The solution to support this is to add a policy to the pipeline when create the service client if the service need those special headers. And this solution is applied for all language SDKs. Now what we need is a pipeline policy, if we add this in Azure.Core shared, every SDK who need those specific headers( batch or coming SDKs in future), we can apply the policy immediately. If not, codegen need to automatically generate this policy.

Azure Core team, would you please think about this and decide if it is accepted to add this policy in Azure.Core shared, thanks?

[chunyu3]

And In http pipeline, the ClientRequestIdPolicy is added into pipeline by default, so each http request has x-ms-client-request-id and x-ms-return-client-request-id. see following:

https://github.com/Azure/azure-sdk-for-net/blob/592196f5c96a5bfdb7ab29fd28fec2db6edffdcd/sdk/core/Azure.Core/src/Pipeline/HttpPipelineBuilder.cs#L148-L149

policies.Add(ClientRequestIdPolicy.Shared);

But Batch service will change the header name to client-request-id and return-client-request-id. We need a way to either update the header name of clientRequestIdPolicy or we will add a new policy and also need to remove ClientRequestIdPolicy from the pipeline.

[lirenhe]

cc @m-nash

[m-nash]

@jsquire as discussed in our last core sync, @chunyu3 will do an estimation exercise on what it would take to update the generator / custom code to no longer use the policy but instead simply add the headers always at request creation time.

@christothes was going to do an estimation exercise on how many fully hand written libraries would be impacted.

After both of these we were going to decide if we wanted to go with replacing the policy or not.

If we decide to replace the policy we need to be able to roll this out in a 2 phase process where adding the headers at the request level doesn't conflict in a negative way with the policy. This could be a simple change in the policy to check existence before adding or some other mechanism. We would add the generator / manual code then once we are confident in the changes remove the policy from being added in the default pipeline.

If we decide not to replace the policy we need some public mechanism to change the name of the header to use since this is hard coded today.

In the meantime to unblock batch we will create an internal policy that runs after the Azure.Core one, removes the default named headers and adds the custom named headers.

[annelo-msft]

After both of these we were going to decide if we wanted to go with replacing the policy or not.

It would also be good to understand if we can remove the ClientRequestIdPolicy completely.

[christothes]

Looks like there are only a handful of clients affected, based on this script (some in this list may not be valid clients, like Azure.Identity for example)

Azure.Identity
Azure.Identity.BrokeredAuthentication
Azure.IoT.ModelsRepository
Azure.Messaging.EventHubs
Azure.Messaging.EventHubs.Experimental
Azure.Messaging.EventHubs.Processor
Azure.Messaging.ServiceBus
Azure.Messaging.WebPubSub.Client
Azure.Monitor.OpenTelemetry.AspNetCore
Azure.Monitor.OpenTelemetry.AspNetCore.Demo
Azure.Monitor.OpenTelemetry.Exporter.Benchmarks
Azure.Monitor.OpenTelemetry.Exporter.Demo
Azure.Security.KeyVault.Certificates
Azure.Security.KeyVault.Keys
Azure.Security.KeyVault.Secrets
Azure.Services.AppAuthentication
Azure.Services.AppAuthentication.TestCommon
Azure.Storage.Blobs.ChangeFeed
Azure.Storage.DataMovement
Azure.Storage.DataMovement.Blobs
Azure.Storage.Internal.Avro

[jsquire]

Azure.Messaging.EventHubs
Azure.Messaging.EventHubs.Experimental
Azure.Messaging.EventHubs.Processor

These are AMQP libraries and don't use the pipeline at all. I suspect most of WebPubSub falls into that as well.

• Azure.Messaging.ServiceBus uses the HTTP pipeline only for the admin client - I don't think there's impact there.
• Azure.Services.AppAuthentication is out of support, no longer maintained, and has been delisted from NuGet. We can just archive the source there.

[annelo-msft]

Per discussion in this week's Azure.Core meeting, investigation by the Azure.Core v-team is complete, and remaining work is on the generator side. @chunyu3, please let us know if that is consistent with your understanding. If so, I will move this to the generator repo next week.

[chunyu3]

Hello @christothes Your script is to find out all the SDKs which is not generated by codegen, and fully handle-written. Right?

But we need to figure out all SDKs which is generated by codegen and also has the customized CreateXXXRequest.

When we remove ClientRequestIDPolicy from pipeline (which will affect both mgmt and data-plane), we need to add the client-request-id header directly in the CreateXXXRequest method. For auto-gen CreateXXXRequest method, we can update codegen to automatically add those headers, but for customized CreateXXXRequest we need to manually update those method to add the header.

I have grepped out all those affected customized files in scirpt

Total files which contains customized CreateXXXRequest methods:46 Total customized CreateXXXRequest methods: 206 Total affected SDKs: 20 Azure.ResourceManager.Automation Azure.Communication.JobRouter Azure.Communication.PhoneNumbers Azure.DigitalTwins.Core Azure.ResourceManager.Dns Azure.Identity Azure.IoT.Hub.Service Azure.IoT.ModelsRepository Azure.Monitor.Ingestion Azure.Monitor.OpenTelemetry.Exporter Azure.ResourceManager.Monitor Azure.ResourceManager.NetApp Azure.AI.OpenAI Azure.ResourceManager.PrivateDns Azure.ResourceManager Azure.ResourceManager.SecurityCenter Azure.Storage.Blobs.Batch Azure.Data.Tables Azure.AI.TextAnalytics Azure.ResourceManager.TrafficManager

If we want to remove the ClientRequestIDPolicy from pipeline, following is what we need to do:

1. update codegen to automatically add header x-ms-client-request-id in CreateXXXRequest methods. which will affect both mgmt-track2 and dataplane.
2. Update existing 206 customized CreateXXXRequest methods to add header x-ms-client-request-id manually
3. regen all SDKs in azure-sdk-for-net
4. remove ClientRequestID policy from Azure.Core pipeline, update the Azure.Core version in azure-sdk-for-net
5. verify if all the SDKs add x-ms-client-request-id header in Http request message. This task is huge, because the test coverage if not high for all SDKs especially for mgmt-track2 SDKs, so we cannot rely on test to verify. There may be no efficient way to verify.
6. update all the test recordings if needed.

And one problem we may consider is that we need to tell customer to add this header when they write customized CreateXXXRequest.

[christothes]

If we want to remove the ClientRequestIDPolicy from pipeline, following is what we need to do:

1. update codegen to automatically add header x-ms-client-request-id in CreateXXXRequest methods. which will affect both mgmt-track2 and dataplane.

Just wanted to note that we also need to consider whether a ClientRequestIdScope exists, and use that requestId if so

2. Update existing 206 customized CreateXXXRequest methods to add header x-ms-client-request-id manually
3. regen all SDKs in azure-sdk-for-net
4. remove ClientRequestID policy from Azure.Core pipeline, update the Azure.Core version in azure-sdk-for-net
5. verify if all the SDKs add x-ms-client-request-id header in Http request message. This task is huge, because the test coverage if not high for all SDKs especially for mgmt-track2 SDKs, so we cannot rely on test to verify. There may be no efficient way to verify.

If we don't have recorded tests to validate this, it should be possible to create a fairly simple mocked transport test to validate this. Perhaps it could even be generated.

6. update all the test recordings if needed.

And one problem we may consider is that we need to tell customer to add this header when they write customized CreateXXXRequest.

I don't believe there are any scenarios where a customer would ever be customizing this. Library authors would, but given they would always start with the generated method that needs to be customized, this seems like a minor issue.

[annelo-msft]

It would be good to track any useful context in the notes on this issue: https://github.com/Azure/azure-sdk-for-net/issues/16704

[jsquire]

@m-nash, @annelo-msft: Was there a resolution to this and, if so, do we still need this issue open to track?

[annelo-msft]

When we last discussed this in our weekly stand-up, @christothes had taken point on exploring designs that could address the various considerations raised in the meeting.

[chunyu3]

Arch tech board decided not to add a new policy for this currently. We will set this header directly when createXXXRequest. Close this issue now.