fix: add typecheck to lambda wrapper #533
Conversation
datadog_lambda/tracing.py
Outdated
| @@ -411,6 +411,9 @@ def is_legacy_lambda_step_function(event): | |||
| """ | |||
| Check if the event is a step function that called a legacy lambda | |||
| """ | |||
| if not isinstance(event, dict): | |||
There was a problem hiding this comment.
do we have other code that assumes that this is a dict in the wrapper flow that will trip over this problem?
also, should we look at the contents of the list if it is a list? or is that impossible for a legacy lambda step function?
There was a problem hiding this comment.
I dont' think we need to look through the whole list. If it's not a dict, we know right away that it's not a step function event.
As for the isinstance, we do have other checks for if the event is a dict. For example in determining trace propagation we skip altogether if it's not a dict. Best answer could even be surrounding the whole thing with a try/except.
There was a problem hiding this comment.
extract_trigger_tags() which is called right after is_legacy_lambda_step_function() expects a dict, assuming there's more
it should be impossible for legacy lambda case
according to the issue
In this case the incoming event is not a dictionary but a list of dictionaries that's generated by a dynamodb stream in an aws eventbridge pipe with batching enabling
The customers use case might still break even with this quick fix
datadog_lambda/tracing.py
Outdated
| @@ -411,6 +411,9 @@ def is_legacy_lambda_step_function(event): | |||
| """ | |||
| Check if the event is a step function that called a legacy lambda | |||
| """ | |||
| if not isinstance(event, dict): | |||
| return False | |||
|
|
|||
| event = event.get("Payload", {}) | |||
There was a problem hiding this comment.
This isn't super related to this incident, but I think it's worth changing anyway. I'm wondering if we can exit early if "Payload" is not in the event. By then we already know that it's not gonna be a step function event. So, exiting early would prevent the allocation of the {} and all the searching on the next line.
There was a problem hiding this comment.
Good callout 👍🏽 , will add an early exit
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
| if not isinstance(event, dict) or "Payload" not in event: | ||
| return False | ||
|
|
||
| event = event.get("Payload") |
There was a problem hiding this comment.
🟠 Code Quality Violation
variable name is the same as a function parameter (...read more)
A function parameter should only be read and not be modified. If your intent is to modify the value of the parameter, return the value in the function and handle the new value in the caller of the function.
|
/merge |
|
🚂 MergeQueue: waiting for PR to be ready This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals. Use |
dd-devflow
bot
added
mergequeue-status: waiting
mergequeue-status: queued
and removed
mergequeue-status: waiting
labels
|
🚂 MergeQueue: pull request added to the queue The median merge time in Use |
dd-devflow
bot
added
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
dd-devflow
bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
What does this PR do?
eventis a dictionary before trying to callget()on it as it may be a listCloses #532
Motivation
Testing Guidelines
Additional Notes
Types of Changes
Check all that apply