S3 Downstream Span Pointers

[nhulston]

What does this PR do?

This is my first time writing Rust code, so focus on my syntax and other Rust concepts I may not understand yet.

Adds span pointers to spans for Lambdas triggered by S3 putObject, copyObject, and completeMultipartUpload events. This change will affect the downstream case for Universal Instrumentation Lambda runtimes (Java, .NET, Golang).

Span pointers are similar to Span Links, but for cases when it is impossible to pass the Trace ID and Span ID between the spans that need to be linked.

When the calculated hashes for the upstream and downstream lambdas match, the Datadog frontend will automatically link the two traces together.

Upstream case (requires tracer to be instrumented):
Downstream case (this code):

When clicking on the linked span, a new tab opens linking to the opposite Lambda function.

Describe how you validated your changes

Mostly manual testing, but I also added unit tests. There are also functional tests in https://github.com/DataDog/serverless-self-monitoring/pull/347

To verify my changes, run this Lambda function (only on the serverless AWS account) with the event payload

{
  "shouldPutObject": false,
  "shouldCopyObject": false,
  "shouldMultipartUpload": false
}

and change one of the bools to true. Check Datadog to ensure that the spans are properly linked.

I also added unit tests:

cargo test test_generate_span_pointer_hash
cargo test test_get_span_pointers
cargo test test_attach_span_pointers_to_span

Additional Notes

• This handles the 'downstream' case, for Lambdas that are triggered S3 requests. I will have another PR in each tracer to handle the 'upstream' case. Java PR here
• S3 and Dynamo are supported in Python+Node. I will later open another PR in this repo to add support for Dynamo.

[apiarian-datadog]

left some questions, mostly pertaining to some of the high level details. i'll have to leave the lower-level rust and style reviews to others.

[apiarian-datadog]

what happens if the span already has span links?

[nhulston]

I believe it just adds it to the array without overwriting it, but let me test and circle back!

[nhulston]

I stand corrected; this replaced existing span links. Thanks for catching this! I just added logic in the latest 2 commits to handle&test this case

[apiarian-datadog]

why wouldn't we handle span pointers for spans named something else?

[nhulston]

If we add span pointers to every (or multiple) spans, the Datadog UI will display that number of span links rather than just 1.

We could add it to any span in the trace; it doesn't have to be the aws.lambda span. It could also be the S3 inferred span as well (see next reply for more comments). But in Python/Node, we just add the span pointers the main aws.lambda span so I was just following that precedent.

It's definitely not optimal that we have to manually check the span name though. In Node we just get the lambda span from the wrappedCurrentSpan instance variable.
https://github.com/DataDog/datadog-lambda-js/blob/c726f30ba0d1dfa43b3b1a0fe025f4a80d14ad2a/src/trace/listener.ts#L204-L208

[nhulston]

Adding the span pointers to the S3 inferred span makes sense. But once we get to dynamo, there are different types of DynamoDB inferred spans. So I think it's better to just keep it simple and add the span pointers to the aws.lambda span since we know that it will always be there.

[apiarian-datadog]

can we add it to the top level span, whatever that is, rather than the span named with a magical "aws.lambda" string?

[nhulston]

The aws.lambda is the main span, and then Bottlecap creates the inferred S3 span. @duncanista do you know if there's a better way to determine if it's the top level span without having to manually check the string name?

[nhulston]

Okay, I've changed it so we get the span directly from the index; no need to loop through and check the span names.

Edit: this does add it to the inferred S3 span; which means I'll need to make some small changes to python + node lambda libraries for consistency

[apiarian-datadog]

they're always upstream?

[nhulston]

Yes, this code only runs when the Lambda was triggered by an S3 stream.

All the cases where ptr.dir is down will be handled by the tracers

[duncanista]

Left a couple comments

[duncanista]

Suggested change

	if let Ok(span_links_json) = serde_json::to_string(&span_links) {
	meta.insert("_dd.span_links".to_string(), span_links_json);
	}
	serde_json::to_string(&span_links)
	.map(|json| meta.insert("_dd.span_links".to_string(), json));

[duncanista]

why casting them and not just appending to them? What happens if you cannot cast?

wouldn't it be easier to just append to whatever is inside _dd.span_links? can't you just instead of replacing whatever is inside the json just add the new ones? not sure why we are casting the existing data here

[nhulston]

AFAIK we can't append without casting because meta.get("_dd.span_links") is a string.

One option could be to parse the string manually -- remove the last ], insert a comma if required, insert the new span link string, and readd the ]

wdyt?

[duncanista]

What if you parsed an array of strings and then appended your already serialized string?

[nhulston]

okay Jordan and I discussed this and this seems like the best way to do it. It's definitely not optimal, but parsing into the SpanLink struct rather than a Value does make the serde operation faster

[purple4reina]

This change seems important enough to me to warrant its own PR.

[nhulston]

cc @duncanista
He said he knows the issue and it's fine to keep it in this PR