PROF-8545: (Simplified) Restore eager release of tags, adapt endpoint profiling code #3759

szegedi · 2023-11-02T09:00:54Z

What does this PR do?

Restores eager releasing of tags for finished and exported spans. Since this also affects the way wall profiler can compute endpoints, that code needs to be adapted as well.

Motivation

Reports of too high memory pressure since we removed eager cleaning of tags.

Additional Notes

The implementation is much simpler than that in #3755. It eschews complications that were merely performance optimizations: release of tags held by sampling context when the span ends, and reduction of work for repeated lookup of web spans for a single span. This is the minimal code change necessary to keep operating correctly in face of restored eager tag release in span_processor.js.

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.

github-actions · 2023-11-02T09:01:47Z

Overall package size

Self size: 5.44 MB
Deduped: 61.09 MB
No deduping: 61.26 MB

Dependency sizes

name	version	self size	total size
@datadog/native-iast-taint-tracking	1.6.3	16.43 MB	16.43 MB
@datadog/native-appsec	4.0.0	14.83 MB	14.83 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
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 · 2023-11-02T09:01:59Z

Codecov Report

Merging #3759 (d8b4b43) into master (78a4827) will increase coverage by 0.00%.
The diff coverage is 9.09%.

@@           Coverage Diff            @@
##           master    #3759    +/-   ##
========================================
  Coverage   85.91%   85.92%            
========================================
  Files         225      227     +2     
  Lines        9138     9326   +188     
  Branches       33       33            
========================================
+ Hits         7851     8013   +162     
- Misses       1287     1313    +26

Files	Coverage Δ
packages/dd-trace/src/span_processor.js	`50.00% <100.00%> (+1.25%)`	⬆️
packages/dd-trace/src/profiling/profilers/wall.js	`45.38% <0.00%> (-4.20%)`	⬇️

... and 4 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

pr-commenter · 2023-11-02T09:08:36Z

Benchmarks

Benchmark execution time: 2023-11-08 14:26:02

Comparing candidate commit d8b4b43 in PR branch szegedi/PROF-8545-eager-cleaner-2 with baseline commit 78a4827 in branch master.

Found 17 performance improvements and 1 performance regressions! Performance is the same for 467 metrics, 7 unstable metrics.

scenario:exporting-pipeline-0.4-16

🟩 cpu_user_time [-54.604ms; -49.193ms] or [-17.618%; -15.872%]
🟩 execution_time [-57.949ms; -53.301ms] or [-16.688%; -15.349%]
🟩 instructions [-114.6M instructions; -113.9M instructions] or [-13.625%; -13.542%]

scenario:exporting-pipeline-0.4-18

🟩 cpu_user_time [-60.449ms; -54.988ms] or [-18.387%; -16.726%]
🟩 execution_time [-59.998ms; -56.130ms] or [-16.371%; -15.316%]
🟩 instructions [-133.3M instructions; -132.8M instructions] or [-14.714%; -14.661%]

scenario:exporting-pipeline-0.4_with_stats-16

🟩 cpu_user_time [-58.061ms; -52.551ms] or [-18.748%; -16.969%]
🟩 execution_time [-61.791ms; -56.489ms] or [-17.783%; -16.257%]
🟩 instructions [-116.2M instructions; -114.9M instructions] or [-13.768%; -13.615%]

scenario:exporting-pipeline-0.4_with_stats-18

🟩 cpu_user_time [-56.055ms; -50.394ms] or [-17.109%; -15.381%]
🟩 execution_time [-59.966ms; -56.202ms] or [-16.353%; -15.327%]
🟩 instructions [-133.2M instructions; -132.7M instructions] or [-14.669%; -14.607%]

scenario:exporting-pipeline-0.5-16

🟩 instructions [-57.0M instructions; -56.7M instructions] or [-8.825%; -8.774%]

scenario:exporting-pipeline-0.5-18

🟩 instructions [-72.0M instructions; -71.6M instructions] or [-11.249%; -11.197%]

scenario:exporting-pipeline-0.5_with_stats-16

