Existing type coercion behavior is implemented pretty well in the environment variable handling logic, given that all environment variable values are strings on input. But sourcing parameters from elsewhere - especially the YAML configuration file - will bypass this type coercion, leading to the following outstanding issues:
Behavior from environment variable input (example: '123' becomes 123) can reasonably be expected from other configuration sources, leading to unexpected behavior (or more specifically a lack of coercion behavior in this case).
The CSEC agent - which needs to deal with numeric parameter values more than the APM agent does - now does interger coercion that the APM agent do. Users may reasonably expect consistency here.
The lack of integer coercion is particularly problematic because in one way the agent tries to do the right thing by not succeeding with a string value at all (rather than converting a string unexpectedly to 0 or whatnot), but unfortunately has the potential to permit a fatal exception to lay hidden by not performing any type or bounds check on some integers until the time that they are actively used. This lack of config parse time checking in addition to the lack of type coercion can be particularly problematic (buggy).
Existing non-coercion behavior for the YAML input seems to be pretty well rooted in New Relic's history of only offering a Ruby agent back when the company first started. In those days, knowledge of Ruby and YAML were pretty safely assumed. Nowadays with New Relic offering multiple agents that take advantage of multiple configuration file formats, knowledge of YAML-to-Ruby behavior should not assumed when reasonable coercion can be performed - especially when this coercion already takes place for environment variables. This will be especially important to customers that leverage multiple agents to observe applications written in multiple languages.
While bullet point 4 might make for a good "feature request" candidate, the first 3 bullet points pertaining to inconsistency and potential unexpected behavior swayed us into favoring the "bug" category.
Definition of Done
For all configuration parameters (defined in default_source.rb) that make use of a :type attribute, ensure that user supplied values are coerced to that type in a way that is consistent between YAML and environment variables and between the Ruby APM and CSEC agents.
To address bullet point 3, any problematic invalid values should be identified at agent start time and not at the time the agent goes to use the values that were previously blindly trusted.
workato-integration[bot]
commented
1 month ago
Existing type coercion behavior is implemented pretty well in the environment variable handling logic, given that all environment variable values are strings on input. But sourcing parameters from elsewhere - especially the YAML configuration file - will bypass this type coercion, leading to the following outstanding issues:
'123'becomes123) can reasonably be expected from other configuration sources, leading to unexpected behavior (or more specifically a lack of coercion behavior in this case).0or whatnot), but unfortunately has the potential to permit a fatal exception to lay hidden by not performing any type or bounds check on some integers until the time that they are actively used. This lack of config parse time checking in addition to the lack of type coercion can be particularly problematic (buggy).While bullet point 4 might make for a good "feature request" candidate, the first 3 bullet points pertaining to inconsistency and potential unexpected behavior swayed us into favoring the "bug" category.
Definition of Done
default_source.rb) that make use of a:typeattribute, ensure that user supplied values are coerced to that type in a way that is consistent between YAML and environment variables and between the Ruby APM and CSEC agents.