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 S3 Downstream Span Pointers #587

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

nhulston
Copy link
Contributor

@nhulston nhulston commented Nov 14, 2024

What does this PR do?

Adds span pointers to spans for Lambdas triggered by putObject, copyObject, and completeMultipartUpload events.

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 backend will automatically link the two traces together.

Screenshot 2024-11-11 at 1 29 11 PM When clicking on the linked span, a new tab opens linking to the downstream Lambda function that was triggered by this S3 object update.

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

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

and change one of the bools to true. Also, try both with AWS SDK v2 and v3. Enable the span pointers feature flag, and check Datadog to ensure that the spans are properly linked.

Additional Notes

  • This handles the 'downstream' case, for Lambdas that make S3 requests. This PR adds span pointers on the upstream case.
  • S3 and Dynamo are supported in Python. I will soon open another PR to add support for Dynamo.

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)

src/utils/span-pointers.ts Outdated Show resolved Hide resolved
src/utils/span-pointers.ts Show resolved Hide resolved
src/trace/trigger.ts Show resolved Hide resolved
@nhulston nhulston force-pushed the nicholas.hulston/s3-span-pointers branch from d0a666e to c8aa5a8 Compare November 14, 2024 17:45
@nhulston nhulston force-pushed the nicholas.hulston/s3-span-pointers branch from c8aa5a8 to a3b587b Compare November 14, 2024 17:55
@nhulston nhulston marked this pull request as ready for review November 14, 2024 18:46
@nhulston nhulston requested a review from a team as a code owner November 14, 2024 18:46
Copy link
Contributor

@apiarian-datadog apiarian-datadog left a comment

Choose a reason for hiding this comment

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

the span pointer stuff looks fine, though some aspects of the api are a little strange. we may want someone with more js experience to take a look as well.

src/utils/span-pointers.ts Outdated Show resolved Hide resolved
src/utils/span-pointers.ts Outdated Show resolved Hide resolved
src/utils/span-pointers.ts Outdated Show resolved Hide resolved
src/trace/listener.ts Outdated Show resolved Hide resolved
src/trace/listener.ts Outdated Show resolved Hide resolved
src/trace/listener.ts Outdated Show resolved Hide resolved
@nhulston nhulston force-pushed the nicholas.hulston/s3-span-pointers branch from c9e623b to ea5594b Compare November 15, 2024 16:23
@nhulston nhulston marked this pull request as draft November 15, 2024 21:50
@nhulston nhulston force-pushed the nicholas.hulston/s3-span-pointers branch from 2309fdd to 1740176 Compare November 18, 2024 22:05
@nhulston nhulston force-pushed the nicholas.hulston/s3-span-pointers branch from 1740176 to 340ac84 Compare November 18, 2024 22:06
@nhulston nhulston marked this pull request as ready for review November 22, 2024 20:37
@nhulston nhulston force-pushed the nicholas.hulston/s3-span-pointers branch from 5ffcfdb to 0715e81 Compare November 22, 2024 20:41
const mockPointerHash = "mock-hash-123";

beforeEach(() => {
jest.spyOn(util, "generatePointerHash").mockReturnValue(mockPointerHash);
Copy link
Contributor

Choose a reason for hiding this comment

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

why not let the generate pointer hash function actually run?

Copy link
Contributor Author

@nhulston nhulston Nov 22, 2024

Choose a reason for hiding this comment

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

I already have tests for generatePointerHash in the tracer; i just wanted to test only the logic behind the functions created in this PR. That way, if we ever make changes to how the generatePointerHash function works in the tracer, it won't break the tests here. And the changes will be automatically propagated to here, requiring no code updates.

src/trace/listener.ts Outdated Show resolved Hide resolved
let util;
try {
constants = require("dd-trace/packages/dd-trace/src/constants");
util = require("dd-trace/packages/dd-trace/src/util");
Copy link
Contributor

Choose a reason for hiding this comment

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

@duncanista, can you help answer a question for me?

I recall when you added support for w3c trace propagation to datadog-lambda-js, you said that instead of directly importing the tracer from dd-trace, we create an interface to wrap it.

If that's correct, then is there any issue with us requiring other files from ddtrace here?

Copy link
Contributor

Choose a reason for hiding this comment

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

There might be an issue when customer is not using the tracer (some do it), it would essentially crash, ideally, we'd have a better way to get this type of data

Copy link
Contributor Author

@nhulston nhulston Nov 26, 2024

Choose a reason for hiding this comment

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

@duncanista Could you take another look at the updated code from my latest commit? It should be safe and avoid any crashing:

  let S3_PTR_KIND;
  let SPAN_POINTER_DIRECTION;
  let generatePointerHash;
  try {
    const constants = require("dd-trace/packages/dd-trace/src/constants");
    const util = require("dd-trace/packages/dd-trace/src/util");

    ({ S3_PTR_KIND, SPAN_POINTER_DIRECTION } = constants);
    ({ generatePointerHash } = util);
  } catch (err) {
    if (err instanceof Error) {
      logDebug("Failed to load dd-trace span pointer dependencies", err);
    }
    return spanPointerAttributesList;
  }

Copy link
Contributor

Choose a reason for hiding this comment

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

@duncanista, Can you give more details (or point me to a doc somewhere) about how "customer is not using the tracer" is implemented? Do you mean they just aren't including ddtrace in their package.json file? I'd like to be able to create a small sample app that I can test this PR with that "is not using the tracer".

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah correct, just the lambda library, not the tracer. Let me review the whole PR

src/trace/listener.ts Outdated Show resolved Hide resolved
src/trace/listener.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@duncanista duncanista left a comment

Choose a reason for hiding this comment

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

This should be fine – I'm approving to unblock, but this is not ideal on how we are importing it. I guess its fail-safe, yet this is not making it friendly to eventually make this package ESBuild compatible

@duncanista
Copy link
Contributor

Please add integration tests for this, due to how you are requiring this packages from the external package

@nhulston nhulston force-pushed the nicholas.hulston/s3-span-pointers branch from 6fab017 to 44b737a Compare November 26, 2024 22:52
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.

4 participants