Skip to content

Configurable NativeSpaceProfiler samplingInterval #5725

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

Closed
wants to merge 1 commit into from

Conversation

dyv
Copy link
Contributor

@dyv dyv commented May 13, 2025

What does this PR do?

Allow users of ddtrace to configure samplingIntervals for the NativeSpaceProfiler.

Motivation

Heap sampling rate needs to be configurable for JS applications that have very high allocation rates. In services, with high allocation rates, the default sampling interval can cause unacceptable slow downs. This change allows users to find a more appropriate heap sampling rate that is tuned to their application's allocation rate.

Prior to this Wall, Events, and Space Profilers were all referencing the same option: samplingInterval, however, they had different units. NativeWallProfiler and EventsProfiler expected samplingInterval to be in hertz, and NativeSpaceProfiler expected samplingInterval to be in number of bytes between samples. This and the lack of environment variable support made it impossible for users to configure the NativeSpaceProfiler samplingInterval.

Additional Notes

This breaks the API of NativeSpaceProfiler since it switches from accepting samplingInterval to accepting heapSamplingInterval. That said, this is not a publicly exported class, so it seems acceptable to make this change in a non-backwards compatible way. If that is not acceptable, it would also be possible to update this change to allow for both samplingInterval and heapSamplingInterval and give the latter precedence. I did not do that, because it seemed to add unnecessary complexity to the profiler.

Tests

All of the following tests have been verified locally:

  • yarn test:profiler
  • yarn test:integration:profiler
  • yarn lint

Allow users of ddtrace to configure samplingIntervals for the
NativeSpaceProfiler. Prior to this Wall, Events, and Space Profilers were all
referencing the same option: samplingInterval, however, they had different
units. NativeWallProfiler and EventsProfiler expected samplingInterval to be in
hertz, and NativeSpaceProfiler expected samplingInterval to be in number of
bytes between samples. This and the lack of environment variable support made
it impossible for users to configure the NativeSpaceProfiler samplingInterval.

Heap sampling rate needs to be configurable for JS applications that have very
high allocation rates. In services, with high allocation rates, the default
sampling interval can cause unacceptable slow downs. This change allows users
to find a more appropriate heap sampling rate that is tuned to their
application's allocation rate.
@dyv dyv requested a review from a team as a code owner May 13, 2025 21:07
Copy link

PR Security Update

All commits in this PR up to and including 92b3ed3 have been reviewed and marked safe by SDLC security. For any questions, please reach out to #ci-for-external-contributors-collab on Slack.

@szegedi
Copy link
Contributor

szegedi commented May 23, 2025

Hi Dylan, Attila from Datadog's Node.js profiler team here. Thank you very much for your contribution, especially for providing a well-written rationale and for ensuring tests all pass.

I agree there's no breakage of backwards compatibility because as far as I can see, we don't have any way to let the customer configure the sampling rate right now (e.g. there's no equivalent environment variable for specifying a time interval.)

I'll be working on integrating this, will follow up here.

Copy link

codecov bot commented May 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.07%. Comparing base (87fcd71) to head (92b3ed3).
Report is 41 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5725      +/-   ##
==========================================
+ Coverage   77.63%   79.07%   +1.43%     
==========================================
  Files         480      514      +34     
  Lines       22460    23526    +1066     
==========================================
+ Hits        17437    18603    +1166     
+ Misses       5023     4923     -100     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@szegedi
Copy link
Contributor

szegedi commented May 27, 2025

Hi Dylan, our usual process is to recreate external contribution PRs with autorship information intact – this is needed to ensure our internal CI processes run correctly. I did so in #5766 and also resolved in it the merge conflict that popped up in the meantime. That PR is now merged and will be picked up for the 5.54.0 release of dd-trace-js. Thank you again for submitting this!

@szegedi szegedi closed this May 27, 2025
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