-
Notifications
You must be signed in to change notification settings - Fork 37
Propagate Step Function Trace Context through Managed Services #667
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
… header and SFN context" This reverts commit a146e03.
Run code formatting
…datadog-lambda-js into ryan.strat/sfn-managed-services
c5130af
to
a813a29
Compare
.gitlab/scripts/publish_layers.sh
Outdated
@@ -87,6 +87,12 @@ if [[ ! ${STAGES[@]} =~ $STAGE ]]; then | |||
fi | |||
|
|||
layer="${LAYERS[$index]}" | |||
if [ -z "$LAYER_NAME_SUFFIX" ]; then |
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.
where is the variable LAYER_NAME_SUFFIX set? is this mainly for testing in development?
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 not, I added it as a local config option to publish private versions. This is similar to an option available in the python layer.
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.
mmm, not sure about this change, how are we sure this is not going to affect gitlab pipelines?
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.
Removing this edit based on feedback from @duncanista
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.
Good job refactoring the code
|
||
it("extracts trace context from Step Function EventBridge event", () => { | ||
// Reset StepFunctionContextService instance | ||
StepFunctionContextService["_instance"] = undefined as any; |
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.
Shouldn't this be done before every step?
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 reset is only required for tests that use the Step Function Context Service. It could be added in a BeforeEach for all tests, but I don't think it would add value.
Co-authored-by: jordan gonzález <[email protected]>
Changes to CI should not be made in the same PR as adding features
/merge |
View all feedbacks in Devflow UI.
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.
The expected merge time in
|
What does this PR do?
This PR adds support for extracting step function trace context in the following cases:
Motivation
This PR adds feature parity to the JS libraries to match functionality in the python tracing library: DataDog/datadog-lambda-python#573
Testing Guidelines
I published a private layer to the ddserverless account and verified that all cases produced the intended traces. The resulting traces are below.
Additional Notes
Minor update to the publish script follows a patter from other layers to allow adding a suffix for test layer releases.
Documentation PR: DataDog/documentation#30715
Types of Changes
Check all that apply