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-67864: use rubocop-performance #94

Merged
merged 3 commits into from
Nov 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/workflows/rubocop-analysis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ jobs:
- name: Set up Ruby
uses: ruby/setup-ruby@v1
with:
ruby-version: 3.1
ruby-version: '3.1.0'

# This step is not necessary if you add the gem to your Gemfile
- name: Install Code Scanning integration
Expand All @@ -33,6 +33,7 @@ jobs:
run: |
bundle install
gem install rubocop
gem install rubocop-performance

- name: RuboCop run
run: |
Expand Down
4 changes: 3 additions & 1 deletion .rubocop.yml
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
require: rubocop-rake
require:
- rubocop-rake
- rubocop-performance

AllCops:
RubyInterpreters:
Expand Down
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,6 @@ group :development, :test do
gem 'package_cloud'
gem 'rubocop'
gem 'rubocop-rake', require: false
gem 'rubocop-performance', require: false
gemspec
end
4 changes: 2 additions & 2 deletions Rakefile
Original file line number Diff line number Diff line change
Expand Up @@ -365,8 +365,8 @@ task :build_gem_push_to_packagecloud, [:version] do |_, args|
end

puts "\n=== Gem will be pushed #{gem_to_push} ===\n"
gem_to_push_version = gem_to_push&.match(/-\d*.\d*.\d*/).to_s.gsub('-', '')
gem_to_push_version = gem_to_push&.match(/-\d*.\d*.\d*.pre/).to_s.gsub('-', '') if args[:version].include? 'pre'
gem_to_push_version = gem_to_push&.match(/-\d*.\d*.\d*/).to_s.delete!('-')
gem_to_push_version = gem_to_push&.match(/-\d*.\d*.\d*.pre/).to_s.delete!('-') if args[:version].include? 'pre'

abort('Could not find the required gem file.') if gem_to_push.nil? || gem_to_push_version != args[:version]

Expand Down
7 changes: 4 additions & 3 deletions ext/oboe_metal/extconf.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,10 @@

# Download the appropriate liboboe from Staging or Production
version = File.read(File.join(swo_include, 'VERSION')).strip
if ENV['OBOE_DEV'].to_s.downcase == 'true'
if ENV['OBOE_DEV'].to_s.casecmp('true').zero?
swo_path = "https://solarwinds-apm-staging.s3.us-west-2.amazonaws.com/apm/c-lib/nightly"
puts 'Fetching c-lib from DEVELOPMENT Build'
elsif ENV['OBOE_STAGING'].to_s.downcase == 'true'
elsif ENV['OBOE_STAGING'].to_s.casecmp('true').zero?
swo_path = File.join('https://agent-binaries.global.st-ssp.solarwinds.com/apm/c-lib/', version)
puts 'Fetching c-lib from STAGING'
else
Expand All @@ -38,7 +38,8 @@

swo_arch = 'x86_64'
system_arch = `uname -m` # for mac, the command is `uname` # "Darwin\n"; try `uname -a`
case system_arch.gsub("\n","")
system_arch.delete!("\n")
case system_arch
when 'x86_64'
swo_arch = 'x86_64'
when 'aarch64'
Expand Down
2 changes: 1 addition & 1 deletion lib/solarwinds_apm.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
SolarWindsAPM::Config.load_config_file
SolarWindsAPM.loaded = false
begin
if RUBY_PLATFORM =~ /linux/
if /linux/.match?(RUBY_PLATFORM)
require_relative './libsolarwinds_apm.so'
require 'solarwinds_apm/oboe_init_options'
require_relative './oboe_metal' # initialize Reporter; sets SolarWindsAPM.loaded = true if successful
Expand Down
5 changes: 2 additions & 3 deletions lib/solarwinds_apm/api/current_trace_info.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ def current_trace_info
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}))$/
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
private_constant :REGEXP

def initialize
Expand Down Expand Up @@ -98,8 +98,7 @@ def for_log
# * Hash
#
def hash_for_log
@hash_for_log = {}
@hash_for_log = {trace_id: @trace_id, span_id: @span_id, trace_flags: @trace_flags, service_name: @service_name} if @do_log
@hash_for_log = @do_log ? {'trace_id' => @trace_id, 'span_id' => @span_id, 'trace_flags' => @trace_flags, 'resource.service.name' => @service_name} : {}
Copy link
Contributor

@cheempz cheempz Nov 24, 2023

Choose a reason for hiding this comment

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

Nice, so seems this also fixes the minor problem you mentioned while reviewing the prerelease doc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it fixed that issue.

end

private
Expand Down
2 changes: 1 addition & 1 deletion lib/solarwinds_apm/config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ def self.config_from_env
end

def self.set_log_level
SolarWindsAPM::Config[:debug_level] = 3 unless (-1..6).include?(SolarWindsAPM::Config[:debug_level])
SolarWindsAPM::Config[:debug_level] = 3 unless (-1..6).cover?(SolarWindsAPM::Config[:debug_level])

# let's find and use the equivalent debug level for ruby
debug_level = (ENV['SW_APM_DEBUG_LEVEL'] || SolarWindsAPM::Config[:debug_level] || 3).to_i
Expand Down
28 changes: 15 additions & 13 deletions lib/solarwinds_apm/oboe_init_options.rb
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ def initialize
# custom token bucket rate
@token_bucket_rate = (ENV['SW_APM_TOKEN_BUCKET_RATE'] || -1).to_i
# use single files in file reporter for each event
@file_single = ENV['SW_APM_REPORTER_FILE_SINGLE'].to_s.downcase == 'true' ? 1 : 0
@file_single = ENV['SW_APM_REPORTER_FILE_SINGLE'].to_s.casecmp('true').zero? ? 1 : 0
# timeout for ec2 metadata
@ec2_md_timeout = read_and_validate_ec2_md_timeout
@grpc_proxy = read_and_validate_proxy
Expand Down Expand Up @@ -135,6 +135,7 @@ def read_and_validate_service_key
token = match[1]
service_name = match[3]

puts "validate_token(token): #{validate_token(token)}"
return '' unless validate_token(token) # return if token is not even valid

if service_name.empty?
Expand Down Expand Up @@ -170,8 +171,9 @@ def read_and_validate_service_key
"#{token}:#{service_name}"
end

# In case of java-collector, please provide a dummy service key
def validate_token(token)
if (token !~ /^[0-9a-zA-Z_-]{71}$/) && ENV['SW_APM_COLLECTOR'] !~ /java-collector:1222/
unless /^[0-9a-zA-Z_-]{71}$/.match?(token)
masked = "#{token[0..3]}...#{token[-4..]}"
SolarWindsAPM.logger.error {"[#{self.class}/#{__method__}] SW_APM_SERVICE_KEY problem. API Token in wrong format. Masked token: #{masked}"}
return false
Expand All @@ -181,20 +183,19 @@ def validate_token(token)
end

def validate_transform_service_name(service_name)
service_name = 'test_ssl_collector' if ENV['SW_APM_COLLECTOR'] =~ /java-collector:1222/
if service_name.empty?
SolarWindsAPM.logger.error {"[#{self.class}/#{__method__}] SW_APM_SERVICE_KEY problem. Service Name is missing"}
return false
end

name = service_name.dup
name.downcase!
name.gsub!(/[^a-z0-9.:_-]/, '')
name = name[0..254]
name_ = service_name.dup
name_.downcase!
name_.gsub!(/[^a-z0-9.:_-]/, '')
name_ = name_[0..254]

if name != service_name
SolarWindsAPM.logger.warn {"[#{self.class}/#{__method__}] SW_APM_SERVICE_KEY problem. Service Name transformed from #{service_name} to #{name}"}
service_name = name
if name_ != service_name
SolarWindsAPM.logger.warn {"[#{self.class}/#{__method__}] SW_APM_SERVICE_KEY problem. Service Name transformed from #{service_name} to #{name_}"}
service_name = name_
end
@service_name = service_name # instance variable used in testing
true
Expand All @@ -212,7 +213,7 @@ def read_and_validate_proxy
proxy = ENV['SW_APM_PROXY'] || SolarWindsAPM::Config[:http_proxy] || ''
return proxy if proxy == ''

