Skip to content

PROF-8520: Add DNS events to timeline#3822

Merged
szegedi merged 7 commits intomasterfrom
szegedi/PROF-8520-timeline-dns
Dec 1, 2023
Merged

PROF-8520: Add DNS events to timeline#3822
szegedi merged 7 commits intomasterfrom
szegedi/PROF-8520-timeline-dns

Conversation

@szegedi
Copy link
Copy Markdown
Contributor

@szegedi szegedi commented Nov 24, 2023

What does this PR do?

Gathers DNS performance events and adds them to the timeline.

Motivation

2023 Q4 Continuous Profiler OKR 5

Additional Notes

This is best reviewed commit by commit, or at least by reviewing the first commit separately, then the others together. The reason for this is that the first commit refactors the timeline profiler from only gathering GC events to having an architecture for gathering multiple types of events. This already pays off in spades – you can see how "Add DNS" is a pretty small commit after the refactor – but it will make adding Net events similarly trivial too.

Security

Datadog employees:

  • If this PR touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

@szegedi szegedi requested a review from a team as a code owner November 24, 2023 16:07
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 24, 2023

Overall package size

Self size: 5.6 MB
Deduped: 61.6 MB
No deduping: 62.35 MB

Dependency sizes

name version self size total size
@datadog/native-iast-taint-tracking 1.6.4 16.43 MB 16.44 MB
@datadog/native-appsec 5.0.0 15.16 MB 15.17 MB
@datadog/pprof 4.0.1 9.32 MB 10.16 MB
protobufjs 7.2.4 2.74 MB 6.52 MB
@datadog/native-iast-rewriter 2.2.1 2.27 MB 2.36 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.4.1 780.32 kB 780.32 kB
import-in-the-middle 1.4.2 41.4 kB 704.79 kB
pprof-format 2.0.7 588.12 kB 588.12 kB
msgpack-lite 0.1.26 201.16 kB 281.59 kB
opentracing 0.14.7 194.81 kB 194.81 kB
semver 7.5.4 93.4 kB 123.8 kB
@datadog/sketches-js 2.1.0 109.9 kB 109.9 kB
lodash.sortby 4.7.0 75.76 kB 75.76 kB
lru-cache 7.14.0 74.95 kB 74.95 kB
ipaddr.js 2.1.0 60.23 kB 60.23 kB
ignore 5.2.4 51.22 kB 51.22 kB
int64-buffer 0.1.10 49.18 kB 49.18 kB
istanbul-lib-coverage 3.2.0 29.34 kB 29.34 kB
lodash.uniq 4.5.0 25.01 kB 25.01 kB
limiter 1.1.5 23.17 kB 23.17 kB
dc-polyfill 0.1.2 22.77 kB 22.77 kB
retry 0.13.1 18.85 kB 18.85 kB
lodash.kebabcase 4.1.1 17.75 kB 17.75 kB
node-abort-controller 3.1.1 16.89 kB 16.89 kB
lodash.pick 4.4.0 16.33 kB 16.33 kB
jest-docblock 29.7.0 8.99 kB 12.76 kB
crypto-randomuuid 1.0.0 11.18 kB 11.18 kB
path-to-regexp 0.1.7 6.78 kB 6.78 kB
koalas 1.0.2 6.47 kB 6.47 kB
methods 1.1.2 5.29 kB 5.29 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

@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 24, 2023

Codecov Report

Attention: 82 lines in your changes are missing coverage. Please review.

Comparison is base (cdebb59) 85.06% compared to head (c7e4d31) 84.87%.

Files Patch % Lines
...ackages/dd-trace/src/profiling/profilers/events.js 3.52% 82 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3822      +/-   ##
==========================================
- Coverage   85.06%   84.87%   -0.20%     
==========================================
  Files         228      230       +2     
  Lines        9363     9579     +216     
  Branches       33       33              
==========================================
+ Hits         7965     8130     +165     
- Misses       1398     1449      +51     

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

@szegedi szegedi force-pushed the szegedi/PROF-8520-timeline-dns branch 2 times, most recently from 2772539 to 4c8ad83 Compare November 24, 2023 16:13
@pr-commenter
Copy link
Copy Markdown

pr-commenter Bot commented Nov 24, 2023

Benchmarks

Benchmark execution time: 2023-11-29 11:25:23

Comparing candidate commit c7e4d31 in PR branch szegedi/PROF-8520-timeline-dns with baseline commit cdebb59 in branch master.

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

@szegedi szegedi force-pushed the szegedi/PROF-8520-timeline-dns branch from 4c8ad83 to daa47f3 Compare November 27, 2023 11:45
@szegedi szegedi changed the base branch from szegedi/profiler-started-promise to master November 27, 2023 11:46
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤓 nitpick: ‏ Using the timestamps of the earliest and latest events to determine profile start and duration might be inaccurate.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, although I'm unsure what I could use instead of it…

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe take start/stop timestamps in start / profile functions.
Or have Profiler class take these timestamps and pass them along, that would have the nice side effect of all profiles having the same start and duration.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

that sounds like a great idea; I filed a JIRA issue in the timelines epic to track this.

nsavoire
nsavoire previously approved these changes Nov 27, 2023
Copy link
Copy Markdown
Collaborator

@nsavoire nsavoire left a comment

Choose a reason for hiding this comment

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

LGTM !
Thanks for splitting the PR into easily reviewable commits 😀

@szegedi
Copy link
Copy Markdown
Contributor Author

szegedi commented Nov 28, 2023

Hey @nsavoire, thank you for the review, but I have updated the code in the meantime :-(

I had to remove one commit (the one that removed locations from the GC events) – turns out the profiler backend ignores samples that are entirely without a location. I did rework it though to create one synthetic dummy location per profile and use it for all samples. I have a change in the pipeline for UI that makes these invisible (just as they are for .Net profiles).

I also experimented with various ways to visualize DNS events and avoid overlap between events. The easiest I could work out so far is to scatter them into multiple virtual "DNS" threads as needed, so I added that functionality too. A small test app that does:

const dns = require('node:dns')

require('dd-trace').init().profilerStarted().then(() => {
  dns.lookupService('13.224.103.60', 80, () => {})
  dns.lookup('example.org', () => {})
  dns.lookup('datadoghq.com', () => {})
  dns.resolve4('datadoghq.com', () => {})
  dns.lookup('dfslkgsjkrtgrdg.com', () => {})
  setTimeout(() => {
    dns.lookup('example.com', () => {})
  }, 200)
})

will produce a profile that renders like this (also thanks to some backend changes I made to group these DNS threads in a single group):
Screenshot 2023-11-28 at 17 39 45

If you can re-review the changes in the last 4 commits, it'd be appreciated; I'll open a new PR for any subsequent work.

@szegedi szegedi force-pushed the szegedi/PROF-8520-timeline-dns branch 3 times, most recently from 369002c to 41b272f Compare November 29, 2023 11:13
@szegedi szegedi force-pushed the szegedi/PROF-8520-timeline-dns branch from 41b272f to c7e4d31 Compare November 29, 2023 11:17
@szegedi szegedi requested a review from nsavoire November 29, 2023 11:19
Copy link
Copy Markdown
Collaborator

@nsavoire nsavoire left a comment

Choose a reason for hiding this comment

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

🚀

@szegedi szegedi merged commit 8bd6f02 into master Dec 1, 2023
@szegedi szegedi deleted the szegedi/PROF-8520-timeline-dns branch December 1, 2023 14:06
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.

2 participants