Inferred Spans #254

astuyve · 2021-12-21T16:40:07Z

What does this PR do?

Creates inferred spans for:

APIGW v1
APIGW HTTPAPI v2
APIGW Websocket v2
DynamoDB stream
EventBridge events
S3 events
SNS
SQS
SNS + SQS
Kinesis stream events

Motivation

Testing Guidelines

Additional Notes

Types of Changes

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

Check all that apply

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)
This PR's changes are covered by the automated tests
This PR collects user input/sensitive content into Datadog
This PR passes the integration tests (ask a Datadog member to run the tests)

…method. Refactor to satisfy law of demeter.

…PI-like inferred spans

… ending all inferred spans in invocationDone by memoizing lambda span and using its start time for the end time of async inferred spans

…nd return headers. Set inferred span attrs so service names are not overwritten. Update tests, remove dead code

…an for snssqs events. Specs fo snssqs events

codecov-commenter · 2021-12-21T16:41:26Z

Codecov Report

Merging #254 (df030e1) into main (0031257) will decrease coverage by 1.30%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##             main     #254      +/-   ##
==========================================
- Coverage   83.56%   82.26%   -1.31%     
==========================================
  Files          35       37       +2     
  Lines        1339     1618     +279     
  Branches      284      349      +65     
==========================================
+ Hits         1119     1331     +212     
- Misses        196      238      +42     
- Partials       24       49      +25

Impacted Files	Coverage Δ
src/trace/tracer-wrapper.ts	`86.48% <25.00%> (-7.46%)`	⬇️
src/trace/listener.ts	`75.55% <45.45%> (-18.82%)`	⬇️
src/trace/span-wrapper.ts	`64.28% <64.28%> (ø)`
src/trace/context.ts	`85.03% <69.13%> (-4.74%)`	⬇️
src/trace/span-inferrer.ts	`85.62% <85.62%> (ø)`
src/trace/trigger.ts	`87.32% <90.90%> (-0.49%)`	⬇️
src/index.ts	`87.15% <100.00%> (+5.41%)`	⬆️
src/trace/trace-context-service.ts	`86.66% <100.00%> (-5.65%)`	⬇️
src/utils/event-type-guards.ts	`100.00% <100.00%> (ø)`
... and 1 more

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 0031257...df030e1. Read the comment docs.

zARODz11z · 2022-01-11T16:24:34Z

src/trace/context.ts

@@ -238,6 +238,89 @@ export function readTraceFromSQSEvent(event: SQSEvent): TraceContext | undefined
  return;
 }

