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

feat(middleware-user-agent): cache user agent string #3970

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TysonAndre
Copy link
Contributor

Cache the user agent string after the first successful computation. Do this in a way that ensures await isn't called in the middleware if the user agent is already computed (run middleware in single tick).

Middleware such as the user agent is called on every single call to aws, so for dbs (e.g. fetching single keys), the middleware should in principle be memory/cpu efficient.

Additionally, this would mitigate issues similar to #2027 for any clients with different implementations of defaultUserAgentProvider (in the future, since that issue was already fixed) (if those implementations read files, make service calls, etc)

Context: I saw that client-dynamodb calls took twice as long to resolve, and cpu usage was slightly higher after switching from aws-sdk, even after the fix for #2223. The impact on dynamodb promise resolution times was higher at peak load when Node.js was using more cpu per process. (The Node.js process had around 40% cpu usage)

  • I'm not completely certain this is the cause, but it seems related. There's extra function calls, map, array creation/join, and string escaping. Additionally, the call to await() would spread out the work of creating the user agent across multiple CPU ticks.
  • An alternate reason might have been using the default maxSockets of 50 after the refactoring - I previously had it at 250 and passed in custom httpAgent/httpsAgent because the application could have large numbers of concurrent calls
  • (Related to Default AWS credentials provider chain resolution changed in v3 for EC2MetadataCredentials/ECSCredentials - readFile calls more frequent #2027 which was fixed by memoizing in the implementation of defaultUserAgentProvider)

Avoid extra cpu from calling map, defaultUserAgentProvider+await, etc.

As a result of 388b180 switching from defaultUserAgent to defaultUserAgentProvider, the middleware would have to recompute the user agent on every single api call. This fixes that by computing the user agent only once, when the middleware is instantiated. (So creating a client but not using it will now call this, but creating a client has overhead anyway)

Issue

Issue number, if available, prefixed with "#"

Description

Reduces cpu overhead of recomputing user agent on every created http request, avoid an await that would postpone
work until the next cpu tick

Testing

How was this change tested?

  • user-agent-middleware.spec.ts was run

TODO:

  • Are there benchmarks comparing time to resolve promises for concurrent calls with aws-sdk (monolithic v2) and aws-sdk-js-v3? (I saw worse response times in APIs after switching from aws-sdk to aws-sdk-js-v3 with dynamodb client for get/set in April 2021, despite doing the same operations)

    Or are there tickets to create these benchmarks?

  • Optionally, Write a short script to benchmark calls per second and cpu per call with realistic user agent values?

  • Verify that middleware calls next() in the same tick

Additional context

Add any other context about the PR here.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Cache the user agent string after the first successful computation.
Do this in a way that ensures `await` isn't called in the middleware if
the user agent is already computed (run middleware in single tick).

Middleware such as the user agent is called on every single call to aws,
so for dbs (e.g. fetching single keys), the middleware should in
principle be memory/cpu efficient.

Context: I saw that client-dynamodb calls took twice as long to resolve,
and cpu usage was slightly higher after switching from aws-sdk,
even after the fix for 2223. The impact on dynamodb promise resolution
times was higher at peak load when Node.js was using more cpu per process.
(The Node.js process had around 40% cpu usage)
- I'm not completely certain this is the cause, but it seems related.
  There's extra function calls, map, array creation/join, and
  string escaping.
  Additionally, the call to await() would spread out the work of
  creating the user agent across multiple CPU ticks.
- (Related to 2027 which was fixed by memoizing in the implementation of
  defaultUserAgentProvider)

Avoid extra cpu from calling map, defaultUserAgentProvider+await, etc.

As a result of 388b180 switching from
defaultUserAgent to defaultUserAgentProvider, the middleware would have
to recompute the user agent on every single api call. This fixes that by
computing the user agent only once, when the middleware is instantiated.
(So creating a client but not using it will now call this, but creating
a client has overhead anyway)
@TysonAndre TysonAndre requested a review from a team as a code owner September 20, 2022 16:39
@kuhe kuhe self-assigned this Aug 31, 2023
@kuhe kuhe added the queued This issues is on the AWS team's backlog label Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
queued This issues is on the AWS team's backlog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants