Explicit trace ID propagation for SFN #526
Conversation
avedmala
changed the title
There was a problem hiding this comment.
Can you test with SFN -> Lambda with payload of _datadog and see if these two spans will link? You might need to build the python layer and use it. Thanks!
This is the command that joey shared to me when I was building py layer
# python
docker buildx build -t datadog-lambda-python-amd64:3.9 . --no-cache \
--build-arg image=python:3.9 \
--build-arg runtime=python3.9 \
--platform linux/amd64 \
--progress=plain \
-o ./.layers/tmp/python && \
pushd ./.layers/tmp && zip -q -r datadog_lambda_py-amd64-3.9.zip ./ && \
sso_sand_run aws lambda publish-layer-version \
--layer-name "Datadog-Python39" \
--region sa-east-1 \
--zip-file "fileb://datadog_lambda_py-amd64-3.9.zip" && popd
joeyzhao2018
left a comment
joeyzhao2018
left a comment
There was a problem hiding this comment.
I'm not convinced that this is any exception for propagator.extract.
I'll update my proposed changes later.
|
|
||
| def _deterministic_sha256_hash(s: str, part: str) -> (int, int): | ||
| sha256_hash = hashlib.sha256(s.encode()).hexdigest() | ||
| return _sha256_to_binary_part(hashlib.sha256(s.encode()).hexdigest(), part) |
There was a problem hiding this comment.
Split this up with a helper so we have an entrypoint for our sha256_hash from the x-datadog-trace-id-hash and x-datadog-parent-id-hash
| dd_data.get("x-datadog-trace-id-hash"), HIGHER_64_BITS | ||
| ) | ||
| )[2:] | ||
| else: |
There was a problem hiding this comment.
This whole branch should be the old behavior
kimi-p
left a comment
kimi-p
left a comment
There was a problem hiding this comment.
LGTM. Let's test this change with reducer change in staging before merge it.
| if "_datadog" in event: | ||
| dd_data = event.get("_datadog") | ||
| parent_id = _sha256_to_binary_part( | ||
| dd_data.get("x-datadog-parent-id-hash"), HIGHER_64_BITS |
There was a problem hiding this comment.
Note: Since this extract_context_from_step_functions() is executed when we know _datadog.serverless-version=v2, we don't need to check if this hash key exists. The v2 contract is that they need to have the _datadog.x-datadog-parent-id-hash.
There was a problem hiding this comment.
I might be misunderstanding, we're not checking if the hash key exists right?
We're computing the parent_id no matter what, the only thing we check in this branch is if x-datadog-trace-id exists
There was a problem hiding this comment.
So we're saying it's the same EventTypes.STEP_FUNCTIONS whether its the old format with context object or new one with _datadog. Both cases will enter extract_context_from_step_functions()
If we want to separate them, one thing we could do is make a new V2 event with a V2 extractor to go along with it
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
| else: # sfn root | ||
| trace_id = _sha256_to_binary_part( | ||
| dd_data.get("x-datadog-trace-id-hash"), LOWER_64_BITS | ||
| ) | ||
| meta["_dd.p.tid"] = hex( | ||
| _sha256_to_binary_part( | ||
| dd_data.get("x-datadog-trace-id-hash"), HIGHER_64_BITS | ||
| ) | ||
| )[2:] |
There was a problem hiding this comment.
⚪ Code Quality Violation
else is not necessary since the if clause has a return (...read more)
If the code in the if branch returns a value, do not have the else branch present.
| 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.
What does this PR do?
Add logic to extract trace context from
_datadogfor Step Functions cases where...x-datadog-trace-idbut we need to compute thex-datadog-parent-idusingx-datadog-parent-id-hashx-datadog-trace-idandx-datadog-parent-idfrom their respective hashes_datadogand we instead figure out context from the Execution and State infoMotivation
Testing Guidelines
Additional Notes
Types of Changes
Check all that apply