-
Notifications
You must be signed in to change notification settings - Fork 312
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
PROF-8458: Enhance the way GC profiler is enabled #3807
Conversation
* `DD_PROFILING_EXPERIMENTAL_TIMELINE_ENABLED=0` no longer enables it * can't set it by including a profiler type `'events'` directly * can only set it through either the env variable or options, with consistent results.
Overall package sizeSelf size: 5.56 MB Dependency sizes
🤖 This report was automatically generated by heaviest-objects-in-the-universe |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #3807 +/- ##
==========================================
+ Coverage 85.25% 85.26% +0.01%
==========================================
Files 230 230
Lines 9482 9482
Branches 33 33
==========================================
+ Hits 8084 8085 +1
+ Misses 1398 1397 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this new way of enabling timeline !
* `DD_PROFILING_EXPERIMENTAL_TIMELINE_ENABLED=0` no longer enables it * can't set it by including a profiler type `'events'` directly * can only set it through either the env variable or options, with consistent results.
* `DD_PROFILING_EXPERIMENTAL_TIMELINE_ENABLED=0` no longer enables it * can't set it by including a profiler type `'events'` directly * can only set it through either the env variable or options, with consistent results.
* `DD_PROFILING_EXPERIMENTAL_TIMELINE_ENABLED=0` no longer enables it * can't set it by including a profiler type `'events'` directly * can only set it through either the env variable or options, with consistent results.
* `DD_PROFILING_EXPERIMENTAL_TIMELINE_ENABLED=0` no longer enables it * can't set it by including a profiler type `'events'` directly * can only set it through either the env variable or options, with consistent results.
* `DD_PROFILING_EXPERIMENTAL_TIMELINE_ENABLED=0` no longer enables it * can't set it by including a profiler type `'events'` directly * can only set it through either the env variable or options, with consistent results.
* `DD_PROFILING_EXPERIMENTAL_TIMELINE_ENABLED=0` no longer enables it * can't set it by including a profiler type `'events'` directly * can only set it through either the env variable or options, with consistent results.
* `DD_PROFILING_EXPERIMENTAL_TIMELINE_ENABLED=0` no longer enables it * can't set it by including a profiler type `'events'` directly * can only set it through either the env variable or options, with consistent results.
What does this PR do?
Rights the wrongs in the way GC profiler is enabled:
DD_PROFILING_EXPERIMENTAL_TIMELINE_ENABLED
to any non-null value including0
orfalse
no longer enables it. Now only values1
andtrue
will work, as intended. 🤦'events'
directly into the list of profilers.Motivation
Use of the env var was buggy as it turned the profiler on as soon as the env var was non-null. We'll also need to use a proper config property
timelineEnabled
soon, so might as well introduce it already. Having an intermediate string specification ('events'
) is both unnecessary and allows for misconfiguration (e.g. turning it on whileconfig.timelineEnabled
is false)Security
Datadog employees:
@DataDog/security-design-and-guidance
.