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 support for exit spans in Code Origin for Spans #4772

Merged
merged 3 commits into from
Oct 30, 2024

Conversation

watson
Copy link
Collaborator

@watson watson commented Oct 11, 2024

What does this PR do?

Note: This PR relates to an upcoming Datadog feature that's not generally available yet.

Support for Code Origin for Spans was added in #4449. That PR laid the groundwork for supporting this new feature and added limited support for just Fastify entry-spans. This PR adds to that original PR by adding support for all exit-spans. That is, spans that record a request going out to a different service, e.g. a database request, an outgoing HTTP request etc.

To enable, set DD_CODE_ORIGIN_FOR_SPANS_ENABLED=true.

Copy link
Collaborator Author

watson commented Oct 11, 2024

Copy link

github-actions bot commented Oct 11, 2024

Overall package size

Self size: 7.64 MB
Deduped: 64.5 MB
No deduping: 64.84 MB

Dependency sizes | name | version | self size | total size | |------|---------|-----------|------------| | @datadog/native-appsec | 8.2.1 | 19.18 MB | 19.19 MB | | @datadog/native-iast-taint-tracking | 3.2.0 | 13.9 MB | 13.91 MB | | @datadog/pprof | 5.4.1 | 9.76 MB | 10.13 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.5.0 | 2.51 MB | 2.65 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 2.0.0 | 898.77 kB | 1.3 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.11.2 | 112.74 kB | 826.22 kB | | msgpack-lite | 0.1.26 | 201.16 kB | 281.59 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | lru-cache | 7.18.3 | 133.92 kB | 133.92 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.0 | 109.9 kB | 109.9 kB | | semver | 7.6.3 | 95.82 kB | 95.82 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 5.3.1 | 51.46 kB | 51.46 kB | | int64-buffer | 0.1.10 | 49.18 kB | 49.18 kB | | shell-quote | 1.8.1 | 44.96 kB | 44.96 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.3.1 | 25.21 kB | 25.21 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | dc-polyfill | 0.1.4 | 23.1 kB | 23.1 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | path-to-regexp | 0.1.10 | 6.38 kB | 6.38 kB | | module-details-from-path | 1.0.3 | 4.47 kB | 4.47 kB |

🤖 This report was automatically generated by heaviest-objects-in-the-universe

Copy link

codecov bot commented Oct 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.04%. Comparing base (7f93d36) to head (d60ee21).
Report is 10 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #4772       +/-   ##
===========================================
+ Coverage   69.19%   92.04%   +22.85%     
===========================================
  Files           1        6        +5     
  Lines         198      327      +129     
  Branches       33        0       -33     
===========================================
+ Hits          137      301      +164     
+ Misses         61       26       -35     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pr-commenter
Copy link

pr-commenter bot commented Oct 11, 2024

Benchmarks

Benchmark execution time: 2024-10-29 10:57:00

Comparing candidate commit 964c763 in PR branch watson/DEBUG-2984/exit-spans with baseline commit 49d6c58 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 260 metrics, 6 unstable metrics.

@watson watson force-pushed the watson/DEBUG-2984/exit-spans branch from 67a3c3c to 4ee30f7 Compare October 11, 2024 13:09
@watson watson force-pushed the watson/span-origin branch from ff95181 to 878d076 Compare October 12, 2024 06:14
@watson watson force-pushed the watson/DEBUG-2984/exit-spans branch from 4ee30f7 to 137a5a4 Compare October 12, 2024 07:55
return String(Number(new Error().stack.split('\n')[2].match(/:(\d+):/)[1]) + 1)
}

function parseTags (tags) {
Copy link
Collaborator Author

@watson watson Oct 12, 2024

Choose a reason for hiding this comment

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

Converts: { _dd.a.a: 1, _dd.a.b: 2 } to { _dd: { a: { a: 1, b: 2 } } }

Do we already have a utility function that does this that I can re-use? Seem very much overkill to write this code for this test, but it does make the actual test much simpler to write once the tags are converted to a proper object structure 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Do we already have a utility function that does this that I can re-use?

Not sure we have one right now, but it could definitely be added to datadog-core/src/util instead of having the code in this file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've updated the code now and moved the function to datadog-core/src/util. Let me know what you think.

@watson watson force-pushed the watson/DEBUG-2984/exit-spans branch from 137a5a4 to d60ee21 Compare October 12, 2024 08:09
@watson watson marked this pull request as ready for review October 12, 2024 08:10
@watson watson requested review from a team as code owners October 12, 2024 08:10
Base automatically changed from watson/span-origin to master October 14, 2024 05:00
@watson watson requested review from a team as code owners October 14, 2024 05:00
@watson watson requested a review from tonyredondo October 14, 2024 05:00
@watson watson force-pushed the watson/DEBUG-2984/exit-spans branch 4 times, most recently from be8019d to 78d6ef0 Compare October 14, 2024 07:33
juan-fernandez
juan-fernandez previously approved these changes Oct 14, 2024
shatzi
shatzi previously approved these changes Oct 28, 2024
startSpan (...args) {
const span = super.startSpan(...args)
if (this._tracerConfig.codeOriginForSpans.enabled) {
span.addTags(exitTag(this.startSpan))
Copy link

Choose a reason for hiding this comment

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

nit: would suggest rename entryTag and exitTag method names. when reading the code it look like it add just one tag, which is misleading. I was thinking... createCodeOriginTagsForExitSpan(this.startSpan) but that might be a mouthful

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree they could probably be renamed from singular to plural 👍

But I think the method names should make sense locally to the module they live in. In this case, they live inside the datadog-code-origin module, so adding something like "code origin" to their name is repeating information that's already present in the module name. Similar to how the functions inside of the path module doesn't repeat the word path (e.g. it's just join instead of pathJoin).

If one wants to make them distinct in the importing module, one would either import the entire module:

const codeOrigin = require('../../../datadog-code-origin')
// ...
codeOrigin.exitTags()

Rename them on import:

const { exitTags: codeOriginExitTags } = require('../../../datadog-code-origin')
// ...
codeOriginExitTags()

Or you just let the reader look up where they are imported from - like today.

My general rule, unless there's a critical risk of misunderstanding what's going on, is to just leave their names as is, even though it's tiny bit harder to parse the code. I feel this is the normal way naming is done in most Node.js code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've updated the code now with the plural method names

rochdev
rochdev previously approved these changes Oct 28, 2024
@watson watson dismissed stale reviews from rochdev and shatzi via 964c763 October 29, 2024 10:46
@watson watson force-pushed the watson/DEBUG-2984/exit-spans branch from 5189a96 to 964c763 Compare October 29, 2024 10:46
@watson watson merged commit 91c4371 into master Oct 30, 2024
205 checks passed
@watson watson deleted the watson/DEBUG-2984/exit-spans branch October 30, 2024 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants