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

Maintenance: e2e test invocation always forces cold starts #590

Closed
dreamorosi opened this issue Feb 24, 2022 · 3 comments
Closed

Maintenance: e2e test invocation always forces cold starts #590

dreamorosi opened this issue Feb 24, 2022 · 3 comments
Labels
completed This item is complete and has been merged/shipped good-first-issue Something that is suitable for those who want to start contributing internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) logger This item relates to the Logger Utility tests PRs that add or change tests

Comments

@dreamorosi
Copy link
Contributor

Bug description

The current implementation of the helper function that invokes functions deployed as part of the e2e test of the logger calls the functions in parallel. This has the potentially unintended effect of always forcing cold starts in the functions. This happens because the Lambda service receives two concurrent invocation requests and as such it has to spin up two execution environments to handle them.

Some of the test cases, like this one, that intend to assert that the correct cold start value is included in the log entries are affected as result and can never pass.

Expected Behavior

Test case to pass if tested code is correct.

Current Behavior

Test case always fails.

Possible Solution

Two possible solutions:

Option A

Change the helper function to always invoke functions in sequence rather than in parallel, like so (this implementation makes the test pass with the changes in #550):

export const invokeFunction = async (functionName: string, times: number = 1): Promise<InvocationLogs[]> => {
  const invocationLogs: InvocationLogs[] = [];
    
  for (let i = 0; i < times; i++) {
    const response = await lambdaClient
      .invoke({
        FunctionName: functionName,
        LogType: 'Tail', // Wait until execution completes and return all logs
      })
      .promise();
    if (response?.LogResult) {
      invocationLogs.push(new InvocationLogs(response?.LogResult));
    } else {
      throw new Error('No LogResult field returned in the response of Lambda invocation. This should not happen.');
    }
  }

  return invocationLogs; 
};

Option B

Change the helper function to accept a third param parallel (defaults to true) and let the caller decide (pseudo-code example):

export const invokeFunction = async (functionName: string, times: number = 1, parallel: boolean = true): Promise<InvocationLogs[]> => {
  const invocationLogs: InvocationLogs[] = [];
  const promises = [];
   
  if (parallel) {
    // ... logic that invokes functions in parallel
  } else {
    // ... logic that invokes functions sequentially
  }

  return invocationLogs; 
};

The benefit of this second option is that since most test cases don't care about the mode of invocation they could have slightly faster execution time (since they'd be invoked in parallel).
The downside of this option is that there's now additional cognitive load for maintainers since this introduces two branches in the code and in case of failure we'd have to first check which mode was used.

Steps to Reproduce

  1. Enable tracing in the e2eUtils.ts file for the functions part of the test
  2. Run e2e tests for logger
  3. Observe results traces generated (attached below)
    Screenshot 2022-02-24 at 13 26 06
  4. See that first execution trace has an Initialisation phase in the X-Ray generated traces (these are generated by Lambda & X-Ray, Powertools Tracer is not being used)
    Screenshot 2022-02-24 at 13 26 17
  5. See second execution trace and observe that this also has an Initialisation phase6.
    Screenshot 2022-02-24 at 13 26 27

Environment

  • Powertools version used: N/A
  • Packaging format (Layers, npm): N/A
  • AWS Lambda function runtime: all
  • Debugging logs: N/A

Related issues, RFCs

#550

@dreamorosi dreamorosi added bug Something isn't working triage This item has not been triaged by a maintainer, please wait labels Feb 24, 2022
@saragerion saragerion added this to the production-ready-release milestone Mar 8, 2022
@saragerion saragerion added priority:high good-first-issue Something that is suitable for those who want to start contributing labels Mar 8, 2022
@ghost
Copy link

ghost commented Mar 14, 2022

@dreamorosi Happy to help with this one!

I really prefer the option B (optional parameter for execution mode) because it gives much more flexibility and doesn't cause unnecessary delays. I'll try to make the code as readable as possible to minimize a risk of any misunderstanding when reading/investigating the execution path.

@ijemmy
Copy link
Contributor

ijemmy commented Mar 14, 2022

I'm also supportive of option B.

In Logger, we have a sample rate testing. In this case, I did it in parallel as I have to run Lambda 20+ times.

@ijemmy ijemmy assigned ghost Mar 14, 2022
@dreamorosi dreamorosi removed the triage This item has not been triaged by a maintainer, please wait label Mar 14, 2022
@github-actions
Copy link
Contributor

⚠️ COMMENT VISIBILITY WARNING ⚠️

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.

@dreamorosi dreamorosi changed the title Bug (logger): e2e test invocation always forces cold starts Bug: e2e test invocation always forces cold starts Nov 14, 2022
@dreamorosi dreamorosi added the completed This item is complete and has been merged/shipped label Nov 14, 2022
@dreamorosi dreamorosi changed the title Bug: e2e test invocation always forces cold starts Maintenance: e2e test invocation always forces cold starts Nov 14, 2022
@dreamorosi dreamorosi added logger This item relates to the Logger Utility internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) tests PRs that add or change tests and removed bug Something isn't working labels Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
completed This item is complete and has been merged/shipped good-first-issue Something that is suitable for those who want to start contributing internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) logger This item relates to the Logger Utility tests PRs that add or change tests
Projects
None yet
Development

No branches or pull requests

3 participants