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

Print warning if traces merging from sfn to sfn is skipped #1447

Merged
merged 2 commits into from
Sep 16, 2024

Conversation

lym953
Copy link
Contributor

@lym953 lym953 commented Sep 12, 2024

Why?

Let's say Step Function A executes Step Function B. If B:

  1. doesn't have a Parameters field, or
  2. its Parameters.Input field is not a JSON object
  3. its Parameters.Input field has a custom CONTEXT.$ field

traces merging will be skipped, but no warning message will be printed.

What

Print a warning message in these cases to let users know what won't work.

Testing

1. Missing Parameters field

I wasn't able to test this because Parameters field is required when editing the State Machine from AWS Management Console.

Screenshot 2024-09-12 at 12 51 26 PM

2. Parameters.Input field is not a JSON object

Steps
  1. Change the Step Function execution step to be like:
    "Step Functions StartExecution": {
      "Type": "Task",
      "Resource": "arn:aws:states:::states:startExecution",
      "Parameters": {
        "StateMachineArn": "...",
        "Input": "Input!"
      },
      "End": true
    }
  1. Run datadog-ci stepfunctions instrument command
Result

A warning message is printed as expected:
image

3. custom CONTEXT.$

image

4. Happy case

datadog-ci stepfunctions instrument finishes with no error

Review checklist

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

@lym953 lym953 requested review from a team as code owners September 12, 2024 17:02
@lym953 lym953 added serverless Related to [lambda, stepfunctions, cloud-run] chores Related to the CI or developer experience labels Sep 12, 2024
@lym953 lym953 changed the title Print warning if traces merging from StepFunc to StepFunc is skipped Print warning if traces merging from xfn to xfn is skipped Sep 12, 2024
@datadog-datadog-prod-us1
Copy link

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

Datadog Report

Branch report: yiming.luo/fix-step-func-2
Commit report: 5ef1845
Test service: datadog-ci-tests

✅ 0 Failed, 364 Passed, 0 Skipped, 1m 26.6s Total duration (2m 1.5s time saved)

Base automatically changed from yiming.luo/fix-step-func to master September 13, 2024 18:05
Copy link
Contributor

@kimi-p kimi-p left a comment

Choose a reason for hiding this comment

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

lgtm

@lym953
Copy link
Contributor Author

lym953 commented Sep 13, 2024

I found a bug: if traces merging is already set up and instrumentation command is run again, the warning will also be printed, which is wrong. I will fix this in a separate PR.

@lym953 lym953 merged commit d27fd76 into master Sep 16, 2024
18 checks passed
@lym953 lym953 deleted the yiming.luo/fix-step-func-2 branch September 16, 2024 15:28
@lym953 lym953 changed the title Print warning if traces merging from xfn to xfn is skipped Print warning if traces merging from sfn to sfn is skipped Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chores Related to the CI or developer experience serverless Related to [lambda, stepfunctions, cloud-run]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants