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

Skip sfn->Lambda context injection if custom Payload is used without $ #1450

Merged
merged 1 commit into from
Sep 16, 2024

Conversation

lym953
Copy link
Contributor

@lym953 lym953 commented Sep 12, 2024

Problem

If a Lambda function in a State Machine has a custom Payload defined using "Payload": ..., e.g.,

    "Lambda Invoke": {
      "Type": "Task",
      "Resource": "arn:aws:states:::lambda:invoke",
      "OutputPath": "$.Payload",
      "Parameters": {
        "FunctionName": "...",
        "Payload": {
          "action": "service/delete_customer"
        }
      },
      ...
    }
  }

then context injection for the Step Function will fail with the error:

[Error] Invalid State Machine Definition: 'SCHEMA_VALIDATION_FAILED: Cannot have both the field 'Payload.$' and the field 'Payload' at the same time at /States/Lambda Invoke/Parameters'. Failed to inject context into lambda functions' payload of arn:aws:states:sa-east-1:425362996713:stateMachine:YimingTestExpressStateMachine2

This is because our instrumentation code will add a field "Payload.$": ..., which duplicates with "Payload": .... Right now we only check for custom "Payload.$": ... (with $, i.e. using JSONPath) but don't check for custom "Payload": ....

What

Also skip context injection when a custom "Payload": ... field is used (without $)

Testing

Automated testing

Pass the added test, which would fail without the changes on helpers.ts

Manual testing

Steps:

  1. change the Payload field of a Lambda function to what's shown in "Problem" section
  2. run datadog-ci stepfunctions instrument

Result: a warning is printed as expected
image

Notes

  1. Thanks @agocs for bringing this up in [lambda] Print warning if traces merging is skipped for Lambda in Step Function #1443 (review)
  2. The behavior will be changed soon: we will soon allow custom Payload field as long as it doesn't contain Execution or State field inside. But I want to first merge this PR to make the existing behavior more clear, so the changes in future PRs will be more clear so the PRs will be easier to review.

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)

@lym953 lym953 requested review from a team as code owners September 12, 2024 19:29
@lym953 lym953 added serverless Related to [lambda, stepfunctions, cloud-run] chores Related to the CI or developer experience labels Sep 12, 2024
@lym953 lym953 force-pushed the yiming.luo/fix-step-func-2 branch 2 times, most recently from 4d23aaa to 5ef1845 Compare September 12, 2024 20:05
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Sep 12, 2024

Datadog Report

Branch report: yiming.luo/fix-step-func-3
Commit report: e9c812d
Test service: datadog-ci-tests

✅ 0 Failed, 372 Passed, 0 Skipped, 1m 29.08s Total duration (1m 58.6s time saved)

@lym953 lym953 added enhancement New feature or request and removed chores Related to the CI or developer experience labels Sep 12, 2024
@nine5two7
Copy link
Contributor

Also skip context injection when a custom "Payload": ... field is used (without $)
For customized payload, we can still explicitly inject

           "Execution.$": "$$.Execution",
          "State.$": "$$.State"

Will there be another PR for such handling?

@lym953
Copy link
Contributor Author

lym953 commented Sep 13, 2024

Will there be another PR for such handling?

@nine5two7 You mean the injection thing? Yes I'll do it in separate PRs. All the PRs so far are small fixes, either printing warnings or making error messages more clear.

Base automatically changed from yiming.luo/fix-step-func-2 to master September 16, 2024 15:28
Copy link
Contributor

@avedmala avedmala left a comment

Choose a reason for hiding this comment

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

good fix! 🚢

@lym953 lym953 merged commit 3644356 into master Sep 16, 2024
18 checks passed
@lym953 lym953 deleted the yiming.luo/fix-step-func-3 branch September 16, 2024 17:47
@lym953 lym953 changed the title Skip xfn->Lambda context injection if custom Payload is used without $ Skip sfn->Lambda context injection if custom Payload is used without $ Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request serverless Related to [lambda, stepfunctions, cloud-run]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants