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

Allow for legacy lambda step function steps #1504

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

avangelillo
Copy link
Contributor

@avangelillo avangelillo commented Dec 13, 2024

What and why?

From the JIRA:

A Step Function State Machine can invoke a Lambda function or a nested State Machine. The outer state machine, inner state machine and the Lambda function can all have their own traces.

To merge their traces (so that, for example, their flame graph can be combined), we need to alter the Payload field of the Lambda Invoke state and the Input field of the Step Function Execution state, so the necessary fields can be passed from the outer state machine into the inner state machine and the Lambda function. We sometimes call this “context injection”.

Previously, legacy definitions of a lambda step were explicitly denied. This PR allows context injection to happen on step functions using the legacy definitions for lambdas.

How?

There was already a check to not add context when the step function step was using the legacy definition. I removed that check and adjusted the validation that the step was for a lambda.

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
Screenshot 2024-12-16 at 11 17 07 AM

Commit

Allow context injection for legacy step function steps using lambda. Parameterized many of the existing unit tests to use a legacy and standard definition of the step to assert that the behavior for each of them is the same.

JIRA: https://datadoghq.atlassian.net/browse/SVLS-5638

Open questions I have

  1. Paramaterizing the tests to make all of the tests assert both formats work felt like a good idea at the start, but kind of seemed like overkill once I was more familiar with things. Should I keep them in or what tests do people think make sense?
  2. Are there any general PR things I missed when creating this?

@avangelillo avangelillo added the serverless Related to [lambda, stepfunctions, cloud-run] label Dec 13, 2024
@avangelillo avangelillo marked this pull request as ready for review December 13, 2024 18:38
@avangelillo avangelillo requested review from a team as code owners December 13, 2024 18:38
// not default lambda api
if (step.Resource !== 'arn:aws:states:::lambda:invoke') {
// not default lambda api or legacy lambda definition
if (step.Resource !== 'arn:aws:states:::lambda:invoke' && !step.Resource?.startsWith('arn:aws:lambda')) {

Choose a reason for hiding this comment

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

There's also the callback invocation pattern for invoking the lambda which probably still won't work (If you're thinking of handling that in a separate PR, that's fine too). This is a case where the value for step.Resource will be arn:aws:states:::lambda:invoke.waitForTaskToken

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's good to know, I hadn't heard of that before. That won't work with this, I'll adjust this PR

Allow context injection for legacy step function steps using lambda.
Parameterized many of the existing unit tests to use a legacy and standard
definition of the step to assert that the behavior for each of them is the
same.

JIRA: https://datadoghq.atlassian.net/browse/SVLS-5638
@avangelillo avangelillo force-pushed the alex-angelillo/legacy-stepfunctions branch from f4c3cba to a4e2cc9 Compare December 16, 2024 17:36
@datadog-datadog-prod-us1
Copy link

Datadog Report

Branch report: alex-angelillo/legacy-stepfunctions
Commit report: a4e2cc9
Test service: datadog-ci-tests

✅ 0 Failed, 664 Passed, 0 Skipped, 1m 58.07s Total duration (1m 55.8s time saved)

Copy link

@clifordshelton clifordshelton left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@clifordshelton
Copy link

Open questions I have

  1. Paramaterizing the tests to make all of the tests assert both formats work felt like a good idea at the start, but kind of seemed like overkill once I was more familiar with things. Should I keep them in or what tests do people think make sense?
  2. Are there any general PR things I missed when creating this?
  1. The tests you've added seem fine. As long as we're having some test coverage, whatever setup makes our lives as developers easier is fine. If at some time we hit the point where parameterizing the tests will save us time and effort, we can switch over.
  2. This PR looks great. I don't think there's any other general things that're missing 👍

Copy link
Contributor

@Drarig29 Drarig29 left a comment

Choose a reason for hiding this comment

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

LGTM

@avangelillo avangelillo merged commit 1a95e51 into master Dec 17, 2024
19 checks passed
@avangelillo avangelillo deleted the alex-angelillo/legacy-stepfunctions branch December 17, 2024 16:32
@amaanq amaanq mentioned this pull request Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
serverless Related to [lambda, stepfunctions, cloud-run]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants