Skip to content
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

NH-71431: refactor log msg and remove unused code #115

Merged
merged 6 commits into from
Feb 5, 2024
Merged

Conversation

xuan-cao-swi
Copy link
Contributor

Description

  • Refactor log msg and remove unused code
  • Remove the threadlocal module because we don't need thread unsafe variable anymore; and merge the code into oboe_metal
  • Update the support_report.rb for existing otel potential issue (may need to expand more)
  • lint: add some newline for file

Test (if applicable)

  • Ad-hoc testbed test
  • Log message test was conducted manually (see page)

@xuan-cao-swi xuan-cao-swi requested a review from a team as a code owner February 1, 2024 22:16
end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why github auto-remove the newline...


SolarWindsAPM.logger.info '==================================================================='
SolarWindsAPM.logger.info "SolarWindsAPM info: Platform #{RUBY_PLATFORM}."
SolarWindsAPM.logger.info "Current solarwinds_apm version: #{SolarWindsAPM::Version::STRING}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to also detect and log out the OTel API/SDK versions (and possibly the instrumentation-all version) installed, seems useful for troubleshooting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added. also updated the test page.

===================================================================
I, [2024-02-05T07:20:09.144844 #507]  INFO -- : Ruby 3.1.0 on platform aarch64-linux.
I, [2024-02-05T07:20:09.144853 #507]  INFO -- : Current solarwinds_apm version: 6.0.0.prev7.
I, [2024-02-05T07:20:09.144859 #507]  INFO -- : OpenTelemetry version: 1.3.1.
I, [2024-02-05T07:20:09.144872 #507]  INFO -- : OpenTelemetry instrumentation version: 0.56.0.
===================================================================

lib/solarwinds_apm.rb Outdated Show resolved Hide resolved
require_relative './oboe_metal' # initialize reporter: SolarWindsAPM.loaded = true

SolarWindsAPM.logger.info '==================================================================='
SolarWindsAPM.logger.info "SolarWindsAPM info: Platform #{RUBY_PLATFORM}."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add a log message about the Ruby version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

Copy link
Contributor

@cheempz cheempz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the testing results page @xuan-cao-swi! Great to keep cleaning up the unused code, I left some comments on the log messgaes.

require 'solarwinds_apm/config'

SolarWindsAPM::Config.load_config_file
SolarWindsAPM.loaded = false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems safer to keep this initialization of loaded to false, any reason to remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I changed the structure of SolarWindsAPM module (oboe_metal.rb) that initialize the loaded as false; so its default will be false as always. It will change to true once reporter initialization process finished without any issue.

xuan-cao-swi and others added 2 commits February 2, 2024 16:53
Co-authored-by: Lin Lin <lin.lin@solarwinds.com>
Copy link
Contributor

@cheempz cheempz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the revisit @xuan-cao-swi!

@xuan-cao-swi xuan-cao-swi merged commit 57beca2 into main Feb 5, 2024
12 checks passed
@xuan-cao-swi xuan-cao-swi deleted the NH-71431 branch February 6, 2024 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants