NH-69948: upgrade to 14.0.1 #110
Conversation
There was a problem hiding this comment.
Looks good @xuan-cao-swi! I'd like a small enhancement as noted in the comments, lmk if we should discuss.
| options = SolarWindsAPM::OboeInitOptions.instance.array_for_oboe | ||
| _(options[21]).must_equal 2 | ||
| end | ||
| end |
There was a problem hiding this comment.
Can we also have tests verify the log_file_path init setting?
There was a problem hiding this comment.
To create the actual file, we kind need the c library, which is not doable here because we don't compile liboboe anymore for testing
There was a problem hiding this comment.
Yes, but how about verify that the init option for log file is set to the correct value based on env var? And if log level is disabled, I'd assume we don't ask liboboe to create a log file, i.e. env var for log file is ignored (not set in init option).
There was a problem hiding this comment.
Actually I think liboboe itself will ignore the log file option if log type is set to disabled? It would be good to see at least ad-hoc testing results in the Jira description.
There was a problem hiding this comment.
Yes, but how about verify that the init option for log file is set to the correct value based on env var?
Yes, we have test for it. I also create new one under logger test section
And if log level is disabled, I'd assume we don't ask liboboe to create a log file, i.e. env var for log file is ignored (not set in init option). Actually I think liboboe itself will ignore the log file option if log type is set to disabled?
Yes, if the log_type is not file based, the option will be invalid, and liboboe logger only create the file when the file path is valid value (here)
It would be good to see at least ad-hoc testing results in the Jira description.
You mean like in testbed? I already did manual testing, it seems work (i.e. no file created if disabled)
Co-authored-by: Lin Lin <lin.lin@solarwinds.com>
Co-authored-by: Lin Lin <lin.lin@solarwinds.com>
There was a problem hiding this comment.
LGTM, thanks for the revisit @xuan-cao-swi! I updated the config docs in 82c7189, take a look.
Description
Test (if applicable)