Refactor User-Agent construction in Marshaller.cs #3423

paulomorgado · 2024-08-05T18:26:22Z

Updated SetUserAgentHeader to improve User-Agent string construction:

Initialize StringBuilder with a capacity of 256.
Append UserAgent after replacing invalid characters.
Append ClientAppId after replacing invalid characters.
Use ToUserAgentHeaderString for retry mode conversion.
Append IsAsync and initialization config directly.
Append UserAgentAddition after replacing invalid characters.
Removed redundant call to ReplaceInvalidUserAgentCharacters.
Added ToUserAgentHeaderString method for RequestRetryMode.

Description

See #3412

Motivation and Context

See #3412

Testing

There should be no changes to the result.

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

My code follows the code style of this project
My change requires a change to the documentation
I have updated the documentation accordingly
I have read the README document
I have added tests to cover my changes
All new and existing tests passed

License

I confirm that this pull request can be released under the Apache 2 license

muhammad-othman · 2024-08-06T16:30:39Z

sdk/src/Core/Amazon.Runtime/Pipeline/Handlers/Marshaller.cs

+
+        private static string ToUserAgentHeaderString(RequestRetryMode requestRetryMode)
+        {
+            if (requestRetryMode == RequestRetryMode.Adaptive)


This change seems a bit limited since we have Legacy mode and we may add other retry modes

I'm sorry, @muhammad-othman, but I don't understand your point.

RequestRetryMode has only 2 values: Adaptive and Standard. There's no Legacy value.

This code can be generated using something like NetEscapades.EnumGenerators. This might me a bit more specific, but nothing that cannot be easily done. I just didn't want to taggle that in this PR.

I've added a Debug.Assert to assert the value is the expected one, when in DEBUG.

We had it before but looks like we removed it at V4, still we may add other RequestRetryMode in the future,.

aws-sdk-net/sdk/src/Core/Amazon.Runtime/Enumerations.cs

Line 94 in 2c7d520

Legacy,

Debug.Assert looks like something we may miss, maybe we can add an else statement that throws an exception instead to be sure.

You need something that catches that at build-time, not at run-time.

Having Debug.Assert catches that at debug-time.

Comprehensive unit tests will also catch this kind of errors.

I don't mind pushing this to some utility class adding a comprehensive of unit tests, if you point me in the right direction.

I was just thinking that Debug.Assert might be lost and someone adding a new retry mode will run atleast one request to test their changes, but I agree unit tests are much better.

You can use this utility method to make sure that the enum didn't change with a note to update ToUserAgentHeaderString if it changed.
https://github.com/aws/aws-sdk-net/blob/v4-development/sdk/test/Common/Utils/AssertExtensions.cs#L137
And here is an example of its usage

aws-sdk-net/sdk/test/Services/SecurityToken/UnitTests/Custom/StaticCheckTests.cs

Line 44 in 8e74d19

public void LookForProfileTypeChanges()

And you can add the tests here
https://github.com/aws/aws-sdk-net/blob/v4-development/sdk/test/UnitTests/Custom/Runtime

@muhammad-othman,

And you can add the tests here https://github.com/aws/aws-sdk-net/blob/v4-development/sdk/test/UnitTests/Custom/Runtime

What solution would that be? There are 405 .sln files in the repo.

Basically any service solution will include the core UnitTestUtilities porject, here is an example from S3

Please include the newly add test file the following projects as we don't include all the files added to the Runtime folder by default

sdk\test\UnitTests\Custom\AWSSDK.UnitTestUtilities.NetFramework.csproj sdk\test\UnitTests\Custom\AWSSDK.UnitTestUtilities.NetStandard.csproj

After digging a little bit into the code, I realized that there is no validation when IRequestContext.ClientConfig.RetryMode, so I changed ToUserAgentHeaderString to have a fast path for know values and a fallback of requestRetryMode.ToString().ToLowerInvariant():

private static string ToUserAgentHeaderString(RequestRetryMode requestRetryMode) { switch (requestRetryMode) { case RequestRetryMode.Standard: return "standard"; case RequestRetryMode.Adaptive: return "adaptive"; default: return requestRetryMode.ToString().ToLowerInvariant(); } }

Added this to sdk/test/Services/SecurityToken/UnitTests/Custom/StaticCheckTests.cs:

[TestMethod] public void LookForRequestRetryModeChanges() { var expectedHash = "2FC699DB6A4284C5D53F3B7FA4E64165576FD1F6AE458BC737AE503F06DFBE4C"; AssertExtensions.AssertEnumUnchanged( typeof(RequestRetryMode), expectedHash, "The Amazon.Runtime.Internal.Marshaller.ToUserAgentHeaderString method implementation may need to be updated."); }

sdk/src/Core/Amazon.Runtime/Pipeline/Handlers/Marshaller.cs

muhammad-othman · 2024-08-08T23:20:28Z

Thank you for making the requested changes.
Running this PR through our internal dry run system and will merge it after that.

paulomorgado · 2024-08-09T16:15:58Z

@normj, @muhammad-othman,

I noticed that RequestRetryMode is changing from:

/// <summary>
/// RetryMode determines which request retry mode is used for requests that do 
/// not complete successfully.
/// </summary>
public enum RequestRetryMode
{
    /// <summary>
    /// Legacy request retry strategy.
    /// </summary>
    Legacy,
    /// <summary>
    /// Standardized request retry strategy that is consistent across all SDKs.
    /// </summary>
    Standard,
    /// <summary>
    /// An experimental request retry strategy that builds on the Standard strategy
    /// and introduces congestion control through client side rate limiting.
    /// </summary>
    Adaptive
}

/// <summary>
/// RetryMode determines which request retry mode is used for requests that do 
/// not complete successfully.
/// </summary>
public enum RequestRetryMode
{
    /// <summary>
    /// Standardized request retry strategy that is consistent across all SDKs.
    /// </summary>
    Standard,
    /// <summary>
    /// An experimental request retry strategy that builds on the Standard strategy
    /// and introduces congestion control through client side rate limiting.
    /// </summary>
    Adaptive
}

This might be an issue for anyone saving these settings as integers because it will be changing from:

Legacy = 0
Standard = 1
Adaptive = 2

to:

Standard = 0
Adaptive = 1

So, Legacy becomes Standard, Standard becomes Adaptive and Adaptive becomes invalid.

normj · 2024-08-09T16:57:23Z

For the way this enum is used I don't see anybody relying on the integer value. It it would be set via the property on the config or via an environment variable which uses the string value. But it wouldn't be bad thing to assign standard and adaptive the same integer value as v3 in case there is some scenario I can't think of.

muhammad-othman · 2024-08-09T17:03:41Z

@paulomorgado Our dry run failed because there conflicts between this branch and the v4-development, can you check these conflicts?

paulomorgado · 2024-08-09T17:48:20Z

Hi @muhammad-othman,

@paulomorgado Our dry run failed because there conflicts between this branch and the v4-development, can you check these conflicts?

I don't know what those conflicts are, but I rebased on the latest commit of v4-development.

paulomorgado · 2024-08-09T18:00:41Z

Hi @normj,

For the way this enum is used I don't see anybody relying on the integer value. It it would be set via the property on the config or via an environment variable which uses the string value. But it wouldn't be bad thing to assign standard and adaptive the same integer value as v3 in case there is some scenario I can't think of.

I'm using here an in-memory collection as a simple example, but it can be any configuration source:

var configuration = new ConfigurationBuilder()
    .AddInMemoryCollection(new Dictionary<string, string?>
    {
        ["Amazon:KeyManagementService:RetryMode"] = "2",
    })
    .Build();

var keyManagmentServiceConfig = new AmazonKeyManagementServiceConfig();
configuration.GetSection("Amazon:KeyManagementService").Bind(keyManagmentServiceConfig);

Console.WriteLine($"The retry mode is: {keyManagmentServiceConfig.RetryMode}");

// The retry mode is: Adaptive

The value of an enum is always a scalar value: byte, short, int, long. Adding to that, the literal 0 is implicitly convertible to any enum type.

muhammad-othman · 2024-08-16T15:59:41Z

Hi @paulomorgado,
I believe the RequestRetryMode discussion doesn't have to be resolved in this PR and this PR is ready to be merged. Do you have any changes you want to add before merging?

Updated `SetUserAgentHeader` to improve User-Agent string construction: - Initialize `StringBuilder` with a capacity of 256. - Append `UserAgent` after replacing invalid characters. - Append `ClientAppId` after replacing invalid characters. - Use `ToUserAgentHeaderString` for retry mode conversion. - Append `IsAsync` and initialization config directly. - Append `UserAgentAddition` after replacing invalid characters. - Removed redundant call to `ReplaceInvalidUserAgentCharacters`. - Added `ToUserAgentHeaderString` method for `RequestRetryMode`.

Added a `using System.Diagnostics;` directive. Introduced a `retryMode` variable and added a `Debug.Assert` to ensure its correctness. Updated user agent string logic to use `retryMode` and `requestContext.IsAsync`. Changed initialization collections flag to `AWSConfigs.InitializeCollections`.

Refactored the ToUserAgentHeaderString method in Marshaller.cs to use a switch statement instead of if-else statements, including a default case for converting requestRetryMode to lowercase. Removed Debug.Assert for retryMode and directly appended it to the StringBuilder object. Added LookForRequestRetryModeChanges test in StaticCheckTests.cs to verify the integrity of the RequestRetryMode enum.

…serAgentHeaderString

paulomorgado · 2024-08-16T16:32:50Z

Hi @paulomorgado, I believe the RequestRetryMode discussion doesn't have to be resolved in this PR and this PR is ready to be merged. Do you have any changes you want to add before merging?

That discussion was a general heads up on the issue.

I don't have anything to add to this PR.

paulomorgado changed the base branch from main to v4-development August 5, 2024 18:26

dscpinheiro added the v4 label Aug 5, 2024

muhammad-othman requested changes Aug 6, 2024

View reviewed changes

philasmar requested a review from normj August 6, 2024 17:27

normj requested changes Aug 6, 2024

View reviewed changes

sdk/src/Core/Amazon.Runtime/Pipeline/Handlers/Marshaller.cs Outdated Show resolved Hide resolved

normj approved these changes Aug 8, 2024

View reviewed changes

muhammad-othman approved these changes Aug 8, 2024

View reviewed changes

paulomorgado force-pushed the performance/string-builder-1 branch from 6e384a2 to 9d267a2 Compare August 9, 2024 17:47

paulomorgado force-pushed the performance/string-builder-1 branch from 9d267a2 to 58989b5 Compare August 15, 2024 12:09

dscpinheiro requested a review from muhammad-othman August 16, 2024 13:15

muhammad-othman approved these changes Aug 16, 2024

View reviewed changes

paulomorgado and others added 4 commits August 16, 2024 12:18

Add devConfig and suppress NormalizeStringsToUppercase warning in ToU…

8dc4639

…serAgentHeaderString

muhammad-othman force-pushed the performance/string-builder-1 branch from 36c21e0 to 8dc4639 Compare August 16, 2024 16:19

muhammad-othman merged commit 3fb7211 into aws:v4-development Aug 16, 2024
1 check passed

paulomorgado deleted the performance/string-builder-1 branch August 20, 2024 12:55

dscpinheiro mentioned this pull request Aug 24, 2024

V4 Development Tracker #3362

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor User-Agent construction in Marshaller.cs #3423

Refactor User-Agent construction in Marshaller.cs #3423

paulomorgado commented Aug 5, 2024

muhammad-othman Aug 6, 2024

paulomorgado Aug 6, 2024

paulomorgado Aug 6, 2024

muhammad-othman Aug 6, 2024

paulomorgado Aug 6, 2024

muhammad-othman Aug 6, 2024

paulomorgado Aug 6, 2024

muhammad-othman Aug 6, 2024

paulomorgado Aug 7, 2024

muhammad-othman commented Aug 8, 2024

paulomorgado commented Aug 9, 2024

normj commented Aug 9, 2024

muhammad-othman commented Aug 9, 2024

paulomorgado commented Aug 9, 2024

paulomorgado commented Aug 9, 2024

muhammad-othman commented Aug 16, 2024

paulomorgado commented Aug 16, 2024

Refactor User-Agent construction in Marshaller.cs #3423

Refactor User-Agent construction in Marshaller.cs #3423

Conversation

paulomorgado commented Aug 5, 2024

Description

Motivation and Context

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muhammad-othman commented Aug 8, 2024

paulomorgado commented Aug 9, 2024

normj commented Aug 9, 2024

muhammad-othman commented Aug 9, 2024

paulomorgado commented Aug 9, 2024

paulomorgado commented Aug 9, 2024

muhammad-othman commented Aug 16, 2024

paulomorgado commented Aug 16, 2024