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

Add lambda timeout option #13

Merged
merged 3 commits into from
Jun 27, 2024
Merged

Conversation

constanca-m
Copy link
Contributor

@constanca-m constanca-m commented Jun 27, 2024

Add a custom timeout option for the lambda.

This issue comes from elastic/elastic-serverless-forwarder#729 (comment).

This option is not available on SAR from what I have seen.

Just add:

lambda-timeout  = 60

And in AWS you should see in your lambda configuration:
image

@constanca-m constanca-m self-assigned this Jun 27, 2024
@constanca-m constanca-m requested a review from a team as a code owner June 27, 2024 14:14
@constanca-m constanca-m mentioned this pull request Jun 27, 2024
9 tasks
variables.tf Outdated
variable "lambda-timeout" {
description = "The amount of time your Lambda Function has to run in seconds."
type = number
default = 3

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@constanca-m Considering the kind of error that lambda timeout triggers and the difficulty in correlating them, I'd suggest keeping this to 15 min, in line to what the SAR deployment is doing:

 ApplicationElasticServerlessForwarder:
    Type: AWS::Serverless::Function
    Properties:
      Timeout: 900
      MemorySize: 512
      CodeUri:
        Bucket: <%REPO_BUCKET%>
        Key: ae7562c6-4e78-4865-91a9-968459fba0ab

Coincidentally, this PR will supersede #12.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree, thank you for checking!! I updated it now to 900s as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I had not noticed your PR. Do you want to have the 900s as an option or not? Otherwise I will just approve yours @emilioalvap

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@constanca-m No worries, I think providing the option is a better approach, I'll close mine

Copy link

@emilioalvap emilioalvap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, tested with terraform and timeout is being updated:

 ~ timeout                        = 900 -> 30

@constanca-m constanca-m merged commit 55d8d3d into elastic:main Jun 27, 2024
2 checks passed
@constanca-m constanca-m deleted the add-timeout branch June 27, 2024 15:49
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.

2 participants