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.

Sign up for GitHub

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

Jump to bottom

Add config tests for OOM heap profiler #2943

Merged
merged 4 commits into from
Mar 28, 2023
Merged

Add config tests for OOM heap profiler #2943

merged 4 commits into from
Mar 28, 2023

Conversation

nsavoire
Copy link
Collaborator

What does this PR do?

  • Use isTrue in profiler config
  • Add config tests for OOM heap profiler

Motivation

More robust check for environment variable activation.
Fix activation of OOM heap profiler that before would have been activated by setting DD_PROFILING_EXPERIMENTAL_OOM_MONITORING_ENABLED to any value.

@nsavoire nsavoire requested a review from a team as a code owner March 27, 2023 20:56
@nsavoire nsavoire requested a review from r1viollet March 27, 2023 20:56
@nsavoire nsavoire assigned szegedi and unassigned szegedi Mar 27, 2023
@nsavoire nsavoire requested a review from szegedi March 27, 2023 20:56
@github-actions
Copy link

github-actions bot commented Mar 27, 2023 •
edited
Loading

Overall package size

Self size: 3.98 MB
Deduped: 59.49 MB
No deduping: 59.54 MB

Dependency sizes

name version self size total size
@datadog/native-iast-taint-tracking 1.1.1 13.38 MB 13.39 MB
@datadog/pprof 2.1.0 12.31 MB 13.2 MB
@datadog/native-appsec 2.0.0 12.33 MB 12.34 MB
@datadog/native-metrics 1.5.0 7.1 MB 7.11 MB
protobufjs 7.1.2 2.76 MB 6.55 MB
@datadog/native-iast-rewriter 2.0.1 2.09 MB 2.1 MB
opentracing 0.14.7 194.81 kB 194.81 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
semver 5.7.1 61.58 kB 61.58 kB
ipaddr.js 2.0.1 59.52 kB 59.52 kB
ignore 5.2.0 48.87 kB 48.87 kB
import-in-the-middle 1.3.4 32.7 kB 37.17 kB
istanbul-lib-coverage 3.2.0 29.34 kB 29.34 kB
retry 0.10.1 27.44 kB 27.44 kB
lodash.uniq 4.5.0 25.01 kB 25.01 kB
limiter 1.1.5 23.17 kB 23.17 kB
lodash.kebabcase 4.1.1 17.75 kB 17.75 kB
lodash.pick 4.4.0 16.33 kB 16.33 kB
node-abort-controller 3.0.1 14.33 kB 14.33 kB
crypto-randomuuid 1.0.0 11.18 kB 11.18 kB
diagnostics_channel 1.1.0 7.07 kB 7.07 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

codecov bot commented Mar 27, 2023 •
edited
Loading

Codecov Report

Merging #2943 (ff2b16b) into master (913d77c) will increase coverage by 1.30%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2943      +/-   ##
==========================================
+ Coverage   87.36%   88.67%   +1.30%     
==========================================
  Files         325      298      -27     
  Lines       11585    10885     -700     
  Branches       33       33              
==========================================
- Hits        10121     9652     -469     
+ Misses       1464     1233     -231
Impacted Files Coverage Δ
packages/dd-trace/src/profiling/config.js 96.93% <100.00%> (+17.55%) ⬆️

... and 32 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pr-commenter
Copy link

pr-commenter bot commented Mar 27, 2023 •
edited
Loading

Benchmarks

Comparing candidate commit ff2b16b in PR branch nsavoire/add_oom_config_test with baseline commit 913d77c in branch master.

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

Qard
Qard previously approved these changes Mar 27, 2023
View reviewed changes
Copy link
Contributor

@Qard Qard left a comment

Choose a reason for hiding this comment

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

LGTM other than a minor correction in test naming. :)

@nsavoire
Fix typo
156c1c0 
Co-authored-by: Stephen Belanger <stephen.belanger@datadoghq.com>
@nsavoire nsavoire requested a review from Qard March 28, 2023 12:37
r1viollet
r1viollet previously approved these changes Mar 28, 2023
View reviewed changes
Copy link

@r1viollet r1viollet left a comment

Choose a reason for hiding this comment

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

LGTM

@nsavoire nsavoire requested a review from r1viollet March 28, 2023 13:57
szegedi
szegedi previously approved these changes Mar 28, 2023
View reviewed changes
Copy link
Contributor

@szegedi szegedi left a comment

Choose a reason for hiding this comment

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

LGTM, just that question about duplicate strategy values.

packages/dd-trace/test/profiling/config.spec.js Outdated
enabled: true,
heapLimitExtensionSize: 1000000,
maxHeapExtensionCount: 2,
exportStrategies: ['process', 'interrupt', 'async', 'interrupt'],
Copy link
Contributor

Choose a reason for hiding this comment

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

I presume pass-through of duplicate values is a desired feature?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice suggestion, I will add de-duplication of strategies

@nsavoire nsavoire dismissed stale reviews from szegedi and r1viollet via ff2b16b March 28, 2023 14:37
@nsavoire nsavoire requested a review from szegedi March 28, 2023 14:37
@nsavoire nsavoire merged commit f493b19 into master Mar 28, 2023
@nsavoire nsavoire deleted the nsavoire/add_oom_config_test branch March 28, 2023 15:30
juan-fernandez pushed a commit that referenced this pull request Mar 28, 2023
@nsavoire @juan-fernandez
Add config tests for OOM heap profiler (#2943)
10c9a90 
* Use isTrue in profiling config

* Add OOM heap profiler config tests

* Deduplicate OOM export strategies
juan-fernandez pushed a commit that referenced this pull request Mar 28, 2023
@nsavoire @juan-fernandez
Add config tests for OOM heap profiler (#2943)
5bf3186 
* Use isTrue in profiling config

* Add OOM heap profiler config tests

* Deduplicate OOM export strategies
juan-fernandez pushed a commit that referenced this pull request Mar 28, 2023
@nsavoire @juan-fernandez
Add config tests for OOM heap profiler (#2943)
051d7e0 
* Use isTrue in profiling config

* Add OOM heap profiler config tests

* Deduplicate OOM export strategies
juan-fernandez pushed a commit that referenced this pull request Mar 28, 2023
@nsavoire @juan-fernandez
Add config tests for OOM heap profiler (#2943)
4d51391 
* Use isTrue in profiling config

* Add OOM heap profiler config tests

* Deduplicate OOM export strategies
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szegedi szegedi szegedi approved these changes

@Qard Qard Awaiting requested review from Qard

@r1viollet r1viollet Awaiting requested review from r1viollet

Assignees
No one assigned
Labels
semver-patch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nsavoire @szegedi @Qard @r1viollet