-
Notifications
You must be signed in to change notification settings - Fork 375
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-6660] Add profiling_enabled
state to environment logger output
#2541
[PROF-6660] Add profiling_enabled
state to environment logger output
#2541
Conversation
# def profiling_enabled | ||
# end | ||
def profiling_enabled | ||
!!Datadog.configuration.profiling.enabled |
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.
We don't need !!
, since the option is already coerced with env_to_bool
.
option :enabled do |o|
o.default { env_to_bool(Profiling::Ext::ENV_ENABLED, false) }
o.lazy
end
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.
Can someone do c.profiling.enabled = nil
?
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.
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.
Good point. It turns out you can use whatever you want:
$ DD_TRACE_DEBUG=true bundle exec pry
[1] pry(main)> require 'ddtrace'
=> true
[2] pry(main)> Datadog.configure { |c| c.profiling.enabled = 'i like pie' };
D, [2023-01-12T13:52:08.108222 #628982] DEBUG -- ddtrace: [ddtrace] (dd-trace-rb/lib/datadog/core/workers/async.rb:132:in `start_worker') Starting thread for: #<Datadog::Core::Telemetry::Heartbeat:0x000062ba9b97a3e8>
D, [2023-01-12T13:52:08.108383 #628982] DEBUG -- ddtrace: [ddtrace] (dd-trace-rb/lib/datadog/core/configuration/components.rb:405:in `startup!') Profiling started
So I think the !!
is probably best to keep for now, although we should probably think about making the configuration system itself less permissive.
**What does this PR do?**: This PR adds a `profiling_enabled` entry in the environment logger output, which reflects the current configuration for profiling. **Motivation**: This was requested internally as being useful to support, and at least Node and Java already provide this key. We also had a TODO for it, so one less TODO. **Additional Notes**: I hesitated between adding the configuration of the profiler being on, or the profiler actually being on (e.g. it's possible that the profiler has been requested on, but can't be enabled because for instance google-protobuf is missing). My thinking is that the information about if the profiler really is on could be instead relayed via telemetry, so it may not be worth duplicating it here. **How to test the change?**: Change includes test coverage.
2e75087
to
8cb72b7
Compare
…-environment-logger
CI is broken for JRuby 9.2.8.0 due to an unrelated issue, going ahead and merging this. |
What does this PR do?:
This PR adds a
profiling_enabled
entry in the environment logger output, which reflects the current configuration for profiling.Motivation:
This was requested internally as being useful to support, and at least Node and Java already provide this key.
We also had a TODO for it, so one less TODO.
Additional Notes:
I hesitated between adding the configuration of the profiler being on, or the profiler actually being on (e.g. it's possible that the profiler has been requested on, but can't be enabled because for instance google-protobuf is missing).
My thinking is that the information about if the profiler really is on could be instead relayed via telemetry, so it may not be worth duplicating it here.
How to test the change?:
Change includes test coverage.