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.

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

Failure to set DefaultConfigurationMode causes performance issues during client construction #2022

Closed
Hawxy opened this issue May 11, 2022 · 7 comments
Assignees
Labels
bug This issue is a bug. low-effort module/sdk-core p2 This is a standard priority issue perf queued

Comments

@Hawxy
Copy link

Hawxy commented May 11, 2022

Describe the bug

This is a follow up to #1996 and is possibly related to #2000

Following an update to the SDK, the initial construction of the client on a network request increased to more than 30 seconds. I've spent some more time on this issue and my findings are below.

Expected Behavior

Client construction should be under 2 seconds.

Current Behavior

Client construction takes more than 30 seconds as you can see in the call tree:

image

Reproduction Steps

  1. Create a .NET 6 API with AWSSDK.Extensions.NETCore.Setup + AWSSDK.S3.
  2. Configure an AWS profile and set the configuration in appsettings.json
  3. Ensure no Environment variables or other settings cause DefaultsMode to be set.
  4. Configure DI:
 services.AddDefaultAWSOptions(Configuration.GetAWSOptions());
 services.AddAWSService<IAmazonS3>();
  1. Request IAmazonS3 in a controller.

Possible Solution

Stepping through the problem, the cause is this check:

if (string.Equals(property.Name, "RetryMode", StringComparison.Ordinal) &&
defaultConfig.RetryMode == RequestRetryMode.Legacy &&
config.DefaultConfigurationMode != DefaultConfigurationMode.Legacy)

When the getter is called on DefaultConfigurationMode, if it hasn't already been set it and is not available locally, it ends up calling this.DefaultConfiguration which then results in the call tree above.

Setting "DefaultsMode": "Standard" in appsettings resolves the problem.

DefaultsMode/DefaultConfigurationMode is not well documented in regards to the .NET SDK and I don't see it in the examples, thus I would consider this unexpected behavior for the majority of consumers.

Additional Information/Context

No response

AWS .NET SDK and/or Package version used

AWSSDK.S3 3.7.9.2
AWSSDK.Extensions.NETCore.Setup 3.7.2

Targeted .NET Platform

.NET 6

Operating System and version

Windows 11

@Hawxy Hawxy added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 11, 2022
@ashishdhingra ashishdhingra added module/sdk-core perf A low-effort and removed needs-triage This issue or PR still needs to be triaged. labels May 11, 2022
@puddlewitt
Copy link

puddlewitt commented May 16, 2022

I am seeing a similar issue. For me this manifests itself as a severe delay in startup performance when under load.

  1. New ECS task started in the EC2 cluster
  2. New ECS task passes first HealthCheck and is deemed ready by the TargetGroup
  3. New ECS task starts receiving traffic from the load balancer
  4. New ECS task does not respond to any traffic due to the lock
  5. New ECS task does not respond to the HealthCheck from the TargetGroup
  6. New ECS task is deemed unhealthy and is removed (this process is then repeated over and over)

image

My solution so far is a /health endpoint which is invoked by the TargetGroup during the initial HealthCheck which invokes the following methods

_amazonDynamoDb.Config.Validate();
_amazonCloudWatch.Config.Validate();

@tomupson
Copy link

tomupson commented Aug 27, 2022

+1. Cost me 4 hours of debugging

Could do with documenting that this is necessary configuration if that is the case

@ashishdhingra ashishdhingra added p2 This is a standard priority issue queued and removed A labels Nov 2, 2022
@zippo227
Copy link

I recommend that it be set to Standard by default without needing to traverse an entire tree for 30 seconds to find the best thing to be.

@dscpinheiro
Copy link
Contributor

Sorry for the delay in the response, but do you still see the problem in the latest version of the AWSSDK.Extensions.NETCore.Setup and AWSSDK.S3 packages? I haven't been able to reproduce it locally, but at the same time I couldn't find a specific PR / commit fixing this issue... (so it's likely I'm missing something).

@dscpinheiro dscpinheiro added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Oct 3, 2024
@Hawxy
Copy link
Author

Hawxy commented Oct 3, 2024

@dscpinheiro I came across a reddit thread of someone running into this issue just last week so it's still an issue in v3. I noticed this codepath got rewritten in v4 so I assume it won't be an issue there.

I can check again but if that getter behaviour hasn't been touched I should be able to reproduce it.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Oct 4, 2024
@normj
Copy link
Member

normj commented Oct 18, 2024

We have made changes in V3 to avoid accidently triggering the DefaultConfigurationMode resolution prematurely. In V4 which is currently in preview we have rewritten this code to get rid of the reflection and have the default config in AWSSDK.Extensions.NETCore.Setup not directly inherit from the ClientConfig in the SDK and instead use a simple type that ensures this regression won't happen again.

@normj normj closed this as completed Oct 18, 2024
Copy link

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. low-effort module/sdk-core p2 This is a standard priority issue perf queued
Projects
None yet
Development

No branches or pull requests

7 participants