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.

Sign up for GitHub

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

RuboCop: address some AbcSize issues #1849

Merged
merged 4 commits into from
Mar 2, 2023
Merged

RuboCop: address some AbcSize issues #1849

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ffd61c5
RuboCop: address some AbcSize issues
fallwith Mar 1, 2023
a6d1cec
browser monitoring: refactoring fix
fallwith Mar 1, 2023
56f45fe
Merge branch 'dev' into the_last_paperback_found_in_the_back_of_the_l…
fallwith Mar 1, 2023
3666602
browser monitoring: don't pass unused param
fallwith Mar 2, 2023
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
5 changes: 3 additions & 2 deletions .rubocop_todo.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,11 @@
# Offense count: 422
# Configuration parameters: AllowedMethods, AllowedPatterns, IgnoredMethods, CountRepeatedAttributes.
Metrics/AbcSize:
Max: 54
Max: 43
Exclude:
- test/**/*
- infinite_tracing/test/**/*
- lib/new_relic/cli/commands/deployments.rb
- test/**/*

# This cop supports safe autocorrection (--autocorrect).
Minitest/AssertInDelta:
Expand Down
60 changes: 34 additions & 26 deletions lib/new_relic/agent/agent_helpers/start_worker_thread.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,36 +60,13 @@ def interval_for(event_type)
end

def create_and_run_event_loop
data_harvest = :"#{Agent.config[:data_report_period]}_second_harvest"
event_harvest = :"#{Agent.config[:event_report_period]}_second_harvest"

@event_loop = create_event_loop
data_harvest = :"#{Agent.config[:data_report_period]}_second_harvest"
@event_loop.on(data_harvest) do
transmit_data
end

@event_loop.on(interval_for(TRANSACTION_EVENT_DATA)) do
transmit_analytic_event_data
end
@event_loop.on(interval_for(CUSTOM_EVENT_DATA)) do
transmit_custom_event_data
end
@event_loop.on(interval_for(ERROR_EVENT_DATA)) do
transmit_error_event_data
end
@event_loop.on(interval_for(SPAN_EVENT_DATA)) do
transmit_span_event_data
end
@event_loop.on(interval_for(LOG_EVENT_DATA)) do
transmit_log_event_data
end

@event_loop.on(:reset_log_once_keys) do
::NewRelic::Agent.logger.clear_already_logged
end
@event_loop.fire_every(Agent.config[:data_report_period], data_harvest)
@event_loop.fire_every(Agent.config[:event_report_period], event_harvest)
@event_loop.fire_every(LOG_ONCE_KEYS_RESET_PERIOD, :reset_log_once_keys)
establish_interval_transmissions
establish_fire_everies(data_harvest)

@event_loop.run
end
Expand Down Expand Up @@ -161,6 +138,37 @@ def deferred_work!(connection_options)
end
end
end

private

def establish_interval_transmissions
@event_loop.on(interval_for(TRANSACTION_EVENT_DATA)) do
transmit_analytic_event_data
end
@event_loop.on(interval_for(CUSTOM_EVENT_DATA)) do
transmit_custom_event_data
end
@event_loop.on(interval_for(ERROR_EVENT_DATA)) do
transmit_error_event_data
end
@event_loop.on(interval_for(SPAN_EVENT_DATA)) do
transmit_span_event_data
end
@event_loop.on(interval_for(LOG_EVENT_DATA)) do
transmit_log_event_data
end
Comment on lines +144 to +159
Copy link
Contributor

Choose a reason for hiding this comment

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

This still feels like a decent amount of duplication. Would it work to put tbe *_EVENT_DATA constants into an array and loop through them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The short answer for why I didn't do so is that I'm trying to very tightly scope this PR's changes to RuboCop related things. I brought in changes in line length and quotes because there are cops for those things, but I didn't want to do any refactoring to the code beyond that.

For a summary of a longer answer, that thought did come to mind in this section of the code but each constant is mapped to a different method call and the names don't all follow a common convention. For example: TRANSACTION_EVENT_DATA maps to transmit_analytic_event_data which breaks the pattern of transmit_<CONSTANT.downcase>. I also did not want to dive into a deep understanding of the requirements of what needs to take place in the context of an event loops doblock. It's possible that something likesend(methodize(CONSTANT)` wouldn't work the same way. So I stuck to just satisfying the cop.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that makes sense to me. One thought I had is that in the future if we want to further refactor this we could put the constants into a hash that has the methods in it, that way we don't have to try to guess or interpolate what the method name is supposed to be.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! I totally agree with making tightly scoped changes. This is great as-is.

end

def establish_fire_everies(data_harvest)
Copy link
Contributor

Choose a reason for hiding this comment

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

fun method name lol everies

@event_loop.on(:reset_log_once_keys) do
::NewRelic::Agent.logger.clear_already_logged
end

event_harvest = :"#{Agent.config[:event_report_period]}_second_harvest"
@event_loop.fire_every(Agent.config[:data_report_period], data_harvest)
@event_loop.fire_every(Agent.config[:event_report_period], event_harvest)
@event_loop.fire_every(LOG_ONCE_KEYS_RESET_PERIOD, :reset_log_once_keys)
end
end
end
end
Expand Down
38 changes: 19 additions & 19 deletions lib/new_relic/agent/configuration/default_source.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,35 +80,35 @@ def self.transform_for(key)
default_settings[:transform] if default_settings
end

def self.config_search_paths
def self.config_search_paths # rubocop:disable Metrics/AbcSize
proc {
paths = [
File.join("config", "newrelic.yml"),
File.join("newrelic.yml"),
File.join("config", "newrelic.yml.erb"),
File.join("newrelic.yml.erb")
]
yaml = 'newrelic.yml'
config_yaml = File.join('config', yaml)
erb = 'newrelic.yml.erb'
config_erb = File.join('config', erb)

paths = [config_yaml, yaml, config_erb, erb]

if NewRelic::Control.instance.root
paths << File.join(NewRelic::Control.instance.root, "config", "newrelic.yml")
paths << File.join(NewRelic::Control.instance.root, "newrelic.yml")
paths << File.join(NewRelic::Control.instance.root, "config", "newrelic.yml.erb")
paths << File.join(NewRelic::Control.instance.root, "newrelic.yml.erb")
paths << File.join(NewRelic::Control.instance.root, config_yaml)
paths << File.join(NewRelic::Control.instance.root, yaml)
paths << File.join(NewRelic::Control.instance.root, config_erb)
paths << File.join(NewRelic::Control.instance.root, erb)
end

if ENV['HOME']
paths << File.join(ENV['HOME'], ".newrelic", "newrelic.yml")
paths << File.join(ENV['HOME'], "newrelic.yml")
paths << File.join(ENV['HOME'], ".newrelic", "newrelic.yml.erb")
paths << File.join(ENV['HOME'], "newrelic.yml.erb")
paths << File.join(ENV['HOME'], '.newrelic', yaml)
paths << File.join(ENV['HOME'], yaml)
paths << File.join(ENV['HOME'], '.newrelic', erb)
paths << File.join(ENV['HOME'], erb)
end

# If we're packaged for warbler, we can tell from GEM_HOME
# the following line needs else branch coverage
if ENV["GEM_HOME"] && ENV["GEM_HOME"].end_with?(".jar!") # rubocop:disable Style/SafeNavigation
app_name = File.basename(ENV["GEM_HOME"], ".jar!")
paths << File.join(ENV["GEM_HOME"], app_name, "config", "newrelic.yml")
paths << File.join(ENV["GEM_HOME"], app_name, "config", "newrelic.yml.erb")
if ENV['GEM_HOME'] && ENV['GEM_HOME'].end_with?(".jar!") # rubocop:disable Style/SafeNavigation
app_name = File.basename(ENV['GEM_HOME'], '.jar!')
paths << File.join(ENV['GEM_HOME'], app_name, config_yaml)
paths << File.join(ENV['GEM_HOME'], app_name, config_erb)
end

paths
Expand Down
20 changes: 12 additions & 8 deletions lib/new_relic/agent/transaction.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,15 +734,19 @@ def merge_metrics
def record_exceptions
error_recorded = false
@exceptions.each do |exception, options|
options[:uri] ||= request_path if request_path
options[:port] = request_port if request_port
options[:metric] = best_name
options[:attributes] = @attributes

span_id = options.delete(:span_id)
error_recorded = !!agent.error_collector.notice_error(exception, options, span_id) || error_recorded
error_recorded = record_exception(exception, options, error_recorded)
end
payload[:error] = error_recorded if payload
payload&.[]=(:error, error_recorded)
end

def record_exception(exception, options, error_recorded)
options[:uri] ||= request_path if request_path
options[:port] = request_port if request_port
options[:metric] = best_name
options[:attributes] = @attributes

span_id = options.delete(:span_id)
!!agent.error_collector.notice_error(exception, options, span_id) || error_recorded
end

# Do not call this. Invoke the class method instead.
Expand Down
32 changes: 16 additions & 16 deletions lib/new_relic/cli/commands/deployments.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,24 +163,24 @@ def set_params_v2(request)
def options
OptionParser.new(%Q(Usage: #{$0} #{self.class.command} [OPTIONS] ["description"] ), 40) do |o|
o.separator("OPTIONS:")
o.on("-a", "--appname=NAME", String,
"Set the application name.",
"Default is app_name setting in newrelic.yml. Available only when using API v1.") { |e| @appname = e }
o.on("-i", "--appid=ID", String,
"Set the application ID",
"If not provided, will connect to the New Relic collector to get it") { |i| @application_id = i }
o.on("-e", "--environment=name", String,
"Override the (RAILS|RUBY|RACK)_ENV setting",
o.on('-a', '--appname=NAME', String,
'Set the application name.',
'Default is app_name setting in newrelic.yml. Available only when using API v1.') { |e| @appname = e }
o.on('-i', '--appid=ID', String,
'Set the application ID',
'If not provided, will connect to the New Relic collector to get it') { |i| @application_id = i }
o.on('-e', '--environment=name', String,
'Override the (RAILS|RUBY|RACK)_ENV setting',
"currently: #{control.env}") { |e| @environment = e }
o.on("-u", "--user=USER", String,
"Specify the user deploying, for information only",
o.on('-u', '--user=USER', String,
'Specify the user deploying, for information only',
"Default: #{@user || '<none>'}") { |u| @user = u }
o.on("-r", "--revision=REV", String,
"Specify the revision being deployed. Required when using New Relic REST API v2") { |r| @revision = r }
o.on("-l", "--license-key=KEY", String,
"Specify the license key of the account for the app being deployed") { |l| @license_key = l }
o.on("-c", "--changes",
"Read in a change log from the standard input") { @changelog = STDIN.read }
o.on('-r', '--revision=REV', String,
'Specify the revision being deployed. Required when using New Relic REST API v2') { |r| @revision = r }
o.on('-l', '--license-key=KEY', String,
'Specify the license key of the account for the app being deployed') { |l| @license_key = l }
o.on('-c', '--changes',
'Read in a change log from the standard input') { @changelog = STDIN.read }
Copy link
Contributor

Choose a reason for hiding this comment

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

I love seeing these single quotes.

Isn't there a built-in linter for changing double quotes to singles for areas without interpolation? I wonder if that might be worth turning on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can tell RuboCop to insist on the use of single quotes for strings without interpolation or you can tell RuboCop to insist on double quotes everywhere. I believe RuboCop currently defaults to the first one of those, but there has been well reasoned arguments for the second approach as well.

I believe we are currently disabling that cop and allowing any kind of quotes at any time regardless of interpolation.

If the devs decide on a standard, we can update the RuboCop config accordingly. Until then, I've been operating with my personal preference of RuboCop defaults.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the record! I prefer single quotes. Double quotes alert me to interpolation, so I'd love to see this single quote cop enabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

My preference is also single quotes unless in interpolation, though it can get hairy if we have apostrophes in our strings.

yield(o) if block_given?
end
end
Expand Down
40 changes: 26 additions & 14 deletions lib/new_relic/control/frameworks/rails.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,26 +44,38 @@ def rails_config
# find a config and use that.
def init_config(options = {})
@config = options[:config]
# Install the dependency detection,
if rails_config && ::Rails.configuration.respond_to?(:after_initialize)
rails_config.after_initialize do
# This will insure we load all the instrumentation as late as possible. If the agent
# is not enabled, it will load a limited amount of instrumentation.
DependencyDetection.detect!
end
install_dependency_detection
install_browser_monitoring_and_agent_hooks
rescue => e
::NewRelic::Agent.logger.error('Failure during init_config for Rails. Is Rails required in a non-Rails ' \
'app? Set NEW_RELIC_FRAMEWORK=ruby to avoid this message. The Ruby agent ' \
'will continue running, but Rails-specific features may be missing. ' \
"#{e.class} - #{e.message}")
end

def install_dependency_detection
Copy link
Contributor

Choose a reason for hiding this comment

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

Love turning that comment into a method!

return unless rails_config && ::Rails.configuration.respond_to?(:after_initialize)

rails_config.after_initialize do
# This will insure we load all the instrumentation as late as
# possible. If the agent is not enabled, it will load a limited
# amount of instrumentation.
DependencyDetection.detect!
end
end

def install_browser_monitoring_and_agent_hooks
return unless rails_config

if !Agent.config[:agent_enabled]
# Might not be running if it does not think mongrel, thin, passenger, etc
# is running, if it thinks it's a rake task, or if the agent_enabled is false.
::NewRelic::Agent.logger.info("New Relic Agent not running.")
# Might not be running if it does not think mongrel, thin,
# passenger, etc. is running, if it thinks it's a rake task, or
# if the agent_enabled is false.
::NewRelic::Agent.logger.info('New Relic Agent not running. Skipping browser monitoring and agent hooks.')
else
install_browser_monitoring(rails_config)
install_agent_hooks(rails_config)
end
rescue => e
::NewRelic::Agent.logger.error("Failure during init_config for Rails. Is Rails required in a non-Rails app? Set NEW_RELIC_FRAMEWORK=ruby to avoid this message.",
"The Ruby agent will continue running, but Rails-specific features may be missing.",
e)
end

def install_agent_hooks(config)
Expand Down
19 changes: 11 additions & 8 deletions lib/new_relic/rack/browser_monitoring.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def traced_call(env)

js_to_inject = NewRelic::Agent.browser_timing_header
if (js_to_inject != NewRelic::EMPTY_STR) && should_instrument?(env, status, headers)
response_string = autoinstrument_source(response, headers, js_to_inject)
response_string = autoinstrument_source(response, js_to_inject)
if headers.key?(CONTENT_LENGTH)
content_length = response_string ? response_string.bytesize : 0
headers[CONTENT_LENGTH] = content_length.to_s
Expand Down Expand Up @@ -69,30 +69,33 @@ def should_instrument?(env, status, headers)

private

def autoinstrument_source(response, headers, js_to_inject)
def autoinstrument_source(response, js_to_inject)
source = gather_source(response)
close_old_response(response)
return nil unless source
return unless source

modify_source(source, js_to_inject)
rescue => e
NewRelic::Agent.logger.debug("Skipping RUM instrumentation on exception: #{e.class} - #{e.message}")
end

def modify_source(source, js_to_inject)
# Only scan the first 50k (roughly) then give up.
beginning_of_source = source[0..SCAN_LIMIT]
meta_tag_positions = find_meta_tag_positions(beginning_of_source)
if body_start = find_body_start(beginning_of_source)
if insertion_index = find_insertion_index(meta_tag_positions, beginning_of_source, body_start)
source = source_injection(source, insertion_index, js_to_inject)
else
NewRelic::Agent.logger.debug("Skipping RUM instrumentation. Could not properly determine location to inject script.")
NewRelic::Agent.logger.debug('Skipping RUM instrumentation. Could not properly determine location to ' \
'inject script.')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why add\ in the debug message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, so while I was fixing the AbcSize issue I decided to also address another RuboCop issue with this code block to do with line length. By using backslashes, I just broke the super long debug text up into 120 column chunks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh just a length thing! Cool thank you

end
else
msg = "Skipping RUM instrumentation. Unable to find <body> tag in first #{SCAN_LIMIT} bytes of document."
NewRelic::Agent.logger.log_once(:warn, :rum_insertion_failure, msg)
NewRelic::Agent.logger.debug(msg)
end

source
rescue => e
NewRelic::Agent.logger.debug("Skipping RUM instrumentation on exception.", e)
nil
end

def html?(headers)
Expand Down