NH-69750: change trigger_tracing_mode to symbol #100
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -51,7 +51,7 @@ | |||
| class TraceInfo | |||
| attr_reader :tracestring, :trace_id, :span_id, :trace_flags, :do_log | |||
|
|
|||
| REGEXP = /^(?<tracestring>(?<version>[a-f0-9]{2})-(?<trace_id>[a-f0-9]{32})-(?<span_id>[a-f0-9]{16})-(?<flags>[a-f0-9]{2}))$/.freeze | |||
| REGEXP = /^(?<tracestring>(?<version>[a-f0-9]{2})-(?<trace_id>[a-f0-9]{32})-(?<span_id>[a-f0-9]{16})-(?<flags>[a-f0-9]{2}))$/.freeze # rubocop:disable Style/RedundantFreeze | |||
Check warning
Code scanning / Rubocop
Checks for rubocop:disable comments that can be removed. Note: this cop is not disabled when disabling all cops. It must be explicitly disabled.
| @@ -107,7 +107,7 @@ | |||
| marginalia_job["class"] if marginalia_job.respond_to?(:[]) | |||
| end | |||
|
|
|||
| DEFAULT_LINES_TO_IGNORE_REGEX = %r{\.rvm|/ruby/gems/|vendor/|marginalia|rbenv|monitor\.rb.*mon_synchronize}.freeze | |||
| DEFAULT_LINES_TO_IGNORE_REGEX = %r{\.rvm|/ruby/gems/|vendor/|marginalia|rbenv|monitor\.rb.*mon_synchronize}.freeze # rubocop:disable Style/RedundantFreeze | |||
Check warning
Code scanning / Rubocop
Checks for rubocop:disable comments that can be removed. Note: this cop is not disabled when disabling all cops. It must be explicitly disabled.
There was a problem hiding this comment.
Nice refactoring @xuan-cao-swi! Getting close, please see my comments.
Co-authored-by: Lin Lin <[email protected]>
There was a problem hiding this comment.
Thanks @xuan-cao-swi, the refactoring LGTM but diving more into tests is making me really wonder about our treatment of logging level (which is great that you added tests for, it's an important aspect of our library and we definitely want to get it right). Can you investigate?
| _(SolarWindsAPM::Config[:dummy_key]).must_equal true | ||
| end | ||
|
|
||
| it 'with nil env, config env true, use default false, should be true' do |
There was a problem hiding this comment.
Minor, I think this test is only meaningful if the env var is set, i.e. ENV['DUMMY_KEY'] = 'false'
test/solarwinds_apm/config_test.rb
Outdated
| it 'debug_level is in the range' do | ||
| SolarWindsAPM::Config[:debug_level] = 1 | ||
| SolarWindsAPM::Config.set_log_level | ||
| _(SolarWindsAPM.logger.level).must_equal 3 |
There was a problem hiding this comment.
Hmm, why is the log level 3 if we're setting what looks to be a valid level of 1?
Also just noticed
apm-ruby/lib/solarwinds_apm/config.rb
Line 71 in 65c5a43
There was a problem hiding this comment.
I think customer doc is correct.
test/solarwinds_apm/config_test.rb
Outdated
| ENV['SW_APM_DEBUG_LEVEL'] = '3' | ||
| SolarWindsAPM::Config[:debug_level] = 1 | ||
| SolarWindsAPM::Config.set_log_level | ||
| _(SolarWindsAPM.logger.level).must_equal 1 |
There was a problem hiding this comment.
The test says env var should override but the result looks the opposite, why?
There was a problem hiding this comment.
When SW_APM_DEBUG_LEVEL = 3, the mapping will result the SolarWindsAPM.logger.level to 1.
If SolarWindsAPM::Config[:debug_level] = 1 (and empty SW_APM_DEBUG_LEVEL), then the mapping will result the SolarWindsAPM.logger.level to 3.
test/solarwinds_apm/config_test.rb
Outdated
| ENV['SW_APM_DEBUG_LEVEL'] = '7' | ||
| SolarWindsAPM::Config[:debug_level] = 1 | ||
| SolarWindsAPM::Config.set_log_level | ||
| _(SolarWindsAPM.logger.level).must_equal 0 |
There was a problem hiding this comment.
If the provided value is invalid, we can go two ways--but neither should result in a very mysterious setting of log level to 0.
- First approach: stop at first source of config, i.e. env var. since its value is invalid, fallback to default.
- Second approach: continue to evaluate other source of config, i.e. config file, and use its value if valid else fallback to default.
| ENV.delete('SW_APM_DEBUG_LEVEL') | ||
| end | ||
|
|
||
| it 'env var override config but out of range by less than 0' do |
There was a problem hiding this comment.
I am honestly very confused and mystified by the range verification... you might be too based on the comment in line 217? Can this logic be cleaned up to actually make sense?
There was a problem hiding this comment.
Yes, inherited set_log_level function is kind confusing. I will make it clear
| end | ||
| end | ||
|
|
||
| describe 'general checking on []=' do |
There was a problem hiding this comment.
Really appreciate this test suite--the checking of defaults on application startup, and typical customer overrides via env var.
test/solarwinds_apm/config_test.rb
Outdated
| describe 'test transaction_settings' do | ||
| # test is in transaction_settings_test.rb | ||
| end |
There was a problem hiding this comment.
Doesn't seem useful, consider removing.
|
Thanks for the response @xuan-cao-swi, yes taking a closer look I realize now that the debug level configuration option is being used to internally set the Ruby Logger level. IMHO it would be clearer if the Ruby Logger constants like
|
lib/solarwinds_apm/config.rb
Outdated
| @@ -68,11 +74,37 @@ def self.config_from_env | |||
| end | |||
|
|
|||
| def self.set_log_level | |||
| SolarWindsAPM::Config[:debug_level] = 3 unless (-1..6).cover?(SolarWindsAPM::Config[:debug_level]) | |||
| log_level = (ENV['SW_APM_DEBUG_LEVEL'] || SolarWindsAPM::Config[:debug_level] || 3).to_i | |||
| SolarWindsAPM.logger.level = LOGGER_LEVEL_MAPPING[log_level] || 1 # default log level info | |||
There was a problem hiding this comment.
Same question, can we use the Ruby Logger constant INFO insteadn of 1?
There was a problem hiding this comment.
LGTM, thanks for the revisits @xuan-cao-swi! Much better foundations for the config logic and will come in handy when we upgrade to the new liboboe that has debug log level changes!
Description
Change the trigger_tracing_mode from string to symbol (e.g.
:enabled/:disabled)