[PROF-6660] Add profiling_enabled state to environment logger output
#2541
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lloeki
approved these changes
Jan 12, 2023
TonyCTHsu
reviewed
Jan 12, 2023
| # def profiling_enabled | ||
| # end | ||
| def profiling_enabled | ||
| !!Datadog.configuration.profiling.enabled |
There was a problem hiding this comment.
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.
Can someone do c.profiling.enabled = nil?
There was a problem hiding this comment.
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.
TonyCTHsu
approved these changes
Jan 12, 2023
marcotc
approved these changes
Jan 12, 2023
**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.
ivoanjo
force-pushed
the
ivoanjo/prof-6660-add-profiling-enabled-to-environment-logger
branch
from
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. |
ivoanjo
deleted the
ivoanjo/prof-6660-add-profiling-enabled-to-environment-logger
branch
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?:
This PR adds a
profiling_enabledentry 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.