+export function readTraceFromSNSSQSEvent(event: SQSEvent): TraceContext | undefined {


Out of curiosity, in dd-lambda-js do we have multiple readTraceFrom...Event functions that are specifically tailored for each aws managed service?
If so then why do we have a readTraceFromSQSEvent function and readTraceFromSNSSQSEvent (line 241)?

Is an SQS event and SNSSQS event different? Therefore needing two separate functions?

Yeah they are a bit different, basically the SNSSQS event is not batched, but a typical SNS event has an array of Records with each SNS message inside it. I think we could probably refactor this though, going to take a closer look

agocs · 2022-01-20T21:26:37Z

src/trace/context.ts

+export function readTraceFromEventbridgeEvent(event: EventBridgeEvent<any, any>): TraceContext | undefined {
+  if (event.detail && event.detail._datadog) {
+    try {
+      const traceData = event.detail._datadog;


I don't think we anticipate having a _datadog key in the event.detail. The way it was implemented in dd-trace-py, we just have x-datadog-trace-id, etc. We could change it either way, but it's important we're consistent.

Ah I see, I'd been following the convention for sns/sqs where we stick a _datadog string map in the message attributes, but I can change it to this. But yes, we need stay consistent here.

agocs · 2022-01-20T21:31:58Z

src/trace/context.ts

+  if (isSNSEvent(event)) {
+    return readTraceFromSNSEvent(event);
+  }
+  // TODO [astuyve] SNSSQS must be before SQS


You might want to move isSNSSQSEvent in this case

Do you know if tests hit this section of code? It might be important to ensure we get it right and keep getting it right in the future.

I'll add better tests for this to ensure it's hit.

We should remove this TODO since it looks like isSNSSQSEvent is being hit before isSQSEvent and just leave the comment.

…ontaining trace context

hghotra · 2022-02-02T17:16:47Z

src/index.ts

 } from "./utils";

 export { TraceHeaders } from "./trace";

 export const apiKeyEnvVar = "DD_API_KEY";
 export const apiKeyKMSEnvVar = "DD_KMS_API_KEY";
 export const captureLambdaPayloadEnvVar = "DD_CAPTURE_LAMBDA_PAYLOAD";
+export const traceManagedServicesEnvVar = "DD_TRACE_MANAGED_SERVICES";


Can we update the README with some information about this env var.

hghotra · 2022-02-02T17:38:36Z

src/trace/context.ts

+        parsedBody.MessageAttributes &&
+        parsedBody.MessageAttributes._datadog &&
+        parsedBody.MessageAttributes._datadog.Value


Nit: conditions like these can be simplified to: parsedBody?.MessageAttributes?._datadog?.Value

hghotra · 2022-02-02T17:49:54Z

src/trace/span-inferrer.spec.ts

+const sqsEvent = require("../../event_samples/sqs.json");
+const ddbEvent = require("../../event_samples/dynamodb.json");
+const kinesisEvent = require("../../event_samples/kinesis.json");
+const eventBridgeEvent = require("../../event_samples/event-bridge.json");


Nit: can we rename this to eventbridge, the - prevents it from showing up in search.

hghotra · 2022-02-02T17:51:18Z

src/trace/context.ts

+        if (typeof traceID !== "string") {
+          return;
+        }
+        const parentID = traceData[parentIDHeader];
+        if (typeof parentID !== "string") {
+          return;
+        }
+        const sampledHeader = traceData[samplingPriorityHeader];
+        if (typeof sampledHeader !== "string") {
+          return;
+        }


Nit: can we do a single type check for all three variables instead of three separate conditions?

hghotra · 2022-02-02T18:24:57Z

src/trace/listener.ts

+      // TODO [astuyve] discuss with team RE nulls or undefined, or a coalescing strategy
+      parentSpanContext = rootTraceHeaders ? this.tracerWrapper.extract(rootTraceHeaders) || undefined : undefined;


Suggested change

// TODO [astuyve] discuss with team RE nulls or undefined, or a coalescing strategy

parentSpanContext = rootTraceHeaders ? this.tracerWrapper.extract(rootTraceHeaders) || undefined : undefined;

parentSpanContext = rootTraceHeaders ? this.tracerWrapper.extract(rootTraceHeaders) ?? undefined : undefined;

hghotra · 2022-02-02T18:45:56Z

src/trace/span-inferrer.ts

+      "span.type": "http",
+      "resource.name": resourceName,
+      "service.name": domain,
+      api_id: event.requestContext.apiId,


Can we rename this to apiid? This is done since the metric dimension is apiid and we want to have an attribute that's the same name as the metric tag. This ensure that the attribute is there when pivoting from a metric to a trace query.

Suggested change

api_id: event.requestContext.apiId,

apiid: event.requestContext.apiId,

apiname: event.requestContext.apiId,

hghotra · 2022-02-02T18:53:01Z

src/trace/span-inferrer.ts

+    const resourceName = `${eventName} ${tableName}`;
+    options.tags = {
+      operation_name: "aws.dynamodb",
+      "aws.dynamodb.table_name": tableName,


Suggested change

"aws.dynamodb.table_name": tableName,

"tablename": tableName,

hghotra · 2022-02-02T18:57:59Z

src/trace/span-inferrer.ts

+      type: Type,
+      subject: Subject,
+      message_id: MessageId,
+      topic_name: topicName,


Suggested change

topic_name: topicName,

topicname: topicName,

hghotra · 2022-02-02T18:58:35Z

src/trace/span-inferrer.ts

+      type: Type,
+      subject: Subject,
+      message_id: MessageId,
+      topic_name: topicName,


Suggested change

topic_name: topicName,

topicname: topicName,

… span attribute names for topicname, tablename, etc.

hghotra · 2022-02-04T14:27:51Z

src/trace/context.ts

@@ -238,6 +237,136 @@ export function readTraceFromSQSEvent(event: SQSEvent): TraceContext | undefined
  return;
 }

+export function readTraceFromSNSSQSEvent(event: SQSEvent): TraceContext | undefined {
+  if (event?.Records[0]?.body) {


It's very unlikely that Records here will be null, but the proper syntax is the weird looking event?.Records?.[0]?.body.

const testEvent1 = {Records: [{body: "Hello"}]} console.log("testEvent1?.Records?.[0]?.body:", testEvent1?.Records?.[0]?.body) console.log("testEvent1?.Records[0]?.body:", testEvent1?.Records[0]?.body) const testEvent2 = {Records: []} console.log("testEvent2?.Records?.[0]?.body:", testEvent2?.Records?.[0]?.body) console.log("testEvent2?.Records[0]?.body:", testEvent2?.Records[0]?.body) const testEvent3 = {Records: null} console.log("testEvent3?.Records?.[0]?.body:", testEvent3?.Records?.[0]?.body) console.log("testEvent3?.Records[0]?.body:", testEvent3?.Records[0]?.body)

/usr/local/lib/node_modules/ts-node-fm/src/index.ts:226 testEvent1?.Records?.[0]?.body: Hello testEvent1?.Records[0]?.body: Hello testEvent2?.Records?.[0]?.body: undefined testEvent2?.Records[0]?.body: undefined testEvent3?.Records?.[0]?.body: undefined TypeError: Cannot read property '0' of null at index.ts:10:126 at Script.runInThisContext (vm.js:122:20) at startRepl (/usr/local/lib/node_modules/ts-node-fm/src/bin.ts:157:12) at Object.<anonymous> (/usr/local/lib/node_modules/ts-node-fm/src/bin.ts:66:1) at Module._compile (internal/modules/cjs/loader.js:778:30) at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10) at Module.load (internal/modules/cjs/loader.js:653:32) at tryModuleLoad (internal/modules/cjs/loader.js:593:12) at Function.Module._load (internal/modules/cjs/loader.js:585:3) at Function.Module.runMain (internal/modules/cjs/loader.js:831:12)

Yeah... But it's ugly. I'll use it :)

hghotra

💯

astuyve added 16 commits November 22, 2021 11:39

feat: stub method for cold starts

639ca15

[WIP] feat: span wrapper to encapsulate metadata for inferred spans

279b546

refactor: Encapsulate trace listener logic in new onEndingInvocation …

342bcbc

…method. Refactor to satisfy law of demeter.

feat: revert private trigger tags

fb2b144

Merge branch 'main' into aj/feat-support-coldstart-spans

5fa4aff

feat: Fix bug where inferred spans weren't ending.

a177154

feat: set service as API URL and resource name as METHOD + PATH for A…

0ffd0bb

…PI-like inferred spans

feat: Add kinesis, DDB Stream, S3, SNS, SQS inferred spans. Switch to…

4864467

… ending all inferred spans in invocationDone by memoizing lambda span and using its start time for the end time of async inferred spans

WIP: sqs trace extraction not working

e962565

feat: refactor rootTraceContext to be private, create method to set a…

b66689f

…nd return headers. Set inferred span attrs so service names are not overwritten. Update tests, remove dead code

feat: support snssqs events

78fa165

Merge branch 'main' into aj/feat-support-coldstart-spans

ee16450

feat: properly re-merge with main

1e0080b

feat: Don't add parent context unless it's present. Fix parent sns sp…

aff08fc

…an for snssqs events. Specs fo snssqs events

test: sns, sqs, ddb tests. Kinesis too, but not on yet.

e225751

feat: kinesis inferred spans, and tests.

1277cfa

astuyve requested a review from a team as a code owner December 21, 2021 16:40

astuyve changed the title ~~Aj/feat support coldstart spans~~ Inferred Spans Dec 21, 2021

astuyve added 4 commits December 21, 2021 11:23

feat: cleanup and lint

d587c3e

Merge branch 'main' into aj/feat-support-coldstart-spans

c7a1712

cleanup: remove util

6a1edaf

feat: bump specs

8b5af7e

zARODz11z reviewed Jan 11, 2022

View reviewed changes

astuyve added 6 commits January 14, 2022 11:56

fix: Fix capturing lambda payloads

b6e9edf

feat: s3 inferred span, kinesis inferred span plus extractor

f87f8e9

tests: Fix timestamp so that it's millis and not seconds

ec27408

feat: kinesis, eventbridge, and websockets

e4d32fc

tests: update specs to use snake case instead of camel case

ef0e0ac

feat: lint

08e9fab

revert: remove lambda function URLs

ed09f4f

astuyve requested review from agocs and hghotra January 20, 2022 18:28

astuyve added 2 commits January 20, 2022 12:31

Merge branch 'main' into aj/feat-support-coldstart-spans

dffbd41

specs: Update integration tests

827bfe4

agocs reviewed Jan 20, 2022

View reviewed changes

astuyve added 7 commits January 24, 2022 10:07

feat: Context specs. Replace inferred span test events with samples c…

038417b

…ontaining trace context

feat: Standardize event bridge with python

61d5b65

feat: revert eventbridge changes, go with the _datadog wrapper

2978a56

feat: Env variable to disable inferred spans

2b1fa62

specs: Update default config to include createInferredSpan

35e117f

feat: Add API Id

d4785d5

feat: Update snapshots

1e2c614

astuyve requested a review from zARODz11z February 1, 2022 18:43

hghotra reviewed Feb 2, 2022

View reviewed changes

feat: CR feedback. Refactor conditionals to use null coalescence. Fix…

4da8b76

… span attribute names for topicname, tablename, etc.

hghotra reviewed Feb 4, 2022

View reviewed changes

feat: Use nullish coalesence, even for array indexing

df030e1

hghotra approved these changes Feb 4, 2022

View reviewed changes

astuyve merged commit a44b34a into main Feb 7, 2022

astuyve deleted the aj/feat-support-coldstart-spans branch February 7, 2022 19:23

		// TODO [astuyve] discuss with team RE nulls or undefined, or a coalescing strategy
		parentSpanContext = rootTraceHeaders ? this.tracerWrapper.extract(rootTraceHeaders) \|\| undefined : undefined;

	api_id: event.requestContext.apiId,
	apiid: event.requestContext.apiId,
	apiname: event.requestContext.apiId,

	"aws.dynamodb.table_name": tableName,
	"tablename": tableName,

Inferred Spans #254

Inferred Spans #254

Uh oh!

Conversation

astuyve commented Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Testing Guidelines

Additional Notes

Types of Changes

Check all that apply

Uh oh!

codecov-commenter commented Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agocs Jan 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hghotra Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hghotra Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hghotra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

astuyve commented Dec 21, 2021 •

edited

Loading

codecov-commenter commented Dec 21, 2021 •

edited

Loading

agocs Jan 20, 2022 •

edited

Loading

hghotra Feb 2, 2022 •

edited

Loading

hghotra Feb 2, 2022 •

edited

Loading