🟩 instructions [-56.8M instructions; -56.6M instructions] or [-8.764%; -8.725%]

scenario:exporting-pipeline-0.5_with_stats-18

🟩 instructions [-74.0M instructions; -73.6M instructions] or [-11.483%; -11.430%]

scenario:plugin-graphql-with-depth-off-18

🟩 max_rss_usage [-131.459KB; -121.729KB] or [-13.665%; -12.654%]

scenario:spans-finish-later-16

🟥 max_rss_usage [+10.077KB; +10.372KB] or [+5.172%; +5.324%]

jbertran · 2023-11-06T13:57:37Z

packages/dd-trace/test/tracer.spec.js

+        tracer.trace('name', {}, () => {})
+        const trace = tracer._exporter.export.getCall(0).args[0][0]
+        expect(trace).to.have.property(EXPORT_SERVICE_NAME, 'service')
+        expect(trace.meta).to.not.have.property(BASE_SERVICE)
      })

      it('should be set when tracer.trace service matched configured service', () => {


Suggested change

it('should be set when tracer.trace service matched configured service', () => {

it('should not be set when tracer.trace service matched configured service', () => {

As you mentioned this test makes no sense with that missing not 🤦

packages/dd-trace/src/profiling/profilers/wall.js

rochdev · 2023-11-07T01:19:25Z

packages/dd-trace/src/profiling/profilers/wall.js

@@ -120,6 +120,7 @@ class NativeWallProfiler {
      this._pprof.time.setContext(this._currentContext)
      this._lastSpan = undefined
      this._lastStartedSpans = undefined
+      this._webTags = undefined


Can this be cleaned up after it's not longer needed so that the tags can be GCed?

What do you mean? We'll keep updating this variable on every async_hook.enter() with the tags of some currently active span, so we don't hold on to any particular tags object for very long. We'll hold on longer to those that were active when we took a sample (once every 10ms) by transferring the reference into the _currentContext. That reference will go away next time we serialize the profile (once every minute.) We can't do better than that. In fact, we can – we can also release the tags as soon as the span ends; I do have that functionality in the other PR, #3755 where I added optimizations to release those as eagerly as possible, but Stephen considered it too hard to review at once so I started with this simplified one instead. I plan to add optimizations back in gradually.

One point of improvement here would be to reset the references when the profiler is stopped, but that won't make much difference.

This ensures we hold a reference to the tags object the span used before it was finished. Even if it finishes between `_enter` and `_updateContext` and its tags get replaced by an empty object, we will still hold the reference to the populated one in `_updateContext`.

…de (#3759) * Restores eager release of tags for exported spans in `process_spans.js`. * Moves tag fetching in `wall.js` from `_updateContext` to `_enter` to compensate for above. * Fixes some tests that relied on tags being lazily released.

…de (DataDog#3759) * Restores eager release of tags for exported spans in `process_spans.js`. * Moves tag fetching in `wall.js` from `_updateContext` to `_enter` to compensate for above. * Fixes some tests that relied on tags being lazily released.

szegedi requested a review from nsavoire November 2, 2023 09:04

szegedi added semver-patch profiling labels Nov 2, 2023

szegedi mentioned this pull request Nov 2, 2023

PROF-8545: Restore eager release of tags, adapt endpoint profiling code #3755

Closed

2 tasks

Base automatically changed from szegedi/PROF-8461-separate-ch-from-ep to master November 2, 2023 09:17

szegedi force-pushed the szegedi/PROF-8545-eager-cleaner-2 branch from d85482a to 5677da3 Compare November 2, 2023 09:21

Qard previously approved these changes Nov 3, 2023

View reviewed changes

szegedi dismissed Qard’s stale review via b111fb6 November 6, 2023 13:41

szegedi marked this pull request as ready for review November 6, 2023 13:45

szegedi requested a review from a team as a code owner November 6, 2023 13:45

szegedi force-pushed the szegedi/PROF-8545-eager-cleaner-2 branch from b111fb6 to a3fecf6 Compare November 6, 2023 13:46

szegedi requested review from Qard and jbertran November 6, 2023 13:48

jbertran reviewed Nov 6, 2023

View reviewed changes

jbertran previously approved these changes Nov 6, 2023

View reviewed changes

szegedi dismissed jbertran’s stale review via 182f995 November 6, 2023 14:04

jbertran previously approved these changes Nov 6, 2023

View reviewed changes

nsavoire reviewed Nov 6, 2023

View reviewed changes

packages/dd-trace/src/profiling/profilers/wall.js Show resolved Hide resolved

rochdev reviewed Nov 7, 2023

View reviewed changes

szegedi dismissed jbertran’s stale review via 247193c November 8, 2023 13:35

szegedi added 7 commits November 8, 2023 15:17

Restore eager tag clearing

0f61727

Fix tests

7d33fdf

Fix test wording

4274594

Add back the comment

2306877

Cleanup temporary span data on stop

a4f0119

Rename var to _lastWebTags for consistency

d8b4b43

szegedi force-pushed the szegedi/PROF-8545-eager-cleaner-2 branch from 247193c to d8b4b43 Compare November 8, 2023 14:17

szegedi mentioned this pull request Nov 8, 2023

PROF-8545: Cache web tags lookup, so we only perform it once per span #3779

Merged

2 tasks

nsavoire approved these changes Nov 8, 2023

View reviewed changes

szegedi merged commit 0999ce6 into master Nov 8, 2023
102 of 105 checks passed

szegedi deleted the szegedi/PROF-8545-eager-cleaner-2 branch November 8, 2023 17:27

This was referenced Nov 17, 2023

v3.40.0 #3801

Merged

v4.19.0 #3803

Merged

erikvanbrakel mentioned this pull request Jul 5, 2024

[DEVX-3565] add neo4j plugin for v4.41 mazedesignhq/dd-trace-js#15

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PROF-8545: (Simplified) Restore eager release of tags, adapt endpoint profiling code #3759

PROF-8545: (Simplified) Restore eager release of tags, adapt endpoint profiling code #3759

szegedi commented Nov 2, 2023

github-actions bot commented Nov 2, 2023 •

edited

Loading

codecov bot commented Nov 2, 2023 •

edited

Loading

pr-commenter bot commented Nov 2, 2023 •

edited

Loading

jbertran Nov 6, 2023

rochdev Nov 7, 2023

szegedi Nov 8, 2023

	it('should be set when tracer.trace service matched configured service', () => {
	it('should not be set when tracer.trace service matched configured service', () => {

PROF-8545: (Simplified) Restore eager release of tags, adapt endpoint profiling code #3759

PROF-8545: (Simplified) Restore eager release of tags, adapt endpoint profiling code #3759

Conversation

szegedi commented Nov 2, 2023

What does this PR do?

Motivation

Additional Notes

Security

github-actions bot commented Nov 2, 2023 • edited Loading

Overall package size

Dependency sizes

codecov bot commented Nov 2, 2023 • edited Loading

Codecov Report

pr-commenter bot commented Nov 2, 2023 • edited Loading

Benchmarks

scenario:exporting-pipeline-0.4-16

scenario:exporting-pipeline-0.4-18

scenario:exporting-pipeline-0.4_with_stats-16

scenario:exporting-pipeline-0.4_with_stats-18

scenario:exporting-pipeline-0.5-16

scenario:exporting-pipeline-0.5-18

scenario:exporting-pipeline-0.5_with_stats-16

scenario:exporting-pipeline-0.5_with_stats-18

scenario:plugin-graphql-with-depth-off-18

scenario:spans-finish-later-16

jbertran Nov 6, 2023

Choose a reason for hiding this comment

rochdev Nov 7, 2023

Choose a reason for hiding this comment

szegedi Nov 8, 2023

Choose a reason for hiding this comment

github-actions bot commented Nov 2, 2023 •

edited

Loading

codecov bot commented Nov 2, 2023 •

edited

Loading

pr-commenter bot commented Nov 2, 2023 •

edited

Loading