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.

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

Temporarily disable GC profiling by default in new Ruby profiler due to overhead #2481

Merged
merged 1 commit into from
Dec 9, 2022

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Dec 8, 2022

What does this PR do?:

We previously only enabled GC profiling by default on Ruby 2.x (we disabled it on 3.x due to a bug, see #2354).

This PR changes the profiling configuration to never enable GC profiling by default (thus bringing the Ruby 2.x and 3.x defaults into sync).

Motivation:

I've had one new Ruby profiler alpha user report that GC profiling had a quite big overhead, and I could replicate the issue on a GC-heavy benchmark.

Since we'd like to get the new Ruby profiler out into customer's hands in beta, I decided to disable this feature by default, and come back to it after beta is out.

Additional Notes:

GC profiling can still be enabled via configuration.

How to test the change?:

Change includes test coverage. Furthermore, profiling any application will no longer show the Garbage Collection frames in the flamegraph by default.

…to overhead

**What does this PR do?**:

We previously only enabled GC profiling by default on Ruby 2.x
(we disabled it on 3.x due to a bug, see #2354).

This PR changes the profiling configuration to never enable
GC profiling by default (thus bringing the Ruby 2.x and 3.x
defaults into sync).

**Motivation**:

I've had one new Ruby profiler alpha user report that GC profiling
had a quite big overhead, and I could replicate the issue on a
GC-heavy benchmark.

Since we'd like to get the new Ruby profiler out into customer's
hands in beta, I decided to disable this feature by default, and
come back to it after beta is out.

**Additional Notes**:

GC profiling can still be enabled via configuration.

**How to test the change?**:

Change includes test coverage. Furthermore, profiling any
application will no longer show the `Garbage Collection` frames in the
flamegraph by default.
@ivoanjo ivoanjo requested a review from a team December 8, 2022 11:28
@github-actions github-actions bot added the core Involves Datadog core libraries label Dec 8, 2022
@ivoanjo ivoanjo added profiling Involves Datadog profiling and removed core Involves Datadog core libraries labels Dec 8, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #2481 (c43e73a) into master (55a5fd3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2481   +/-   ##
=======================================
  Coverage   98.31%   98.32%           
=======================================
  Files        1118     1118           
  Lines       60341    60337    -4     
=======================================
- Hits        59327    59324    -3     
+ Misses       1014     1013    -1     
Impacted Files Coverage Δ
lib/datadog/core/configuration/settings.rb 99.19% <ø> (ø)
lib/datadog/core/configuration/components.rb 98.92% <100.00%> (ø)
spec/datadog/core/configuration/components_spec.rb 100.00% <100.00%> (ø)
...dog/profiling/collectors/cpu_and_wall_time_spec.rb 97.48% <0.00%> (-0.42%) ⬇️
...ng/contrib/active_support/cache/instrumentation.rb 87.58% <0.00%> (-0.09%) ⬇️
spec/datadog/profiling/native_extension_spec.rb 98.74% <0.00%> (+0.62%) ⬆️
lib/datadog/core/diagnostics/environment_logger.rb 100.00% <0.00%> (+1.57%) ⬆️

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

@ivoanjo ivoanjo merged commit dfed57e into master Dec 9, 2022
@ivoanjo ivoanjo deleted the ivoanjo/disable-gc-profiling-by-default branch December 9, 2022 08:40
@github-actions github-actions bot added this to the 1.8.0 milestone Dec 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
profiling Involves Datadog profiling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants