-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
[Lambda DevX] Introducing Lambda Debug Mode Config #11363
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
Conversation
S3 Image Test Results (AMD64 / ARM64) 2 files 2 suites 3m 28s ⏱️ Results for commit ae01009. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work enabling multi-function debugging for Lambda 👏👏
Lovely validations and config test coverage 💯
We should enable telemetry for the debug flag and add the new configs to the config list for CLI compatibility. Otherwise, nothing blocking a merge (just nits, discussions).
Given the lack of automated testing (for now), I highly recommend adding a ## Testing
section with manual instructions including suggested testing steps for reviewers, potentially linking to an example (the example you demonstrated in our Tue session would be very suitable).
I would appreciate an extra pair of 👀 from @dfangl on this one ✨
if is_lambda_debug_mode(): | ||
timeout_seconds = DEFAULT_LAMBDA_DEBUG_MODE_TIMEOUT_SECONDS | ||
else: | ||
# TODO: integration timeouts should be enforced instead. | ||
# Do not wait longer for an invoke than the maximum lambda timeout plus a buffer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should we keep the buffer explanation here (it still applies)?
# The following logic selects which maximum waiting time to consider depending | ||
# on whether the application is being debugged or not. Note however, that if | ||
# debugging timeouts are disabled for the lambda function invoked at this endpoint, | ||
# the lambda function will already enforce the expected timeouts. | ||
if is_lambda_debug_mode(): | ||
timeout_seconds = DEFAULT_LAMBDA_DEBUG_MODE_TIMEOUT_SECONDS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could we add a clarifying comment as to why we still set a timeout here in debug mode? You motivated a good reason such that we ensure the hanging container eventually gets cleaned up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's mentioned in the first sentence above, but might be worth explicitly pointing out why this also applies to debug mode (feel free to ignore if it's too nit-picky; just wanted to perverse our motivation here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The explanation for DEFAULT_LAMBDA_DEBUG_MODE_TIMEOUT_SECONDS
clarifies 👍
# The following logic selects which maximum waiting time to consider depending | ||
# on whether the application is being debugged or not. Note however, that if | ||
# debugging timeouts are disabled for the lambda function invoked at this endpoint, | ||
# the lambda function will already enforce the expected timeouts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't fully understand the last part of the sentence, this refers to the timeouts enforced by the Lambda RIE, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct -- I have adjusted this comment for clarity
localstack-core/localstack/utils/lambda_debug_mode/lambda_debug_mode_config.py
Show resolved
Hide resolved
|
||
class LambdaDebugConfig(BaseModel): | ||
debug_port: Optional[int] = Field(None, alias="debug-port") | ||
timeout_disable: bool = Field(False, alias="timeout-disable") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glancing over our existing LocalStack configurations, we typically use DISABLE
as a prefix. Would it be more consistent to use disable-timeout
instead?
functions: dict[Arn, LambdaDebugConfig] | ||
|
||
|
||
class LambdaDebugModeConfigException(Exception): ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome validations and clear error messages 💯 👏👏
|
||
|
||
class _LambdaDebugModeSession: | ||
_instance = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dfangl Does this match our common way of creating singletons?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we usually create singletons with a .get()
function:
@staticmethod
@singleton_factory
def get() -> "_LambdaDebugModeSession":
"""Returns a singleton instance of the debug mode session."""
return _LambdaDebugModeSession()
I would prefer to avoid overriding __new__
here. Instead ofusing the module global, we can always call _LambdaDebugModeSession.get()
. Might be nice to rename it to avoid the leading _
then, though.
debug-port: null | ||
""" | ||
|
||
DEBUG_CONFIG_NULL_TIMEOUT_DISABLE = """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this contain a timeout disable flag?
@@ -433,6 +433,9 @@ def in_docker(): | |||
# When enabled it triggers specialised workflows for the debugging. | |||
LAMBDA_DEBUG_MODE = is_env_true("LAMBDA_DEBUG_MODE") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add LAMBDA_DEBUG_MODE
to the TRACKED_ENV_VAR
in localstack-core/localstack/runtime/analytics.py
for telemetry insights?
@@ -433,6 +433,9 @@ def in_docker(): | |||
# When enabled it triggers specialised workflows for the debugging. | |||
LAMBDA_DEBUG_MODE = is_env_true("LAMBDA_DEBUG_MODE") | |||
|
|||
# path to the lambda debug mode configuration file. | |||
LAMBDA_DEBUG_MODE_CONFIG_PATH = os.environ.get("LAMBDA_DEBUG_MODE_CONFIG_PATH") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add all new environment variables to CONFIG_ENV_VARS
in config.py
at the bottom such that they work using the CLI?
|
||
class LambdaDebugConfig(BaseModel): | ||
debug_port: Optional[int] = Field(None, alias="debug-port") | ||
timeout_disable: bool = Field(False, alias="timeout-disable") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering about the naming: Would enforce_timeouts: True|False
be clearer? What's your naming suggestion @dfangl @dominikschubert ?
Current behavior:
LAMBDA_DEBUG_MODE=0
-> never disable any timeoutLAMBDA_DEBUG_MODE=1
timeout-disable
not set or no debug config for Lambda function ARN available -> extend timeoutstimeout-disable=False
-> extend timeoutstimeout-disable=True
-> enforce timeouts as expected
Am I getting confused here or is this a logic error (the default value is False
, which gets negated in is_lambda_debug_timeout_for
) 🤔
sidenote: Technically, we're not fully disabling the timeouts, but extend them.
We need to document the behavior clearly after merging ...
PS: migrated from wrong follow-up PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you here, it is confusing. For a variable called timeout-disable
I would expect the flag being reversed. I personally prefer enforce_timeout
in this case, but even if we reverse it, I am not a bit fan of timeout-disable
, I think even disable-timouts
would be clearer here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All in all, this is a great step towards improving DevX! I just had some minor nits, but nothing major!
return debug_configuration.debug_port | ||
|
||
|
||
def is_lambda_debug_timeout_for(lambda_arn: Arn) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to rename this function to be clearer, and maybe add docstrings to this entire module.
is_lambda_debug_timeout_for
does not really make sense for me. Maybe is_lambda_debug_timeout_enabled_for
?
if is_qualified and lambda_arn_parts[-1]: | ||
return lambda_arn | ||
|
||
# The arn is not unqualified, but probably erroneous, pass the value upstream. | ||
is_unqualified = lambda_arn_parts_len == 7 | ||
if not is_unqualified: | ||
return lambda_arn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can see, we just ignore invalid arns, for example those ending with :
(so they have 8 parts, but are missing the qualifier, or totally invalid arns) and still set them in the internal config? Personally, I would prefer raising an error to silently accepting them, because they will never match anything.
|
||
|
||
class _LambdaDebugModeSession: | ||
_instance = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we usually create singletons with a .get()
function:
@staticmethod
@singleton_factory
def get() -> "_LambdaDebugModeSession":
"""Returns a singleton instance of the debug mode session."""
return _LambdaDebugModeSession()
I would prefer to avoid overriding __new__
here. Instead ofusing the module global, we can always call _LambdaDebugModeSession.get()
. Might be nice to rename it to avoid the leading _
then, though.
def _validate_lambda_debug_mode_config( | ||
validation_state: _LambdaDebugModeConfigValidationState, config: LambdaDebugModeConfig | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is called validate_
but does actively change the values of the config (by replacing unqualified ARNs with qualified ARNs). It should be clear from the method that we are not only validating here, but also "canonicalizing". So maybe renaming it, or at least adding it in a docstring or something.
functions: | ||
arn:aws:lambda:eu-central-1:000000000000:function:functionname: | ||
debug-port: null | ||
enforce-timeout: null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The flag is now called enforce-timeouts
, not enforce-timeout
. So we are not testing the null
value here, but actually the default, as pydantic per default ignores extra fields.
Motivation
Currently, Lambda Debug Mode supports debugging of only one Lambda function at a time, using a remote debugger connected via a port specified through Lambda Docker Flags during startup.
The proposed changes introduce a new configuration feature for Lambda Debug Mode that enables users to assign specific debug ports to each Lambda function, as well as other parameters. This is achieved though a yaml file, such as:
In this initial release, users can also disable the automatic timeout extensions that are normally applied by starting LocalStack with Lambda Debug Mode on.
The configuration can be passed to LocalStack through the new
LAMBDA_DEBUG_MODE_CONFIG_PATH
environment variable, which once set is only sourced and reflected in the running logic of the app isLAMBDA_DEBUG_MODE
is set.Changes
LAMBDA_DEBUG_MODE_CONFIG_PATH
which sources the yaml Lambda Debug Mode Config fileLAMBDA_DEBUG_MODE_CONFIG_PATH
, as well as useful error messaging logs for each of these stepsTesting
We do not yet have the necessary stack to test for lambda debugging integration tests. What follows is a guide to manually test a base scenario of this feature.
Deploy Lambda Functions
Configure Debugging
Enable Lambda Debug Mode
Debug