Skip to content
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

Add DynamoDB Downstream Span Pointers #600

Merged
merged 9 commits into from
Dec 19, 2024

Conversation

nhulston
Copy link
Contributor

@nhulston nhulston commented Dec 18, 2024

What does this PR do?

Adds span pointers to spans for Lambdas triggered by DynamoDB INSERT, MODIFY, and REMOVE event streams. In practice, this covers DynamoDB updates caused by putItem, updateItem, deleteItem, transactWriteItems, and batchWriteItems requests.

This also updates the span pointer util imports that were modified in DataDog/dd-trace-js#4912.

Screenshot 2024-12-18 at 2 36 57 PM Screenshot 2024-12-18 at 2 38 24 PM Screenshot 2024-12-18 at 2 41 02 PM

Motivation

This feature already exists in Python, and I'm working on adding it to all other runtimes (Node, .NET, Java, Golang).

Testing Guidelines

Easy: Checkout this span enable the feature flag, and you will see that it's pointing to the downstream Lambda.

More thorough: Run this Lambda function with the event payload

{
  "shouldUseV2": false,
  "twoKeyTable": false,
  "shouldPutItem": false,
  "shouldUpdateItem": false,
  "shouldDeleteItem": false,
  "shouldBatchWriteItems": false,
  "shouldTransactWriteItems": false
}

As you can see, there are a lot of different cases to test -- v2 vs v3 AWS SDK, one primary key vs two primary key tables, and the 5 different types of requests. You can test by enabling one of the requests, running the Lambda, and then check Datadog to ensure that the spans are properly linked.

Additional Notes

  • This handles the 'downstream' case, for Lambdas that are triggered by DynamoDB updates. [serverless] Add DynamoDB Span Pointers dd-trace-js#4912 adds span pointers on the upstream case (for Lambdas that make the DynamoDB requests)
  • Tests will fail until dd-trace-js is released and we bump the version in datadog-lambda-js first.

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 -- todo, will add to self-monitoring
  • This PR collects user input/sensitive content into Datadog
  • This PR passes the integration tests (ask a Datadog member to run the tests)

src/utils/span-pointers.ts Show resolved Hide resolved
src/utils/span-pointers.spec.ts Show resolved Hide resolved
src/utils/span-pointers.ts Show resolved Hide resolved
src/utils/span-pointers.spec.ts Show resolved Hide resolved
@nhulston nhulston force-pushed the nicholas.hulston/dynamodb-span-pointers branch from f17702a to efa7bc3 Compare December 18, 2024 21:36
@nhulston nhulston marked this pull request as ready for review December 18, 2024 21:43
@nhulston nhulston requested a review from a team as a code owner December 18, 2024 21:43
@nhulston nhulston force-pushed the nicholas.hulston/dynamodb-span-pointers branch from 93b7c32 to 2e9c05c Compare December 19, 2024 00:23
@nhulston nhulston force-pushed the nicholas.hulston/dynamodb-span-pointers branch from c279f26 to 55aa3c0 Compare December 19, 2024 17:38
@nhulston nhulston merged commit 99c0b48 into main Dec 19, 2024
30 checks passed
@nhulston nhulston deleted the nicholas.hulston/dynamodb-span-pointers branch December 19, 2024 18:20

function getTableNameFromARN(arn: string): string | undefined {
// ARN format: arn:aws:dynamodb:<region>:<account-id>:table/<table-name>/stream/<YYYY-MM-DDThh:mm:ss.ms>
const match = arn.match(/table\/([^\/]*)/);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we be more defensive in this regex? or do we assume that this will always be an arn in this particular format?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, i don't see how the arn could be of a different format unless aws releases a breaking change. we should keep this in mind though and fix in the future if needed


const keys = record.dynamodb?.Keys;
const eventSourceARN = record.eventSourceARN;
const tableName = record.eventSourceARN ? getTableNameFromARN(eventSourceARN) : undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of curiosity: why not say const tableName = eventSourceARN ? ... here since we've already pulled the eventSourceARN out of the record in the line above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, will fix in #607

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants