RuboCop: address some AbcSize issues #1849

fallwith · 2023-03-01T00:54:30Z

Address some Metrics/AbcSize issues to get our maximum ABC size down closer towards a reasonable value

helps with #1520

Address some Metrics/AbcSize issues to get our maximum ABC size down closer towards a reasonable value

after refactoring to use 2 methods, the JS to inject needs to be passed to the second method. also the headers aren't used, so grab that var as `_headers`.

…ast_stick_shift

hannahramadan · 2023-03-01T21:32:47Z

lib/new_relic/agent/agent_helpers/start_worker_thread.rb

+          end
+        end
+
+        def establish_fire_everies(data_harvest)


fun method name lol everies

hannahramadan · 2023-03-01T21:44:18Z

lib/new_relic/control/frameworks/rails.rb

+                                         "#{e.class} - #{e.message}")
+        end
+
+        def install_dependency_detection


Love turning that comment into a method!

hannahramadan · 2023-03-01T23:41:49Z

lib/new_relic/rack/browser_monitoring.rb

        # 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.')


Why add\ in the debug message?

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.

Oh just a length thing! Cool thank you

kaylareopelle

Thanks for taking care of this! I have a few suggestions/comments.

Did you happen to check SimpleCov for the updated files?

I ask because I think I may have forgotten last time I worked on a ticket like this and accidentally created a bug.

kaylareopelle · 2023-03-02T00:24:59Z

lib/new_relic/agent/agent_helpers/start_worker_thread.rb

+        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


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?

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.

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.

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

kaylareopelle · 2023-03-02T00:28:23Z

lib/new_relic/cli/commands/deployments.rb

+      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 }


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?

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.

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

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

lib/new_relic/rack/browser_monitoring.rb

stop passing an argument that is ultimately not used to a method

github-actions · 2023-03-02T01:27:19Z

SimpleCov Report

	Coverage	Threshold
Line	93.97%	93%
Branch	85.5%	84%

tannalynn · 2023-03-02T18:37:53Z

lib/new_relic/agent/agent_helpers/start_worker_thread.rb

+        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


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.

fallwith added 3 commits February 28, 2023 16:53

RuboCop: address some AbcSize issues

ffd61c5

Address some Metrics/AbcSize issues to get our maximum ABC size down closer towards a reasonable value

browser monitoring: refactoring fix

a6d1cec

after refactoring to use 2 methods, the JS to inject needs to be passed to the second method. also the headers aren't used, so grab that var as `_headers`.

Merge branch 'dev' into the_last_paperback_found_in_the_back_of_the_l…

56f45fe

…ast_stick_shift

hannahramadan reviewed Mar 1, 2023

View reviewed changes

hannahramadan previously approved these changes Mar 1, 2023

View reviewed changes

kaylareopelle previously approved these changes Mar 2, 2023

View reviewed changes

browser monitoring: don't pass unused param

3666602

stop passing an argument that is ultimately not used to a method

fallwith dismissed stale reviews from kaylareopelle and hannahramadan via 3666602 March 2, 2023 01:16

tannalynn approved these changes Mar 2, 2023

View reviewed changes

kaylareopelle approved these changes Mar 2, 2023

View reviewed changes

fallwith merged commit ce4e64c into dev Mar 2, 2023

fallwith deleted the the_last_paperback_found_in_the_back_of_the_last_stick_shift branch March 2, 2023 19:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RuboCop: address some AbcSize issues #1849

RuboCop: address some AbcSize issues #1849

fallwith commented Mar 1, 2023

hannahramadan Mar 1, 2023

hannahramadan Mar 1, 2023

hannahramadan Mar 1, 2023

fallwith Mar 2, 2023

hannahramadan Mar 2, 2023

kaylareopelle left a comment

kaylareopelle Mar 2, 2023

fallwith Mar 2, 2023

tannalynn Mar 2, 2023

kaylareopelle Mar 2, 2023

kaylareopelle Mar 2, 2023

fallwith Mar 2, 2023

hannahramadan Mar 2, 2023

kaylareopelle Mar 2, 2023

github-actions bot commented Mar 2, 2023

tannalynn Mar 2, 2023

RuboCop: address some AbcSize issues #1849

RuboCop: address some AbcSize issues #1849

Conversation

fallwith commented Mar 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaylareopelle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 2, 2023

SimpleCov Report

Choose a reason for hiding this comment