nhinsch commented 3 years ago

What does this PR do?

Fix some tracing bugs:

Refactor the function that determines the root trace context. Most importantly, stop using the parent ID from X-Ray when MERGE_XRAY_TRACES is off. If MERGE_XRAY_TRACES is off, incoming X-Ray context should be ignored entirely.
Always mark the the function execution span for deletion by the Forwarder if there is a valid parent and MERGE_XRAY_TRACES is on. Previously, we would NOT drop the function execution span if the sourceType of the TraceContext was fromEvent. In practice, this meant that an extra span would appear if there was incoming Datadog trace context.
Remove the concept of a sourceType from the TraceContext altogether as it is no longer needed.
Tag spans with _dd.origin, not __dd.origin (typo). This metadata is used by the trace sampler.

Also:

Rename the GetTraceHeaders function to ConvertCurrentXrayTraceContext. This function is for Lambdas instrumented with the X-Ray SDK that want to add Datadog trace headers to outgoing requests, and does not take into account incoming Datadog trace context or Datadog spans at all. Refactor this function so that it is totally independent from the rest of the hybrid tracing logic. Remove an optional boolean argument useCurrentSegmentAsParent that was always true.

Other minor things:

Move the ConvertTraceContextToSpanContext function to context.go.
Add some more comments to clarify and document behaviors
Rename a bunch of functions (and some files) for improved clarity
Deploy the integration test Lambda in sa-east-1 instead of us-east-1

Motivation

There was a customer issue with an extra span appearing in their trace. This was an opportunity to more broadly revisit this feature to look for possible issues and tighten up the code.

Testing Guidelines

Manual testing and unit tests.

Types of changes

[x] Bug fix
[ ] New feature
[ ] Breaking change
[x] Misc (docs, refactoring, dependency upgrade, etc.)

Checklist

[x] This PR's description is comprehensive
[ ] This PR contains breaking changes that are documented in the description
[ ] This PR introduces new APIs or parameters that are documented and unlikely to change in the foreseeable future
[ ] This PR impacts documentation, and it has been updated (or a ticket has been logged)
[x] This PR's changes are covered by the automated tests
[ ] This PR collects user input/sensitive content into Datadog

codecov-io commented 3 years ago

Codecov Report

Merging #60 (dad4f2d) into main (8aafaf9) will decrease coverage by 1.84%. The diff coverage is 47.45%.

@@            Coverage Diff             @@
##             main      #60      +/-   ##
==========================================
- Coverage   82.56%   80.72%   -1.85%     
==========================================
  Files          12       12              
  Lines         608      607       -1     
==========================================
- Hits          502      490      -12     
- Misses         83       94      +11     
  Partials       23       23

Impacted Files	Coverage Δ
ddlambda.go	`80.00% <0.00%> (ø)`
internal/trace/listener.go	`59.18% <35.00%> (-2.22%)`	:arrow_down:
internal/trace/context.go	`75.00% <56.75%> (-12.16%)`	:arrow_down:
internal/metrics/processor.go	`96.87% <0.00%> (+3.12%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8aafaf9...dad4f2d. Read the comment docs.