unless proxy =~ /http:\/\/.*:\d+$/
unless /http:\/\/.*:\d+$/.match?(proxy)
SolarWindsAPM.logger.error {"[#{self.class}/#{__method__}] SW_APM_PROXY/http_proxy doesn't start with 'http://', #{proxy}"}
return '' # try without proxy, it may work, shouldn't crash but may not report
end
Expand All @@ -221,14 +222,15 @@ def read_and_validate_proxy
end

def read_certificates
certificate = ''

file = appoptics_collector?? "#{__dir__}/cert/star.appoptics.com.issuer.crt" : ENV['SW_APM_TRUSTEDPATH']
return String.new if file.nil? || file&.empty?
return certificate if file.nil? || file&.empty?

begin
certificate = File.open(file,"r").read
rescue StandardError => e
SolarWindsAPM.logger.error {"[#{self.class}/#{__method__}] certificates: #{file} doesn't exist or caused by #{e.message}."}
certificate = String.new
end

certificate
Expand Down
2 changes: 1 addition & 1 deletion lib/solarwinds_apm/opentelemetry/solarwinds_exporter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ def add_instrumented_framework(event, span_data)
# helper function that extract gem library version for func add_instrumented_framework
def check_framework_version(framework)
framework_version = nil
if @version_cache.keys.include? framework
if @version_cache.has_key?(framework)

framework_version = @version_cache[framework]
else
Expand Down
3 changes: 2 additions & 1 deletion lib/solarwinds_apm/opentelemetry/solarwinds_sampler.rb
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,8 @@ def add_tracestate_capture_to_new_attributes(new_attributes, liboboe_decision, t
SolarWindsAPM.logger.debug {"[#{self.class}/#{__method__}] trace_state_no_response #{trace_state_no_response.inspect}"}

trace_state_no_response = parent_span_context.tracestate.delete(XTraceOptions.sw_xtraceoptions_response_key)
no_sw_count = trace_state_no_response.to_h.reject { |k, _v| k == "sw" }.count
no_sw_count = trace_state_no_response.to_h.count { |k, _v| k != 'sw' }

new_attributes[SW_TRACESTATE_CAPTURE_KEY] = Utils.trace_state_header(trace_state_no_response) if no_sw_count > 0
SolarWindsAPM.logger.debug {"[#{self.class}/#{__method__}] new_attributes after add_tracestate_capture_to_new_attributes: #{new_attributes.inspect}"}

Expand Down
2 changes: 1 addition & 1 deletion lib/solarwinds_apm/support/logger_formatter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ def call(severity, time, progname, msg)
private

def insert_trace_id(msg)
return msg if msg =~ /trace_id=/
return msg if /trace_id=/.match?(msg)

current_trace = SolarWindsAPM::API.current_trace_info
if current_trace.do_log
Expand Down
4 changes: 2 additions & 2 deletions lib/solarwinds_apm/support/swomarginalia/comment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ def self.update_adapter!(adapter)
end

def self.construct_comment
ret = String.new
ret = +''
components.each do |c|
component_value = send(c)
ret << "#{c}='#{component_value}'," if component_value.present?
Expand Down Expand Up @@ -107,7 +107,7 @@ def self.sidekiq_job
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}
DEFAULT_LINES_TO_IGNORE_REGEX = %r{\.rvm|/ruby/gems/|vendor/|marginalia|rbenv|monitor\.rb.*mon_synchronize}.freeze

def self.line
SWOMarginalia::Comment.lines_to_ignore ||= DEFAULT_LINES_TO_IGNORE_REGEX
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ def record_query_comment

def self.with_annotation(comment, &block)
SWOMarginalia::Comment.inline_annotations.push(comment)
block.call if block.present?
yield if block.present?
ensure
SWOMarginalia::Comment.inline_annotations.pop
end
Expand Down