Skip to content

Use one connection for all communication with extension. #405

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

Closed
wants to merge 2 commits into from

Conversation

purple4reina
Copy link
Contributor

@purple4reina purple4reina commented Dec 13, 2023

What does this PR do?

Changes all communication with the extension to using a persistent http connection.

Motivation

This change reduces runtime duration by somewhere between 5-10%.

Screenshot 2023-12-14 at 10 01 45 AM

Testing Guidelines

⚠️ This is a high risk change. Sufficient testing must be done before merging! ⚠️

As I run tests on this branch, I will post the results below as comments.

Additional Notes

Types of Changes

  • Bug fix
  • New feature
  • Breaking change
  • Misc (docs, refactoring, dependency upgrade, etc.)

Check all that apply

  • This PR's description is comprehensive
  • This PR contains breaking changes that are documented in the description
  • This PR introduces new APIs or parameters that are documented and unlikely to change in the foreseeable future
  • This PR impacts documentation, and it has been updated (or a ticket has been logged)
  • This PR's changes are covered by the automated tests
  • This PR collects user input/sensitive content into Datadog
  • This PR passes the integration tests (ask a Datadog member to run the tests)

@purple4reina purple4reina force-pushed the rey.abolofia/keep-alive branch 2 times, most recently from 2017ff7 to 5fdef45 Compare December 14, 2023 17:40
@purple4reina purple4reina force-pushed the rey.abolofia/keep-alive branch from 5fdef45 to 9c5019c Compare December 14, 2023 17:42
@purple4reina purple4reina marked this pull request as ready for review December 14, 2023 18:03
@purple4reina purple4reina requested a review from a team as a code owner December 14, 2023 18:03
@purple4reina
Copy link
Contributor Author

I created two apps, one using v85 of the prod layer and another using a layer built from this branch. I then curled the two apps on a loop:

while true ; do curl [URL1] ; curl [URL2] ; done

I let this run for several hours. Here are stats from a 4 hour window of this run:

Screenshot 2023-12-14 at 10 06 50 AM

@purple4reina
Copy link
Contributor Author

I created two apps, one using v85 of the prod layer and another using a layer built from this branch. I then curled the two apps on a loop with a sleep in between:

while true ; do curl [URL1] ; curl [URL2] ; sleep 120 ; done

I let this run for about 30 minutes. Here are stats:

Screenshot 2023-12-14 at 10 16 26 AM

As you can see, the difference is not as large as it was for the busy function. Since there are no errors, I will say that this PR neither hurts nor helps apps like these.

@purple4reina
Copy link
Contributor Author

I created two apps, one using v85 of the prod layer and another using a layer built from this branch. The functions will timeout during init on the first try, but succeed on the second.

import os
import time

filename = '/tmp/testfile'
if not os.path.exists(filename):
    # cause timeout on first invocation only
    with open(filename, 'w') as f:
        f.write('hello')
    time.sleep(600)

def handler(event, context):
    return 'ok'

I then curled the two apps on a loop, changing the max memory in between in order to force a cold start.

Screenshot 2023-12-14 at 10 50 04 AM

In neither the prod nor testing layer versions was I able to receive the aws.lambda.enhanced.init_duration metric. Therefore, I am unable to state the overhead impact on this scenario.

On the other hand, the function still performs as expected when using the new layer.

@purple4reina
Copy link
Contributor Author

Here is a test with functions that are forced to cold start on each invocation. We can see that there is no significant change in overhead on cold start.

Screenshot 2023-12-14 at 11 27 35 AM

@purple4reina
Copy link
Contributor Author

Closing in favor of #406. The flush endpoint on the extension does nothing, therefore we shouldn't be calling it in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant