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

Only report gc_time metric if profiler is enabled #877

Merged
merged 1 commit into from
Aug 5, 2022

Conversation

tombruijn
Copy link
Member

Let's only report the gc_time metric when there's an actual value to
report to avoid confusion of very small GC times.

If the Profiler is disabled, it reports 0.0 as the value for this
metric. We know when the profiler is enabled by asking it
GC::Profiler.enabled?.

By directly asking the Ruby GC profiler, instead of our wrapper based on
the enable_gc_instrumentation config option, we allow users to enable
and disable the profiler temporarily with GC::Profiler.enable and
GC::Profiler.disable calls in their app.

If the profiler is enabled, disabled, and then enabled again, it
remembers the total time from the last time it was enabled. By not
reporting 0 when it is disabled we don't reset the cache and don't
trigger a very high duration being reported the next time it's enabled.

> GC::Profiler.total_time
# => 0.0

> GC::Profiler.enable
> GC::Profiler.total_time
=> 0.001
> 10000.times { SecureRandom.uuid }
# => Does stuff for a long time
> GC::Profiler.total_time
# => 58.93548299997067

> GC::Profiler.disable
> GC::Profiler.total_time
# => 0.0
> 10000.times { SecureRandom.uuid }
# => Does stuff for a long time
> GC::Profiler.total_time
# => 0.0
> GC::Profiler.enable
> GC::Profiler.total_time
# => 58.93548299997067
# Same as before it was disabled without the GC time of the code that
# was run when it was disabled

Part of #868

Let's only report the `gc_time` metric when there's an actual value to
report to avoid confusion of very small GC times.

If the Profiler is disabled, it reports 0.0 as the value for this
metric. We know when the profiler is enabled by asking it
`GC::Profiler.enabled?`.

By directly asking the Ruby GC profiler, instead of our wrapper based on
the `enable_gc_instrumentation` config option, we allow users to enable
and disable the profiler temporarily with `GC::Profiler.enable` and
`GC::Profiler.disable` calls in their app.

If the profiler is enabled, disabled, and then enabled again, it
remembers the total time from the last time it was enabled. By not
reporting 0 when it is disabled we don't reset the cache and don't
trigger a very high duration being reported the next time it's enabled.

```ruby
> GC::Profiler.total_time
# => 0.0

> GC::Profiler.enable
> GC::Profiler.total_time
=> 0.001
> 10000.times { SecureRandom.uuid }
# => Does stuff for a long time
> GC::Profiler.total_time
# => 58.93548299997067

> GC::Profiler.disable
> GC::Profiler.total_time
# => 0.0
> 10000.times { SecureRandom.uuid }
# => Does stuff for a long time
> GC::Profiler.total_time
# => 0.0
> GC::Profiler.enable
> GC::Profiler.total_time
# => 58.93548299997067
# Same as before it was disabled without the GC time of the code that
# was run when it was disabled
```

Part of #868
@tombruijn tombruijn merged commit ffe49cf into main Aug 5, 2022
@tombruijn tombruijn deleted the gc-temporarily-disable-profiler branch July 31, 2023 09:26
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.

4 participants