From 61b4a2ee0c75c1c36141a9aaec5b0cb5d7d07f66 Mon Sep 17 00:00:00 2001 From: fallwith Date: Sat, 26 Aug 2023 00:02:11 -0700 Subject: [PATCH 01/16] Attribute pre-filtering, Sidekiq args filtering - Added pre-filtering logic to the `Attribute::Processing` class, currently used only by Sidekiq - Added new `sidekiq.args.include` and `sidekiq.args.exclude` configuration options to permit the capturing of a subset of Sidekiq job arguments - Rewrote all existing Sidekiq multiverse tests. The suite should now complete in ~4 seconds instead of 32 and doesn't require Mocha. --- .rubocop.yml | 1 + lib/new_relic/agent/attribute_processing.rb | 97 ++++++ .../agent/configuration/default_source.rb | 16 + .../agent/instrumentation/sidekiq/server.rb | 26 +- test/helpers/config_scanning.rb | 8 + test/multiverse/suites/sidekiq/after_suite.rb | 16 - .../sidekiq/sidekiq_args_filtration_test.rb | 98 ++++++ .../sidekiq/sidekiq_instrumentation_test.rb | 284 ++---------------- .../suites/sidekiq/sidekiq_server.rb | 47 --- .../suites/sidekiq/sidekiq_test_helpers.rb | 80 +++++ test/multiverse/suites/sidekiq/test_model.rb | 12 - test/multiverse/suites/sidekiq/test_worker.rb | 78 ----- .../agent/attribute_processing_test.rb | 234 +++++++++++++++ 13 files changed, 590 insertions(+), 407 deletions(-) delete mode 100644 test/multiverse/suites/sidekiq/after_suite.rb create mode 100644 test/multiverse/suites/sidekiq/sidekiq_args_filtration_test.rb delete mode 100644 test/multiverse/suites/sidekiq/sidekiq_server.rb create mode 100644 test/multiverse/suites/sidekiq/sidekiq_test_helpers.rb delete mode 100644 test/multiverse/suites/sidekiq/test_model.rb delete mode 100644 test/multiverse/suites/sidekiq/test_worker.rb diff --git a/.rubocop.yml b/.rubocop.yml index a6e4095d54..f2d67bc47b 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1259,6 +1259,7 @@ Style/MethodCallWithArgsParentheses: - raise - require - skip + - sleep - source - stub - stub_const diff --git a/lib/new_relic/agent/attribute_processing.rb b/lib/new_relic/agent/attribute_processing.rb index e23a145c9a..ba31882bb7 100644 --- a/lib/new_relic/agent/attribute_processing.rb +++ b/lib/new_relic/agent/attribute_processing.rb @@ -9,6 +9,8 @@ module AttributeProcessing EMPTY_HASH_STRING_LITERAL = '{}'.freeze EMPTY_ARRAY_STRING_LITERAL = '[]'.freeze + PRE_FILTER_KEYS = %i[include exclude].freeze + DISCARDED = :nr_discarded def flatten_and_coerce(object, prefix = nil, result = {}, &blk) if object.is_a?(Hash) @@ -57,6 +59,101 @@ def flatten_and_coerce_array(array, prefix, result, &blk) end end end + + def formulate_regexp_union(option) + return if NewRelic::Agent.config[option].empty? + + Regexp.union(NewRelic::Agent.config[option].map { |p| string_to_regexp(p) }.uniq.compact).freeze + rescue StandardError => e + NewRelic::Agent.logger.warn("Failed to formulate a Regexp union from the '#{option}' configuration option " + + "- #{e.class}: #{e.message}") + end + + def string_to_regexp(str) + Regexp.new(str) + rescue StandardError => e + NewRelic::Agent.logger.warn("Failed to initialize Regexp from string '#{str}' - #{e.class}: #{e.message}") + end + + # attribute filtering suppresses data that has already been flattened + # and coerced (serialized as text) via #flatten_and_coerce, and is + # restricted to basic text matching with a single optional wildcard. + # pre filtering operates on raw Ruby objects beforehand and uses full + # Ruby regex syntax + def pre_filter(values = [], options = {}) + return values unless !options.empty? && PRE_FILTER_KEYS.any? { |k| options.key?(k) } + + # if there's a prefix in play for (non-pre) attribute filtration and + # attribute filtration won't allow that prefix, then don't even bother + # with pre filtration that could only result in values that would be + # blocked + if options.key?(:attribute_namespace) && + !NewRelic::Agent.instance.attribute_filter.might_allow_prefix?(options[:attribute_namespace]) + return values + end + + values.each_with_object([]) do |element, filtered| + object = pre_filter_object(element, options) + filtered << object unless discarded?(object) + end + end + + def pre_filter_object(object, options) + if object.is_a?(Hash) + pre_filter_hash(object, options) + elsif object.is_a?(Array) + pre_filter_array(object, options) + else + pre_filter_scalar(object, options) + end + end + + def pre_filter_hash(hash, options) + filtered_hash = hash.each_with_object({}) do |(key, value), filtered| + filtered_key = pre_filter_object(key, options) + next if discarded?(filtered_key) + + # If the key is permitted, skip include filtration for the value + # but still apply exclude filtration + if options.key?(:exclude) + exclude_only = options.dup + exclude_only.delete(:include) + filtered_value = pre_filter_object(value, exclude_only) + next if discarded?(filtered_value) + else + filtered_value = value + end + + filtered[filtered_key] = filtered_value + end + + filtered_hash.empty? && !hash.empty? ? DISCARDED : filtered_hash + end + + def pre_filter_array(array, options) + filtered_array = array.each_with_object([]) do |element, filtered| + filtered_element = pre_filter_object(element, options) + next if discarded?(filtered_element) + + filtered.push(filtered_element) + end + + filtered_array.empty? && !array.empty? ? DISCARDED : filtered_array + end + + def pre_filter_scalar(scalar, options) + return DISCARDED if options.key?(:include) && !scalar.to_s.match?(options[:include]) + return DISCARDED if options.key?(:exclude) && scalar.to_s.match?(options[:exclude]) + + scalar + end + + # `nil`, empty enumerable objects, and `false` are all valid in their + # own right as application data, so pre-filtering uses a special value + # to indicate that filtered out data has been discarded + def discarded?(object) + object == DISCARDED + end end end end diff --git a/lib/new_relic/agent/configuration/default_source.rb b/lib/new_relic/agent/configuration/default_source.rb index 5ca0b0828f..eed9b32092 100644 --- a/lib/new_relic/agent/configuration/default_source.rb +++ b/lib/new_relic/agent/configuration/default_source.rb @@ -3,6 +3,7 @@ # frozen_string_literal: true require 'forwardable' +require_relative '../../constants' module NewRelic module Agent @@ -1709,6 +1710,21 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :transform => DefaultSource.method(:convert_to_regexp_list), :description => 'Define transactions you want the agent to ignore, by specifying a list of patterns matching the URI you want to ignore. For more detail, see [the docs on ignoring specific transactions](/docs/agents/ruby-agent/api-guides/ignoring-specific-transactions/#config-ignoring).' }, + # Sidekiq + :'sidekiq.args.include' => { + default: NewRelic::EMPTY_ARRAY, + public: true, + type: Array, + allowed_from_server: false, + description: "An array of strings that will collectively serve as an allowlist for filtering which Sidekiq job arguments get reported to New Relic. The capturing of any Sidekiq arguments requires that 'job.sidekiq.args.*' be added to the separate :'attributes.include' configuration option. Each string in this array will be turned into a regular expression via `Regexp.new` to permit advanced matching. For job argument hashes, if either a key or value matches the pair will be included. All matching job argument array elements and job argument scalars will be included." + }, + :'sidekiq.args.exclude' => { + default: NewRelic::EMPTY_ARRAY, + public: true, + type: Array, + allowed_from_server: false, + description: "An array of strings that will collectively serve as a denylist for filtering which Sidekiq job arguments get reported to New Relic. The capturing of any Sidekiq arguments requires that 'job.sidekiq.args.*' be added to the separate :'attributes.include' configuration option. Each string in this array will be turned into a regular expression via `Regexp.new` to permit advanced matching. For job argument hashes, if either a key or value matches the pair will be excluded. All matching job argument array elements and job argument scalars will be excluded." + }, # Slow SQL :'slow_sql.enabled' => { :default => value_of(:'transaction_tracer.enabled'), diff --git a/lib/new_relic/agent/instrumentation/sidekiq/server.rb b/lib/new_relic/agent/instrumentation/sidekiq/server.rb index b15a2cc998..2bb56e900f 100644 --- a/lib/new_relic/agent/instrumentation/sidekiq/server.rb +++ b/lib/new_relic/agent/instrumentation/sidekiq/server.rb @@ -7,6 +7,10 @@ class Server include NewRelic::Agent::Instrumentation::ControllerInstrumentation include Sidekiq::ServerMiddleware if defined?(Sidekiq::ServerMiddleware) + ATTRIBUTE_BASE_NAMESPACE = 'sidekiq.args' + ATTRIBUTE_FILTER_TYPES = %i[include exclude].freeze + ATTRIBUTE_JOB_NAMESPACE = :"job.#{ATTRIBUTE_BASE_NAMESPACE}" + # Client middleware has additional parameters, and our tests use the # middleware client-side to work inline. def call(worker, msg, queue, *_) @@ -18,10 +22,16 @@ def call(worker, msg, queue, *_) trace_headers = msg.delete(NewRelic::NEWRELIC_KEY) perform_action_with_newrelic_trace(trace_args) do - NewRelic::Agent::Transaction.merge_untrusted_agent_attributes(msg['args'], :'job.sidekiq.args', - NewRelic::Agent::AttributeFilter::DST_NONE) + NewRelic::Agent::Transaction.merge_untrusted_agent_attributes( + NewRelic::Agent::AttributeProcessing.pre_filter(msg['args'], self.class.nr_attribute_options), + ATTRIBUTE_JOB_NAMESPACE, + NewRelic::Agent::AttributeFilter::DST_NONE + ) + + if ::NewRelic::Agent.config[:'distributed_tracing.enabled'] && trace_headers&.any? + ::NewRelic::Agent::DistributedTracing::accept_distributed_trace_headers(trace_headers, 'Other') + end - ::NewRelic::Agent::DistributedTracing::accept_distributed_trace_headers(trace_headers, 'Other') if ::NewRelic::Agent.config[:'distributed_tracing.enabled'] && trace_headers&.any? yield end end @@ -33,5 +43,15 @@ def self.default_trace_args(msg) :category => 'OtherTransaction/SidekiqJob' } end + + def self.nr_attribute_options + @nr_attribute_options ||= begin + ATTRIBUTE_FILTER_TYPES.each_with_object({}) do |type, opts| + pattern = + NewRelic::Agent::AttributeProcessing.formulate_regexp_union(:"#{ATTRIBUTE_BASE_NAMESPACE}.#{type}") + opts[type] = pattern if pattern + end.merge(attribute_namespace: ATTRIBUTE_JOB_NAMESPACE) + end + end end end diff --git a/test/helpers/config_scanning.rb b/test/helpers/config_scanning.rb index cd5cdec149..10f20e90d9 100644 --- a/test/helpers/config_scanning.rb +++ b/test/helpers/config_scanning.rb @@ -15,6 +15,11 @@ module ConfigScanning EVENT_BUFFER_MACRO_PATTERN = /(capacity_key|enabled_key)\s+:(['"])?([a-z\._]+)\2?/ ASSIGNED_CONSTANT_PATTERN = /[A-Z]+\s*=\s*:(['"])?([a-z\._]+)\1?\s*/ + # These config settings shouldn't be worried about, possibly because they + # are only referenced via Ruby metaprogramming that won't work with this + # module's regex matching + IGNORED = %i[sidekiq.args.include sidekiq.args.exclude] + def scan_and_remove_used_entries(default_keys, non_test_files) non_test_files.each do |file| lines_in(file).each do |line| @@ -30,6 +35,9 @@ def scan_and_remove_used_entries(default_keys, non_test_files) captures.flatten.compact.each do |key| default_keys.delete(key.delete("'").to_sym) end + + IGNORED.each { |key| default_keys.delete(key) } + # Remove any config keys that are annotated with the 'dynamic_name' setting # This indicates that the names of these keys are constructed dynamically at # runtime, so we don't expect any explicit references to them in code. diff --git a/test/multiverse/suites/sidekiq/after_suite.rb b/test/multiverse/suites/sidekiq/after_suite.rb deleted file mode 100644 index 702d8b662d..0000000000 --- a/test/multiverse/suites/sidekiq/after_suite.rb +++ /dev/null @@ -1,16 +0,0 @@ -# This file is distributed under New Relic's license terms. -# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. -# frozen_string_literal: true - -module Sidekiq - class CLI - def exit(*args) - # No-op Sidekiq's exit since we don't want it shutting us down and eating - # our exit code - end - end -end - -if defined?(SidekiqServer) - SidekiqServer.instance.stop -end diff --git a/test/multiverse/suites/sidekiq/sidekiq_args_filtration_test.rb b/test/multiverse/suites/sidekiq/sidekiq_args_filtration_test.rb new file mode 100644 index 0000000000..02b622fd3c --- /dev/null +++ b/test/multiverse/suites/sidekiq/sidekiq_args_filtration_test.rb @@ -0,0 +1,98 @@ +# This file is distributed under New Relic's license terms. +# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. +# frozen_string_literal: true + +require_relative 'sidekiq_test_helpers' + +class SidekiqArgsFiltrationTest < Minitest::Test + include SidekiqTestHelpers + + ARGUMENTS = [{'username' => 'JBond007', + 'color' => 'silver', + 'record' => true, + 'items' => %w[stag thistle peat], + 'price_map' => {'apple' => 0.75, 'banana' => 0.50, 'pear' => 0.99}}, + 'When I thought I heard myself say no', + false].freeze + + def setup + cache_var = :@nr_attribute_options + if NewRelic::Agent::Instrumentation::Sidekiq::Server.instance_variables.include?(cache_var) + NewRelic::Agent::Instrumentation::Sidekiq::Server.remove_instance_variable(cache_var) + end + end + + def test_by_default_no_args_are_captured + captured_args = run_job_and_get_attributes(*ARGUMENTS) + + assert_empty captured_args, "Didn't expect to capture any attributes for the Sidekiq job, " + + "captured: #{captured_args}" + end + + def test_all_args_are_captured + expected = flatten(ARGUMENTS) + with_config(:'attributes.include' => 'job.sidekiq.args.*') do + captured_args = run_job_and_get_attributes(*ARGUMENTS) + + assert_equal expected, captured_args, "Expected all args to be captured. Wanted:\n\n#{expected}\n\n" + + "Got:\n\n#{captured_args}\n\n" + end + end + + def test_only_included_args_are_captured + included = ['price_map'] + expected = flatten([{included.first => ARGUMENTS.first[included.first]}]) + with_config(:'attributes.include' => 'job.sidekiq.args.*', + :'sidekiq.args.include' => included) do + captured_args = run_job_and_get_attributes(*ARGUMENTS) + + assert_equal expected, captured_args, "Expected only '#{included}' args to be captured. " + + "Wanted:\n\n#{expected}\n\nGot:\n\n#{captured_args}\n\n" + end + end + + def test_excluded_args_are_not_captured + excluded = ['username'] + without_excluded = ARGUMENTS.dup + without_excluded.first.delete(excluded.first) + expected = flatten(without_excluded) + with_config(:'attributes.include' => 'job.sidekiq.args.*', + :'sidekiq.args.exclude' => excluded) do + captured_args = run_job_and_get_attributes(*ARGUMENTS) + + assert_equal expected, captured_args, "Expected '#{excluded}' to be excluded from capture. " + + "Wanted:\n\n#{expected}\n\nGot:\n\n#{captured_args}\n\n" + end + end + + def test_include_and_exclude_cascaded + included = ['price_map'] + excluded = %w[apple pear] + hash = {included.first => ARGUMENTS.first[included.first].dup} + # TODO: OLD RUBIES - Requires 3.0 + # Hash#except would be better here, requires Ruby v3+ + excluded.each { |exclude| hash[included.first].delete(exclude) } + expected = flatten([hash]) + with_config(:'attributes.include' => 'job.sidekiq.args.*', + :'sidekiq.args.include' => included, + :'sidekiq.args.exclude' => excluded) do + captured_args = run_job_and_get_attributes(*ARGUMENTS) + + assert_equal expected, captured_args, "Used included='#{included}', excluded='#{excluded}'. " + + "Wanted:\n\n#{expected}\n\nGot:\n\n#{captured_args}\n\n" + end + end + + def test_arcane_pattern_usage + # no booleans, nothing with numbers, no *.name except unitname, anything ending in 't', a string with I, I, and y, y + excluded = ['^true|false$', '\d+', '(?! 'silver', 'items' => %w[stag thistle]}]) + with_config(:'attributes.include' => 'job.sidekiq.args.*', + :'sidekiq.args.exclude' => excluded) do + captured_args = run_job_and_get_attributes(*ARGUMENTS) + + assert_equal expected, captured_args, "Used excluded='#{excluded}'. " + + "Wanted:\n\n#{expected}\n\nGot:\n\n#{captured_args}\n\n" + end + end +end diff --git a/test/multiverse/suites/sidekiq/sidekiq_instrumentation_test.rb b/test/multiverse/suites/sidekiq/sidekiq_instrumentation_test.rb index 62a79c52cf..4b3ded74ad 100644 --- a/test/multiverse/suites/sidekiq/sidekiq_instrumentation_test.rb +++ b/test/multiverse/suites/sidekiq/sidekiq_instrumentation_test.rb @@ -2,273 +2,55 @@ # See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. # frozen_string_literal: true -# https://newrelic.atlassian.net/browse/RUBY-775 +require_relative 'sidekiq_test_helpers' -require File.join(File.dirname(__FILE__), 'sidekiq_server') -SidekiqServer.instance.run +class SidekiqInstrumentationTest < Minitest::Test + include SidekiqTestHelpers -# Important to require after Sidekiq server starts for middleware to install -require 'newrelic_rpm' + def test_running_a_job_produces_a_healthy_segment + # NOTE: run_job itself asserts that exactly 1 segment could be found + segment = run_job -require 'logger' -require 'stringio' - -require 'fake_collector' -require File.join(File.dirname(__FILE__), 'test_model') -require File.join(File.dirname(__FILE__), 'test_worker') - -class SidekiqTest < Minitest::Test - JOB_COUNT = 5 - - ROLLUP_METRIC = 'OtherTransaction/SidekiqJob/all' - TRANSACTION_NAME = 'OtherTransaction/SidekiqJob/TestWorker/perform' - DELAYED_TRANSACTION_NAME = 'OtherTransaction/SidekiqJob/TestModel/do_work' - DELAYED_FAILED_TXN_NAME = 'OtherTransaction/SidekiqJob/Sidekiq::Extensions::DelayedClass/perform' - - include MultiverseHelpers - - setup_and_teardown_agent do - TestWorker.register_signal('jobs_completed') - @sidekiq_log = ::StringIO.new - - string_logger = ::Logger.new(@sidekiq_log) - string_logger.formatter = Sidekiq.logger.formatter - set_sidekiq_logger(string_logger) - end - - def set_sidekiq_logger(logger) - if Sidekiq::VERSION >= '7.0.0' - Sidekiq.default_configuration.logger = logger - else - Sidekiq.logger = logger - end - end - - def teardown - teardown_agent - if !passed? || ENV['VERBOSE'] - @sidekiq_log.rewind - puts @sidekiq_log.read - end - end - - def run_jobs - run_and_transmit do |i| - TestWorker.perform_async('jobs_completed', i + 1) - end - end - - def run_delayed - run_and_transmit do |i| - TestModel.delay(:queue => SidekiqServer.instance.queue_name, :retry => false).do_work - end - end - - def run_and_transmit - with_config(:'transaction_tracer.transaction_threshold' => 0.0) do - TestWorker.run_jobs(JOB_COUNT) do |i| - yield(i) - end - end - - run_harvest + assert_predicate segment, :finished? + assert_predicate segment, :record_metrics? + assert segment.duration.is_a?(Float) + assert segment.start_time.is_a?(Float) + assert segment.end_time.is_a?(Float) + assert segment.time_range.is_a?(Range) end - if defined?(Sidekiq::VERSION) && Sidekiq::VERSION.split('.').first.to_i < 5 - def test_delayed - run_delayed - - assert_metric_and_call_count(ROLLUP_METRIC, JOB_COUNT) - assert_metric_and_call_count(DELAYED_TRANSACTION_NAME, JOB_COUNT) - end - - def test_delayed_with_malformed_yaml - YAML.stubs(:load).raises(RuntimeError.new('Ouch')) - run_delayed + def test_disributed_tracing_for_sidekiq + with_config('distributed_tracing.enabled': true, + account_id: '190', + primary_application_id: '46954', + trusted_account_key: 'trust_this!') do + NewRelic::Agent.agent.stub :connected?, true do + run_job - assert_metric_and_call_count(ROLLUP_METRIC, JOB_COUNT) - if RUBY_VERSION >= '3.0.0' - assert_metric_and_call_count(DELAYED_TRANSACTION_NAME, JOB_COUNT) - else - assert_metric_and_call_count(DELAYED_FAILED_TXN_NAME, JOB_COUNT) + assert_metrics_recorded 'Supportability/DistributedTrace/CreatePayload/Success' end end end - def test_all_jobs_ran - run_jobs - completed_jobs = Set.new(TestWorker.records_for('jobs_completed').map(&:to_i)) - expected_completed_jobs = Set.new((1..JOB_COUNT).to_a) - - assert_equal(expected_completed_jobs, completed_jobs) - end - - def test_distributed_trace_instrumentation - @config = { - :'distributed_tracing.enabled' => true, - :account_id => '190', - :primary_application_id => '46954', - :trusted_account_key => 'trust_this!' - } - NewRelic::Agent::DistributedTracePayload.stubs(:connected?).returns(true) - NewRelic::Agent.config.add_config_for_testing(@config) - - in_transaction('test_txn') do |t| - run_jobs - end - - assert_metric_and_call_count 'Supportability/TraceContext/Accept/Success', JOB_COUNT # method for metrics created on server side - assert_metrics_recorded 'Supportability/DistributedTrace/CreatePayload/Success' # method for metrics created on the client side - - NewRelic::Agent.config.remove_config(@config) - NewRelic::Agent.config.reset_to_defaults - NewRelic::Agent.drop_buffered_data - end - - def test_agent_posts_correct_metric_data - run_jobs - - assert_metric_and_call_count(ROLLUP_METRIC, JOB_COUNT) - assert_metric_and_call_count(TRANSACTION_NAME, JOB_COUNT) - end - - def test_doesnt_capture_args_by_default - stub_for_span_collection - - run_jobs - - refute_attributes_on_transaction_trace - refute_attributes_on_events - end - - def test_isnt_influenced_by_global_capture_params - stub_for_span_collection - - with_config(:capture_params => true) do - run_jobs - end - - refute_attributes_on_transaction_trace - refute_attributes_on_events - end - - def test_arguments_are_captured_on_transaction_and_span_events_when_enabled - stub_for_span_collection + def test_captures_errors_taking_place_during_the_processing_of_a_job + # TODO: MAJOR VERSION - remove this when Sidekiq v5 is no longer supported + skip 'Test requires Sidekiq v6+' unless Sidekiq::VERSION.split('.').first.to_i >= 6 - with_config(:'attributes.include' => 'job.sidekiq.args.*') do - run_jobs - end + segment = run_job('raise_error' => true) + noticed_error = segment.noticed_error - assert_attributes_on_transaction_trace - assert_attributes_on_events - end - - def test_captures_errors_from_job - TestWorker.fail = true - run_jobs - - assert_error_for_each_job - ensure - TestWorker.fail = false + assert noticed_error, 'Expected the segment to have a noticed error' + assert_equal NRDeadEndJob::ERROR_MESSAGE, noticed_error.message end def test_captures_sidekiq_internal_errors - # When testing internal Sidekiq error capturing, we're looking to - # ensure Sidekiq properly forwards errors to our custom error handler - # in order for us to notice the error. - - exception = StandardError.new('foo') - NewRelic::Agent.expects(:notice_error).with(exception) - Sidekiq::CLI.instance.handle_exception(exception) - end - - def test_accept_dt_headers_not_called_if_headers_nil - NewRelic::Agent::DistributedTracing.stubs(:insert_distributed_trace_headers) - NewRelic::Agent::DistributedTracing.expects(:accept_distributed_trace_headers).never - in_transaction do - run_jobs - end - end - - def assert_metric_and_call_count(name, expected_call_count) - metric_data = $collector.calls_for('metric_data') - - assert_equal(1, metric_data.size, 'expected exactly one metric_data post from agent') - - metrics = metric_data.first.metrics - metric = metrics.find { |m| m[0]['name'] == name } - message = "Could not find metric named #{name}. Did have metrics:\n" + metrics.map { |m| m[0]['name'] }.join("\t\n") - - assert(metric, message) - - call_count = metric[1][0] - - assert_equal(expected_call_count, call_count) - end - - def assert_attributes_on_transaction_trace - transaction_samples = $collector.calls_for('transaction_sample_data') - - refute_empty transaction_samples, 'Expected a transaction trace' - - transaction_samples.each do |post| - post.samples.each do |sample| - assert_equal sample.metric_name, TRANSACTION_NAME, "Huh, that transaction shouldn't be in there!" - - actual = sample.agent_attributes.keys.to_set - expected = Set.new(['job.sidekiq.args.0', 'job.sidekiq.args.1']) - - assert_equal expected, actual - end - end - end - - def refute_attributes_on_transaction_trace - transaction_samples = $collector.calls_for('transaction_sample_data') - - refute_empty transaction_samples, "Didn't find any transaction samples!" - - transaction_samples.each do |post| - post.samples.each do |sample| - assert_equal sample.metric_name, TRANSACTION_NAME, "Huh, that transaction shouldn't be in there!" - assert sample.agent_attributes.keys.none? { |k| k =~ /^job.sidekiq.args.*/ } - end - end - end - - def assert_attributes_on_events - transaction_event_posts = $collector.calls_for('analytic_event_data')[0].events - span_event_posts = $collector.calls_for('span_event_data')[0].events - events = transaction_event_posts + span_event_posts - events.each do |event| - assert_includes event[2].keys, 'job.sidekiq.args.0' - assert_includes event[2].keys, 'job.sidekiq.args.1' - end - end - - def refute_attributes_on_events - transaction_event_posts = $collector.calls_for('analytic_event_data')[0].events - span_event_posts = $collector.calls_for('span_event_data')[0].events - events = transaction_event_posts + span_event_posts - - events.each do |event| - assert event[2].keys.none? { |k| k.start_with?('job.sidekiq.args') }, 'Found unexpected sidekiq arguments' + exception = StandardError.new('bonk') + noticed = [] + NewRelic::Agent.stub :notice_error, proc { |e| noticed.push(e) } do + Sidekiq::CLI.instance.handle_exception(exception) end - end - def assert_error_for_each_job(txn_name = TRANSACTION_NAME) - error_posts = $collector.calls_for('error_data') - - assert_equal 1, error_posts.length, 'Wrong number of error posts!' - - errors = error_posts.first - - assert_equal JOB_COUNT, errors.errors.length, 'Wrong number of errors noticed!' - - assert_metric_and_call_count('Errors/all', JOB_COUNT) - if txn_name - assert_metric_and_call_count('Errors/allOther', JOB_COUNT) - assert_metric_and_call_count("Errors/#{TRANSACTION_NAME}", JOB_COUNT) - end + assert_equal 1, noticed.size + assert_equal exception, noticed.first end end diff --git a/test/multiverse/suites/sidekiq/sidekiq_server.rb b/test/multiverse/suites/sidekiq/sidekiq_server.rb deleted file mode 100644 index 7f33ac403e..0000000000 --- a/test/multiverse/suites/sidekiq/sidekiq_server.rb +++ /dev/null @@ -1,47 +0,0 @@ -# This file is distributed under New Relic's license terms. -# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. -# frozen_string_literal: true - -require 'sidekiq' -require 'sidekiq/cli' -require_relative '../../../helpers/docker' - -class SidekiqServer - include Singleton - - THREAD_JOIN_TIMEOUT = 30 - - attr_reader :queue_name - - def initialize - @queue_name = "sidekiq#{Process.pid}" - @sidekiq = Sidekiq::CLI.instance - set_redis_host - end - - def run(file = 'test_worker.rb') - @sidekiq.parse(['--require', File.join(File.dirname(__FILE__), file), - '--queue', "#{queue_name},1"]) - @cli_thread = Thread.new { @sidekiq.run } - end - - # If we just let the process go away, occasional timing issues cause the - # Launcher actor in Sidekiq to throw a fuss and exit with a failed code. - def stop - puts "Trying to stop Sidekiq gracefully from #{$$}" - Process.kill('INT', $$) - if @cli_thread.join(THREAD_JOIN_TIMEOUT).nil? - puts "#{$$} Sidekiq::CLI thread timeout on exit" - end - end - - private - - def set_redis_host - return unless docker? - - Sidekiq.configure_server do |config| - config.redis = {url: 'redis://redis:6379/1'} - end - end -end diff --git a/test/multiverse/suites/sidekiq/sidekiq_test_helpers.rb b/test/multiverse/suites/sidekiq/sidekiq_test_helpers.rb new file mode 100644 index 0000000000..788451c0eb --- /dev/null +++ b/test/multiverse/suites/sidekiq/sidekiq_test_helpers.rb @@ -0,0 +1,80 @@ +# This file is distributed under New Relic's license terms. +# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. +# frozen_string_literal: true + +require 'sidekiq' +require 'sidekiq/cli' +require 'newrelic_rpm' + +class NRDeadEndJob + # TODO: MAJOR VERSION - remove this when Sidekiq v5 is no longer supported + if Sidekiq::VERSION.split('.').first.to_i >= 6 + include Sidekiq::Job + else + include Sidekiq::Worker + end + + sidekiq_options retry: 5 + + COMPLETION_VAR = :@@nr_job_complete + ERROR_MESSAGE = 'kaboom' + + def perform(*args) + raise ERROR_MESSAGE if args.first.is_a?(Hash) && args.first['raise_error'] + ensure + self.class.class_variable_set(COMPLETION_VAR, true) + end +end + +module SidekiqTestHelpers + def run_job(*args) + segments = nil + in_transaction do |txn| + NRDeadEndJob.perform_async(*args) + process_queued_jobs + segments = txn.segments.select { |s| s.name.eql?('Nested/OtherTransaction/SidekiqJob/NRDeadEndJob/perform') } + end + + assert_equal 1, segments.size, "Expected to find a single Sidekiq job segment, found #{segments.size}" + segments.first + end + + def run_job_and_get_attributes(*args) + run_job(*args).attributes.agent_attributes_for(NewRelic::Agent::AttributeFilter::DST_TRANSACTION_TRACER) + end + + def process_queued_jobs + NRDeadEndJob.class_variable_set(NRDeadEndJob::COMPLETION_VAR, false) + config = cli.instance_variable_defined?(:@config) ? cli.instance_variable_get(:@config) : Sidekiq.options + + # TODO: MAJOR VERSION - remove this when Sidekiq v5 is no longer supported + require 'sidekiq/launcher' if Sidekiq::VERSION.split('.').first.to_i < 6 + + launcher = Sidekiq::Launcher.new(config) + launcher.run + Timeout.timeout(5) do + sleep 0.01 until NRDeadEndJob.class_variable_get(NRDeadEndJob::COMPLETION_VAR) + end + + # TODO: MAJOR VERSION - Sidekiq v7 is fine with launcher.stop, but v5 and v6 + # need the Manager#quiet call + if launcher.instance_variable_defined?(:@manager) + launcher.instance_variable_get(:@manager).quiet + else + launcher.stop + end + end + + def cli + @cli ||= begin + cli = Sidekiq::CLI.instance + cli.parse(['--require', File.absolute_path(__FILE__), '--queue', 'default,1']) + cli.logger.instance_variable_get(:@logdev).instance_variable_set(:@dev, File.new('/dev/null', 'w')) + cli + end + end + + def flatten(object) + NewRelic::Agent::AttributeProcessing.flatten_and_coerce(object, 'job.sidekiq.args') + end +end diff --git a/test/multiverse/suites/sidekiq/test_model.rb b/test/multiverse/suites/sidekiq/test_model.rb deleted file mode 100644 index 6f3f5ad682..0000000000 --- a/test/multiverse/suites/sidekiq/test_model.rb +++ /dev/null @@ -1,12 +0,0 @@ -# This file is distributed under New Relic's license terms. -# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. -# frozen_string_literal: true - -# This class is used to test Sidekiq's Delayed Extensions -# which give the framework an interface like Delayed Job. -# The Delayed Extensions cannot be used to operate directly -# on a Sidekiq Worker. -class TestModel - def self.do_work - end -end diff --git a/test/multiverse/suites/sidekiq/test_worker.rb b/test/multiverse/suites/sidekiq/test_worker.rb deleted file mode 100644 index bcafdf789c..0000000000 --- a/test/multiverse/suites/sidekiq/test_worker.rb +++ /dev/null @@ -1,78 +0,0 @@ -# This file is distributed under New Relic's license terms. -# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. -# frozen_string_literal: true - -require_relative 'sidekiq_server' - -class TestWorker - include Sidekiq::Worker - - sidekiq_options :queue => SidekiqServer.instance.queue_name, :retry => false - @jobs = {} - @jobs_mutex = Mutex.new - - @done = Queue.new - - def self.register_signal(key) - @jobs_mutex.synchronize do - return if @registered_signal - - NewRelic::Agent.subscribe(:transaction_finished) do |payload| - @done.push(true) - end - @registered_signal = true - end - end - - def self.run_jobs(count) - reset(count) - count.times do |i| - yield(i) - end - wait - end - - def self.reset(done_at) - @jobs_mutex.synchronize do - @jobs = {} - @done_at = done_at - end - end - - def self.record(key, val) - @jobs_mutex.synchronize do - @jobs[key] ||= [] - @jobs[key] << val - end - end - - def self.records_for(key) - @jobs[key] - end - - def self.wait - # Don't hang out forever, but shouldn't count on the timeout functionally - Timeout.timeout(15) do - @done_at.times do - @done.pop - sleep(0.01) - end - end - end - - def self.fail=(val) - @fail = val - end - - def self.am_i_a_failure? - @fail - end - - def perform(key, val) - if self.class.am_i_a_failure? - raise 'Uh oh' - else - TestWorker.record(key, val) - end - end -end diff --git a/test/new_relic/agent/attribute_processing_test.rb b/test/new_relic/agent/attribute_processing_test.rb index 16a983af35..1114e3b0d9 100644 --- a/test/new_relic/agent/attribute_processing_test.rb +++ b/test/new_relic/agent/attribute_processing_test.rb @@ -159,4 +159,238 @@ def test_flatten_and_coerce_leaves_nils_alone assert_equal expected, result end + + def test_string_to_regexp + regexp = NewRelic::Agent::AttributeProcessing.string_to_regexp('(? [/^up$/, /\Alift\z/]} + with_stubbed_config(config) do + union = NewRelic::Agent::AttributeProcessing.formulate_regexp_union(option) + + assert union.is_a?(Regexp) + assert_match union, 'up', "Expected the Regexp union to match 'up'" + assert_match union, 'lift', "Expected the Regexp union to match 'lift'" + end + end + + def test_formulate_regexp_union_with_single_regexp + option = :micro_machines + config = {option => [/4x4/]} + with_stubbed_config(config) do + union = NewRelic::Agent::AttributeProcessing.formulate_regexp_union(option) + + assert union.is_a?(Regexp) + assert_match union, '4x4 set 20', "Expected the Regexp union to match '4x4 set 20'" + end + end + + def test_formulate_regexp_union_when_option_is_not_set + option = :soul_calibur2 + config = {option => []} + + with_stubbed_config(config) do + assert_nil NewRelic::Agent::AttributeProcessing.formulate_regexp_union(option) + end + end + + def test_formulate_regexp_union_with_exception_is_raised + skip_unless_minitest5_or_above + + with_stubbed_config do + # formulate_regexp_union expects to be working with options that have an + # empty array for a default value. If it receives a bogus option, an + # exception will be raised, caught, and logged and a nil will be returned. + phony_logger = Minitest::Mock.new + phony_logger.expect :warn, nil, [/Failed to formulate/] + NewRelic::Agent.stub :logger, phony_logger do + assert_nil NewRelic::Agent::AttributeProcessing.formulate_regexp_union(:option_name_with_typo) + phony_logger.verify + end + end + end + + def test_pre_filter + input = [{one: 1, two: 2}, [1, 2], 1, 2] + options = {include: /one|1/} + expected = [{one: 1}, [1], 1] + values = NewRelic::Agent::AttributeProcessing.pre_filter(input, options) + + assert_equal expected, values, "pre_filter returned >>#{values}<<, expected >>#{expected}<<" + end + + def test_pre_filter_without_include_or_exclude + input = [{one: 1, two: 2}, [1, 2], 1, 2] + values = NewRelic::Agent::AttributeProcessing.pre_filter(input, {}) + + assert_equal input, values, "pre_filter returned >>#{values}<<, expected >>#{input}<<" + end + + def test_pre_filter_with_prefix_that_will_be_filtered_out_after_pre_filter + skip_unless_minitest5_or_above + + input = [{one: 1, two: 2}, [1, 2], 1, 2] + namespace = 'something filtered out by default' + # config has specified an include pattern for pre filtration, but regular + # filtration will block all of the content anyhow, so expect a no-op result + options = {include: /one|1/, attribute_namespace: namespace} + NewRelic::Agent.instance.attribute_filter.stub :might_allow_prefix?, false, [namespace] do + values = NewRelic::Agent::AttributeProcessing.pre_filter(input, options) + + assert_equal input, values, "pre_filter returned >>#{values}<<, expected >>#{input}<<" + end + end + + def test_pre_filter_hash + input = {one: 1, two: 2} + options = {exclude: /1/} + expected = {two: 2} + result = NewRelic::Agent::AttributeProcessing.pre_filter_hash(input, options) + + assert_equal expected, result, "pre_filter_hash returned >>#{result}<<, expected >>#{expected}<<" + end + + # if a key matches an include, include the key/value pair even though the + # value itself doesn't match the include + def test_pre_filter_hash_includes_a_value_when_a_key_is_included + input = {one: 1, two: 2} + options = {include: /one/} + expected = {one: 1} + result = NewRelic::Agent::AttributeProcessing.pre_filter_hash(input, options) + + assert_equal expected, result, "pre_filter_hash returned >>#{result}<<, expected >>#{expected}<<" + end + + # even if a key matches an include, withhold the key/value pair if the + # value matches an exclude + def test_pre_filter_hash_still_applies_exclusions_to_hash_values + input = {one: 1, two: 2} + options = {include: /one|two/, exclude: /1/} + expected = {two: 2} + result = NewRelic::Agent::AttributeProcessing.pre_filter_hash(input, options) + + assert_equal expected, result, "pre_filter_hash returned >>#{result}<<, expected >>#{expected}<<" + end + + def test_pre_filter_hash_allows_an_empty_hash_to_pass_through + input = {} + options = {include: /one|two/} + result = NewRelic::Agent::AttributeProcessing.pre_filter_hash(input, options) + + assert_equal input, result, "pre_filter_hash returned >>#{result}<<, expected >>#{input}<<" + end + + def test_pre_filter_hash_removes_the_hash_if_nothing_can_be_included + input = {one: 1, two: 2} + options = {include: /three/} + result = NewRelic::Agent::AttributeProcessing.pre_filter_hash(input, options) + + assert_equal NewRelic::Agent::AttributeProcessing::DISCARDED, result, "pre_filter_hash returned >>#{result}<<, expected a 'discarded' result" + end + + def test_pre_filter_array + input = %w[one two 1 2] + options = {exclude: /1|one/} + expected = %w[two 2] + result = NewRelic::Agent::AttributeProcessing.pre_filter_array(input, options) + + assert_equal expected, result, "pre_filter_array returned >>#{result}<<, expected >>#{expected}<<" + end + + def test_pre_filter_array_allows_an_empty_array_to_pass_through + input = [] + options = {exclude: /1|one/} + result = NewRelic::Agent::AttributeProcessing.pre_filter_array(input, options) + + assert_equal input, result, "pre_filter_array returned >>#{result}<<, expected >>#{input}<<" + end + + def test_pre_filter_array_removes_the_array_if_nothing_can_be_included + input = %w[one two 1 2] + options = {exclude: /1|one|2|two/} + result = NewRelic::Agent::AttributeProcessing.pre_filter_array(input, options) + + assert_equal NewRelic::Agent::AttributeProcessing::DISCARDED, result, "pre_filter_array returned >>#{result}<<, expected a 'discarded' result" + end + + def test_pre_filter_scalar + input = false + options = {include: /false/, exclude: /true/} + result = NewRelic::Agent::AttributeProcessing.pre_filter_scalar(input, options) + + assert_equal input, result, "pre_filter_scalar returned >>#{result}<<, expected >>#{input}<<" + end + + def test_pre_filter_scalar_without_include + input = false + options = {exclude: /true/} + result = NewRelic::Agent::AttributeProcessing.pre_filter_scalar(input, options) + + assert_equal input, result, "pre_filter_scalar returned >>#{result}<<, expected >>#{input}<<" + end + + def test_pre_filter_scalar_without_exclude + input = false + options = {exclude: /true/} + result = NewRelic::Agent::AttributeProcessing.pre_filter_scalar(input, options) + + assert_equal input, result, "pre_filter_scalar returned >>#{result}<<, expected >>#{input}<<" + end + + def test_pre_filter_scalar_include_results_in_discarded + input = false + options = {include: /true/} + result = NewRelic::Agent::AttributeProcessing.pre_filter_scalar(input, options) + + assert_equal NewRelic::Agent::AttributeProcessing::DISCARDED, result, "pre_filter_scalar returned >>#{result}<<, expected a 'discarded' result" + end + + def test_pre_filter_scalar_exclude_results_in_discarded + input = false + options = {exclude: /false/} + result = NewRelic::Agent::AttributeProcessing.pre_filter_scalar(input, options) + + assert_equal NewRelic::Agent::AttributeProcessing::DISCARDED, result, "pre_filter_scalar returned >>#{result}<<, expected a 'discarded' result" + end + + def test_pre_filter_scalar_without_include_or_exclude + input = false + result = NewRelic::Agent::AttributeProcessing.pre_filter_scalar(input, {}) + + assert_equal input, result, "pre_filter_scalar returned >>#{result}<<, expected >>#{input}<<" + end + + def test_discarded? + [nil, [], {}, false].each do |object| + refute NewRelic::Agent::AttributeProcessing.discarded?(object) + end + end + + private + + def with_stubbed_config(config = {}, &blk) + NewRelic::Agent.stub :config, config do + yield + end + end end From 7996532f4db10558c9b61987891f9cc40de18c10 Mon Sep 17 00:00:00 2001 From: fallwith Date: Sat, 26 Aug 2023 00:35:13 -0700 Subject: [PATCH 02/16] CHANGELOG entry for Sidekiq args filtration explain `sidekiq.args.include` and `sidekiq.args.exclude` in `CHANGELOG.md` --- CHANGELOG.md | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e3e0733ba..77e4adb0d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,14 +1,41 @@ # New Relic Ruby Agent Release Notes - ## dev -Version allows the agent to record additional response information on a transaction when middleware instrumentation is disabled and fixes a bug in `NewRelic::Rack::AgentHooks.needed?`. - +Version allows the agent to record additional response information on a transaction when middleware instrumentation is disabled, introduces new `:'sidekiq.args.include'` and `:'sidekiq.args.exclude:` configuration options to permit for the capturing of only certain Sidekiq job arguments, and fixes a bug in `NewRelic::Rack::AgentHooks.needed?`. - **Feature: Report transaction HTTP status codes when middleware instrumentation is disabled** Previously, when `disable_middleware_instrumentation` was set to `true`, the agent would not record the value of the response code or content type on the transaction. This was due to the possibility that a middleware could alter the response, which would not be captured by the agent when the middleware instrumentation was disabled. However, based on customer feedback, the agent will now report the HTTP status code and content type on a transaction when middleware instrumentation is disabled. [PR#2175](https://github.com/newrelic/newrelic-ruby-agent/pull/2175) +- **Feature: Permit the capturing of only certain Sidekiq job arguments** + New `:'sidekiq.args.include'` and `:'sidekiq.args.exclude'` configuration options have been introduced to permit fine grained control over which Sidekiq job arguments (args) are reported to New Relic. By default, no Sidekiq args are reported. To report any Sidekiq options, the `:'attributes.include'` array must include the string `'jobs.sidekiq.args.*'`. With that string in place, all arguments will be reported unless one or more of the new include/exclude options are used. The `:'sidekiq.args.include'` option can be set to an array of strings. Each of those strings will be passed to `Regexp.new` and collectively serve as an allowlist for desired args. For job arguments that are hashes, if a hash's key matches one of the include patterns, then both the key and its corresponding value will be included. For scalar arguments, the string representation of the scalar will need to match one of the include patterns to be included (sent to New Relic). The `:'sidekiq.args.exclude'` option works similarly. It can be set to an array of strings that will each be passed to `Regexp.new` to create patterns. These patterns will collectively serve as a denylist for unwanted job args. Any hash key, has value, or scalar that matches an exclude pattern will be excluded (not sent to New Relic). [PR#2177](https://github.com/newrelic/newrelic-ruby-agent/pull/2177) + + `newrelic.yml` based examples: + + ``` + # Include any argument whose string representation matches either "apple" or "banana" + sidekiq.args.include: + - apple + - banana + + # Exclude any arguments that match either "grape", "orange", or "pear" + sidekiq.args.exclude: + - grape + - orange + - pear + + # Exclude any argument that is a 9 digit number + sidekiq.args.exclude: + - '\d{9}' + + # Include anything that starts with "blue" but exclude anything that ends in "green" + sidekiq.args.include + - '^blue' + + sidekiq.args.exclude + - 'green$' + ``` + - **Bugfix: Resolve inverted logic of NewRelic::Rack::AgentHooks.needed?** Previously, `NewRelic::Rack::AgentHooks.needed?` incorrectly used inverted logic. This has now been resolved, allowing AgentHooks to be installed when `disable_middleware_instrumentation` is set to true. [PR#2175](https://github.com/newrelic/newrelic-ruby-agent/pull/2175) From 55940fb9c2a8bc88a46111fdb3d6c02ef143393e Mon Sep 17 00:00:00 2001 From: fallwith Date: Sat, 26 Aug 2023 00:45:44 -0700 Subject: [PATCH 03/16] Line length fixes for RuboCop We don't typically break up the description lines, but our RuboCop todo file has a threshold of the longest known line so let's not break that threshold. --- .../agent/configuration/default_source.rb | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/lib/new_relic/agent/configuration/default_source.rb b/lib/new_relic/agent/configuration/default_source.rb index 2101ea8ae5..35dc2e1ff1 100644 --- a/lib/new_relic/agent/configuration/default_source.rb +++ b/lib/new_relic/agent/configuration/default_source.rb @@ -1722,14 +1722,24 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) public: true, type: Array, allowed_from_server: false, - description: "An array of strings that will collectively serve as an allowlist for filtering which Sidekiq job arguments get reported to New Relic. The capturing of any Sidekiq arguments requires that 'job.sidekiq.args.*' be added to the separate :'attributes.include' configuration option. Each string in this array will be turned into a regular expression via `Regexp.new` to permit advanced matching. For job argument hashes, if either a key or value matches the pair will be included. All matching job argument array elements and job argument scalars will be included." + description: 'An array of strings that will collectively serve as an allowlist for filtering which Sidekiq ' + + 'job arguments get reported to New Relic. The capturing of any Sidekiq arguments requires that ' + + "'job.sidekiq.args.*' be added to the separate :'attributes.include' configuration option. Each " + + 'string in this array will be turned into a regular expression via `Regexp.new` to permit advanced ' + + 'matching. For job argument hashes, if either a key or value matches the pair will be included. All ' + + 'matching job argument array elements and job argument scalars will be included.' }, :'sidekiq.args.exclude' => { default: NewRelic::EMPTY_ARRAY, public: true, type: Array, allowed_from_server: false, - description: "An array of strings that will collectively serve as a denylist for filtering which Sidekiq job arguments get reported to New Relic. The capturing of any Sidekiq arguments requires that 'job.sidekiq.args.*' be added to the separate :'attributes.include' configuration option. Each string in this array will be turned into a regular expression via `Regexp.new` to permit advanced matching. For job argument hashes, if either a key or value matches the pair will be excluded. All matching job argument array elements and job argument scalars will be excluded." + description: 'An array of strings that will collectively serve as a denylist for filtering which Sidekiq ' + + 'job arguments get reported to New Relic. The capturing of any Sidekiq arguments requires that ' + + "'job.sidekiq.args.*' be added to the separate :'attributes.include' configuration option. Each string " + + 'in this array will be turned into a regular expression via `Regexp.new` to permit advanced matching. ' + + 'For job argument hashes, if either a key or value matches the pair will be excluded. All matching job ' + + 'argument array elements and job argument scalars will be excluded.' }, # Slow SQL :'slow_sql.enabled' => { From 5c049ba82013f8460cfdf8116ef215778f448708 Mon Sep 17 00:00:00 2001 From: James Bunch Date: Tue, 29 Aug 2023 11:35:20 -0700 Subject: [PATCH 04/16] Update CHANGELOG.md Sidekiq args CHANGELOG entry grammar and typo fixes Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com> --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 77e4adb0d8..04a30373d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## dev -Version allows the agent to record additional response information on a transaction when middleware instrumentation is disabled, introduces new `:'sidekiq.args.include'` and `:'sidekiq.args.exclude:` configuration options to permit for the capturing of only certain Sidekiq job arguments, and fixes a bug in `NewRelic::Rack::AgentHooks.needed?`. +Version allows the agent to record additional response information on a transaction when middleware instrumentation is disabled, introduces new `:'sidekiq.args.include'` and `:'sidekiq.args.exclude:` configuration options to permit capturing only certain Sidekiq job arguments, and fixes a bug in `NewRelic::Rack::AgentHooks.needed?`. - **Feature: Report transaction HTTP status codes when middleware instrumentation is disabled** Previously, when `disable_middleware_instrumentation` was set to `true`, the agent would not record the value of the response code or content type on the transaction. This was due to the possibility that a middleware could alter the response, which would not be captured by the agent when the middleware instrumentation was disabled. However, based on customer feedback, the agent will now report the HTTP status code and content type on a transaction when middleware instrumentation is disabled. [PR#2175](https://github.com/newrelic/newrelic-ruby-agent/pull/2175) From c8c1eee50241720f648e867ea502e3698b8e1bd7 Mon Sep 17 00:00:00 2001 From: James Bunch Date: Tue, 29 Aug 2023 11:35:51 -0700 Subject: [PATCH 05/16] Update CHANGELOG.md Sidekiq args CHANGELOG entry improvements Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com> --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 04a30373d9..657ee5a5da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ Version allows the agent to record additional response information on a tr - **Feature: Report transaction HTTP status codes when middleware instrumentation is disabled** Previously, when `disable_middleware_instrumentation` was set to `true`, the agent would not record the value of the response code or content type on the transaction. This was due to the possibility that a middleware could alter the response, which would not be captured by the agent when the middleware instrumentation was disabled. However, based on customer feedback, the agent will now report the HTTP status code and content type on a transaction when middleware instrumentation is disabled. [PR#2175](https://github.com/newrelic/newrelic-ruby-agent/pull/2175) -- **Feature: Permit the capturing of only certain Sidekiq job arguments** +- **Feature: Permit capturing only certain Sidekiq job arguments** New `:'sidekiq.args.include'` and `:'sidekiq.args.exclude'` configuration options have been introduced to permit fine grained control over which Sidekiq job arguments (args) are reported to New Relic. By default, no Sidekiq args are reported. To report any Sidekiq options, the `:'attributes.include'` array must include the string `'jobs.sidekiq.args.*'`. With that string in place, all arguments will be reported unless one or more of the new include/exclude options are used. The `:'sidekiq.args.include'` option can be set to an array of strings. Each of those strings will be passed to `Regexp.new` and collectively serve as an allowlist for desired args. For job arguments that are hashes, if a hash's key matches one of the include patterns, then both the key and its corresponding value will be included. For scalar arguments, the string representation of the scalar will need to match one of the include patterns to be included (sent to New Relic). The `:'sidekiq.args.exclude'` option works similarly. It can be set to an array of strings that will each be passed to `Regexp.new` to create patterns. These patterns will collectively serve as a denylist for unwanted job args. Any hash key, has value, or scalar that matches an exclude pattern will be excluded (not sent to New Relic). [PR#2177](https://github.com/newrelic/newrelic-ruby-agent/pull/2177) `newrelic.yml` based examples: From 6f04c8e5c84d55bb3069ca124ca8db194fb57843 Mon Sep 17 00:00:00 2001 From: fallwith Date: Tue, 29 Aug 2023 11:48:37 -0700 Subject: [PATCH 06/16] CHANGELOG Sidekiq args: wording We already use "capture" so stick with that instead of saying something along the lines of "sent to New Relic". --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 657ee5a5da..8ad4354dc1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ Version allows the agent to record additional response information on a tr Previously, when `disable_middleware_instrumentation` was set to `true`, the agent would not record the value of the response code or content type on the transaction. This was due to the possibility that a middleware could alter the response, which would not be captured by the agent when the middleware instrumentation was disabled. However, based on customer feedback, the agent will now report the HTTP status code and content type on a transaction when middleware instrumentation is disabled. [PR#2175](https://github.com/newrelic/newrelic-ruby-agent/pull/2175) - **Feature: Permit capturing only certain Sidekiq job arguments** - New `:'sidekiq.args.include'` and `:'sidekiq.args.exclude'` configuration options have been introduced to permit fine grained control over which Sidekiq job arguments (args) are reported to New Relic. By default, no Sidekiq args are reported. To report any Sidekiq options, the `:'attributes.include'` array must include the string `'jobs.sidekiq.args.*'`. With that string in place, all arguments will be reported unless one or more of the new include/exclude options are used. The `:'sidekiq.args.include'` option can be set to an array of strings. Each of those strings will be passed to `Regexp.new` and collectively serve as an allowlist for desired args. For job arguments that are hashes, if a hash's key matches one of the include patterns, then both the key and its corresponding value will be included. For scalar arguments, the string representation of the scalar will need to match one of the include patterns to be included (sent to New Relic). The `:'sidekiq.args.exclude'` option works similarly. It can be set to an array of strings that will each be passed to `Regexp.new` to create patterns. These patterns will collectively serve as a denylist for unwanted job args. Any hash key, has value, or scalar that matches an exclude pattern will be excluded (not sent to New Relic). [PR#2177](https://github.com/newrelic/newrelic-ruby-agent/pull/2177) + New `:'sidekiq.args.include'` and `:'sidekiq.args.exclude'` configuration options have been introduced to permit fine grained control over which Sidekiq job arguments (args) are reported to New Relic. By default, no Sidekiq args are reported. To report any Sidekiq options, the `:'attributes.include'` array must include the string `'jobs.sidekiq.args.*'`. With that string in place, all arguments will be reported unless one or more of the new include/exclude options are used. The `:'sidekiq.args.include'` option can be set to an array of strings. Each of those strings will be passed to `Regexp.new` and collectively serve as an allowlist for desired args. For job arguments that are hashes, if a hash's key matches one of the include patterns, then both the key and its corresponding value will be included. For scalar arguments, the string representation of the scalar will need to match one of the include patterns to be captured. The `:'sidekiq.args.exclude'` option works similarly. It can be set to an array of strings that will each be passed to `Regexp.new` to create patterns. These patterns will collectively serve as a denylist for unwanted job args. Any hash key, has value, or scalar that matches an exclude pattern will be excluded (not sent to New Relic). [PR#2177](https://github.com/newrelic/newrelic-ruby-agent/pull/2177) `newrelic.yml` based examples: From 7f9bd75ac1c15bd918eb4a9986ff3d1094fb98d3 Mon Sep 17 00:00:00 2001 From: James Bunch Date: Tue, 29 Aug 2023 11:52:04 -0700 Subject: [PATCH 07/16] Update CHANGELOG.md Sidekiq arg capturing entry wording fix Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com> --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ad4354dc1..c522837355 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ Version allows the agent to record additional response information on a tr - **Feature: Permit capturing only certain Sidekiq job arguments** New `:'sidekiq.args.include'` and `:'sidekiq.args.exclude'` configuration options have been introduced to permit fine grained control over which Sidekiq job arguments (args) are reported to New Relic. By default, no Sidekiq args are reported. To report any Sidekiq options, the `:'attributes.include'` array must include the string `'jobs.sidekiq.args.*'`. With that string in place, all arguments will be reported unless one or more of the new include/exclude options are used. The `:'sidekiq.args.include'` option can be set to an array of strings. Each of those strings will be passed to `Regexp.new` and collectively serve as an allowlist for desired args. For job arguments that are hashes, if a hash's key matches one of the include patterns, then both the key and its corresponding value will be included. For scalar arguments, the string representation of the scalar will need to match one of the include patterns to be captured. The `:'sidekiq.args.exclude'` option works similarly. It can be set to an array of strings that will each be passed to `Regexp.new` to create patterns. These patterns will collectively serve as a denylist for unwanted job args. Any hash key, has value, or scalar that matches an exclude pattern will be excluded (not sent to New Relic). [PR#2177](https://github.com/newrelic/newrelic-ruby-agent/pull/2177) - `newrelic.yml` based examples: + `newrelic.yml` examples: ``` # Include any argument whose string representation matches either "apple" or "banana" From 60715e529b9b3b7f71ec73edfce480cdfbdf5734 Mon Sep 17 00:00:00 2001 From: James Bunch Date: Tue, 29 Aug 2023 11:52:36 -0700 Subject: [PATCH 08/16] Update CHANGELOG.md Sidekiq arg capturing entry: specify YAML as the syntax to use for the newrelic.yml code block Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com> --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c522837355..c5eba13c5c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ Version allows the agent to record additional response information on a tr `newrelic.yml` examples: - ``` + ```yaml # Include any argument whose string representation matches either "apple" or "banana" sidekiq.args.include: - apple From fa500e436fced67a32e7fea7d06dcbd739a97b0e Mon Sep 17 00:00:00 2001 From: fallwith Date: Tue, 29 Aug 2023 12:11:39 -0700 Subject: [PATCH 09/16] CHANGELOG: Sidekiq args filtration examples Update the CHANGELOG entry for Sidekiq args filtration to more clearly explain regexp, link to RubyDocs for regexp, and explain inexact matches. --- CHANGELOG.md | 3 +++ sidekiq_todo | 5 +++++ test.rb | 8 ++++++++ 3 files changed, 16 insertions(+) create mode 100644 sidekiq_todo create mode 100644 test.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index c5eba13c5c..c89d47cb67 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,8 +12,11 @@ Version allows the agent to record additional response information on a tr `newrelic.yml` examples: + Any string in the `:'sidekiq.args.include'` or `:'sidekiq.args.exclude'` arrays will be turned into a regular expression. Knowledge of [Ruby regular expression support](https://ruby-doc.org/3.2.2/Regexp.html) can be leveraged but is not required. If regular expression syntax is not used, inexact matches will be performed and the string "Fortune" will match both "Fortune 500" and "Fortune and Glory". For exact matches, use [regular expression anchors](https://ruby-doc.org/3.2.2/Regexp.html#class-Regexp-label-Anchors). + ```yaml # Include any argument whose string representation matches either "apple" or "banana" + # The "apple" pattern will match both "green apple" and "red apple" sidekiq.args.include: - apple - banana diff --git a/sidekiq_todo b/sidekiq_todo new file mode 100644 index 0000000000..8072da970a --- /dev/null +++ b/sidekiq_todo @@ -0,0 +1,5 @@ +- Supportability metric on every job call? +- Perform sync not reported? +- Should we report job success/failure status and queue name? + + diff --git a/test.rb b/test.rb new file mode 100644 index 0000000000..453c47bcf9 --- /dev/null +++ b/test.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +string = 'hello string' +array = %w[hello array] +symbol = :hello_symbol +regexp = /hello regex/ + +[string, array, symbol, regexp].each { |o| puts "#{o} (#{o.class}) frozen? => #{o.frozen?}" } From ea08703e9553151b8a38ae6a0687afdea60f95b3 Mon Sep 17 00:00:00 2001 From: fallwith Date: Tue, 29 Aug 2023 14:00:39 -0700 Subject: [PATCH 10/16] Sidekiq args filtering: default source changes Sidekiq args filtering - in `default_source.rb`, use `dynamic_name: true`, and use heredocs for the descriptions. By using `dynamic_name: true`, we don't have to have an ignore list for config scanning. --- .../agent/configuration/default_source.rb | 30 +++++++++++-------- test/helpers/config_scanning.rb | 7 ----- 2 files changed, 18 insertions(+), 19 deletions(-) diff --git a/lib/new_relic/agent/configuration/default_source.rb b/lib/new_relic/agent/configuration/default_source.rb index 35dc2e1ff1..640a32b35b 100644 --- a/lib/new_relic/agent/configuration/default_source.rb +++ b/lib/new_relic/agent/configuration/default_source.rb @@ -1721,25 +1721,31 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) default: NewRelic::EMPTY_ARRAY, public: true, type: Array, + dynamic_name: true, allowed_from_server: false, - description: 'An array of strings that will collectively serve as an allowlist for filtering which Sidekiq ' + - 'job arguments get reported to New Relic. The capturing of any Sidekiq arguments requires that ' + - "'job.sidekiq.args.*' be added to the separate :'attributes.include' configuration option. Each " + - 'string in this array will be turned into a regular expression via `Regexp.new` to permit advanced ' + - 'matching. For job argument hashes, if either a key or value matches the pair will be included. All ' + - 'matching job argument array elements and job argument scalars will be included.' + description: <<~SIDEKIQ_ARGS_INCLUDE.chomp.tr("\n", ' ') + An array of strings that will collectively serve as an allowlist for filtering which Sidekiq + job arguments get reported to New Relic. The capturing of any Sidekiq arguments requires that + 'job.sidekiq.args.*' be added to the separate :'attributes.include' configuration option. Each + string in this array will be turned into a regular expression via `Regexp.new` to permit advanced + matching. For job argument hashes, if either a key or value matches the pair will be included. All + matching job argument array elements and job argument scalars will be included. + SIDEKIQ_ARGS_INCLUDE }, :'sidekiq.args.exclude' => { default: NewRelic::EMPTY_ARRAY, public: true, type: Array, + dynamic_name: true, allowed_from_server: false, - description: 'An array of strings that will collectively serve as a denylist for filtering which Sidekiq ' + - 'job arguments get reported to New Relic. The capturing of any Sidekiq arguments requires that ' + - "'job.sidekiq.args.*' be added to the separate :'attributes.include' configuration option. Each string " + - 'in this array will be turned into a regular expression via `Regexp.new` to permit advanced matching. ' + - 'For job argument hashes, if either a key or value matches the pair will be excluded. All matching job ' + - 'argument array elements and job argument scalars will be excluded.' + description: <<~SIDEKIQ_ARGS_EXCLUDE.chomp.tr("\n", ' ') + An array of strings that will collectively serve as a denylist for filtering which Sidekiq + job arguments get reported to New Relic. The capturing of any Sidekiq arguments requires that + 'job.sidekiq.args.*' be added to the separate :'attributes.include' configuration option. Each string + in this array will be turned into a regular expression via `Regexp.new` to permit advanced matching. + For job argument hashes, if either a key or value matches the pair will be excluded. All matching job + argument array elements and job argument scalars will be excluded. + SIDEKIQ_ARGS_EXCLUDE }, # Slow SQL :'slow_sql.enabled' => { diff --git a/test/helpers/config_scanning.rb b/test/helpers/config_scanning.rb index 10f20e90d9..83bff5eeee 100644 --- a/test/helpers/config_scanning.rb +++ b/test/helpers/config_scanning.rb @@ -15,11 +15,6 @@ module ConfigScanning EVENT_BUFFER_MACRO_PATTERN = /(capacity_key|enabled_key)\s+:(['"])?([a-z\._]+)\2?/ ASSIGNED_CONSTANT_PATTERN = /[A-Z]+\s*=\s*:(['"])?([a-z\._]+)\1?\s*/ - # These config settings shouldn't be worried about, possibly because they - # are only referenced via Ruby metaprogramming that won't work with this - # module's regex matching - IGNORED = %i[sidekiq.args.include sidekiq.args.exclude] - def scan_and_remove_used_entries(default_keys, non_test_files) non_test_files.each do |file| lines_in(file).each do |line| @@ -36,8 +31,6 @@ def scan_and_remove_used_entries(default_keys, non_test_files) default_keys.delete(key.delete("'").to_sym) end - IGNORED.each { |key| default_keys.delete(key) } - # Remove any config keys that are annotated with the 'dynamic_name' setting # This indicates that the names of these keys are constructed dynamically at # runtime, so we don't expect any explicit references to them in code. From 429d03d164f6e3eea1789ac080abae2ca8fc4f9b Mon Sep 17 00:00:00 2001 From: fallwith Date: Tue, 29 Aug 2023 15:27:45 -0700 Subject: [PATCH 11/16] remove junk files remove junk that was accidentally added --- sidekiq_todo | 5 ----- test.rb | 8 -------- 2 files changed, 13 deletions(-) delete mode 100644 sidekiq_todo delete mode 100644 test.rb diff --git a/sidekiq_todo b/sidekiq_todo deleted file mode 100644 index 8072da970a..0000000000 --- a/sidekiq_todo +++ /dev/null @@ -1,5 +0,0 @@ -- Supportability metric on every job call? -- Perform sync not reported? -- Should we report job success/failure status and queue name? - - diff --git a/test.rb b/test.rb deleted file mode 100644 index 453c47bcf9..0000000000 --- a/test.rb +++ /dev/null @@ -1,8 +0,0 @@ -# frozen_string_literal: true - -string = 'hello string' -array = %w[hello array] -symbol = :hello_symbol -regexp = /hello regex/ - -[string, array, symbol, regexp].each { |o| puts "#{o} (#{o.class}) frozen? => #{o.frozen?}" } From 2fa3e6664096ea033fab3d177513afd49b1185b6 Mon Sep 17 00:00:00 2001 From: fallwith Date: Tue, 29 Aug 2023 18:01:25 -0700 Subject: [PATCH 12/16] use a dedicated AttributePreFiltering class Given that the existing attributing processing methods and the new attribute pre-filtering methods don't share any content or logic, have the pre-filtering methods live in a separate dedicated `AttributePreFiltering` class. --- lib/new_relic/agent.rb | 1 + .../agent/attribute_pre_filtering.rb | 109 ++++++++ lib/new_relic/agent/attribute_processing.rb | 97 ------- .../agent/instrumentation/sidekiq/server.rb | 4 +- .../agent/attribute_pre_filtering_test.rb | 242 ++++++++++++++++++ .../agent/attribute_processing_test.rb | 234 ----------------- 6 files changed, 354 insertions(+), 333 deletions(-) create mode 100644 lib/new_relic/agent/attribute_pre_filtering.rb create mode 100644 test/new_relic/agent/attribute_pre_filtering_test.rb diff --git a/lib/new_relic/agent.rb b/lib/new_relic/agent.rb index 680586fe31..1a6934b2dc 100644 --- a/lib/new_relic/agent.rb +++ b/lib/new_relic/agent.rb @@ -58,6 +58,7 @@ module Agent require 'new_relic/agent/deprecator' require 'new_relic/agent/logging' require 'new_relic/agent/distributed_tracing' + require 'new_relic/agent/attribute_pre_filtering' require 'new_relic/agent/attribute_processing' require 'new_relic/agent/linking_metadata' require 'new_relic/agent/local_log_decorator' diff --git a/lib/new_relic/agent/attribute_pre_filtering.rb b/lib/new_relic/agent/attribute_pre_filtering.rb new file mode 100644 index 0000000000..6f06ca6867 --- /dev/null +++ b/lib/new_relic/agent/attribute_pre_filtering.rb @@ -0,0 +1,109 @@ +# This file is distributed under New Relic's license terms. +# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. +# frozen_string_literal: true + +module NewRelic + module Agent + module AttributePreFiltering + module_function + + PRE_FILTER_KEYS = %i[include exclude].freeze + DISCARDED = :nr_discarded + + def formulate_regexp_union(option) + return if NewRelic::Agent.config[option].empty? + + Regexp.union(NewRelic::Agent.config[option].map { |p| string_to_regexp(p) }.uniq.compact).freeze + rescue StandardError => e + NewRelic::Agent.logger.warn("Failed to formulate a Regexp union from the '#{option}' configuration option " + + "- #{e.class}: #{e.message}") + end + + def string_to_regexp(str) + Regexp.new(str) + rescue StandardError => e + NewRelic::Agent.logger.warn("Failed to initialize Regexp from string '#{str}' - #{e.class}: #{e.message}") + end + + # attribute filtering suppresses data that has already been flattened + # and coerced (serialized as text) via #flatten_and_coerce, and is + # restricted to basic text matching with a single optional wildcard. + # pre filtering operates on raw Ruby objects beforehand and uses full + # Ruby regex syntax + def pre_filter(values = [], options = {}) + return values unless !options.empty? && PRE_FILTER_KEYS.any? { |k| options.key?(k) } + + # if there's a prefix in play for (non-pre) attribute filtration and + # attribute filtration won't allow that prefix, then don't even bother + # with pre filtration that could only result in values that would be + # blocked + if options.key?(:attribute_namespace) && + !NewRelic::Agent.instance.attribute_filter.might_allow_prefix?(options[:attribute_namespace]) + return values + end + + values.each_with_object([]) do |element, filtered| + object = pre_filter_object(element, options) + filtered << object unless discarded?(object) + end + end + + def pre_filter_object(object, options) + if object.is_a?(Hash) + pre_filter_hash(object, options) + elsif object.is_a?(Array) + pre_filter_array(object, options) + else + pre_filter_scalar(object, options) + end + end + + def pre_filter_hash(hash, options) + filtered_hash = hash.each_with_object({}) do |(key, value), filtered| + filtered_key = pre_filter_object(key, options) + next if discarded?(filtered_key) + + # If the key is permitted, skip include filtration for the value + # but still apply exclude filtration + if options.key?(:exclude) + exclude_only = options.dup + exclude_only.delete(:include) + filtered_value = pre_filter_object(value, exclude_only) + next if discarded?(filtered_value) + else + filtered_value = value + end + + filtered[filtered_key] = filtered_value + end + + filtered_hash.empty? && !hash.empty? ? DISCARDED : filtered_hash + end + + def pre_filter_array(array, options) + filtered_array = array.each_with_object([]) do |element, filtered| + filtered_element = pre_filter_object(element, options) + next if discarded?(filtered_element) + + filtered.push(filtered_element) + end + + filtered_array.empty? && !array.empty? ? DISCARDED : filtered_array + end + + def pre_filter_scalar(scalar, options) + return DISCARDED if options.key?(:include) && !scalar.to_s.match?(options[:include]) + return DISCARDED if options.key?(:exclude) && scalar.to_s.match?(options[:exclude]) + + scalar + end + + # `nil`, empty enumerable objects, and `false` are all valid in their + # own right as application data, so pre-filtering uses a special value + # to indicate that filtered out data has been discarded + def discarded?(object) + object == DISCARDED + end + end + end +end diff --git a/lib/new_relic/agent/attribute_processing.rb b/lib/new_relic/agent/attribute_processing.rb index ba31882bb7..e23a145c9a 100644 --- a/lib/new_relic/agent/attribute_processing.rb +++ b/lib/new_relic/agent/attribute_processing.rb @@ -9,8 +9,6 @@ module AttributeProcessing EMPTY_HASH_STRING_LITERAL = '{}'.freeze EMPTY_ARRAY_STRING_LITERAL = '[]'.freeze - PRE_FILTER_KEYS = %i[include exclude].freeze - DISCARDED = :nr_discarded def flatten_and_coerce(object, prefix = nil, result = {}, &blk) if object.is_a?(Hash) @@ -59,101 +57,6 @@ def flatten_and_coerce_array(array, prefix, result, &blk) end end end - - def formulate_regexp_union(option) - return if NewRelic::Agent.config[option].empty? - - Regexp.union(NewRelic::Agent.config[option].map { |p| string_to_regexp(p) }.uniq.compact).freeze - rescue StandardError => e - NewRelic::Agent.logger.warn("Failed to formulate a Regexp union from the '#{option}' configuration option " + - "- #{e.class}: #{e.message}") - end - - def string_to_regexp(str) - Regexp.new(str) - rescue StandardError => e - NewRelic::Agent.logger.warn("Failed to initialize Regexp from string '#{str}' - #{e.class}: #{e.message}") - end - - # attribute filtering suppresses data that has already been flattened - # and coerced (serialized as text) via #flatten_and_coerce, and is - # restricted to basic text matching with a single optional wildcard. - # pre filtering operates on raw Ruby objects beforehand and uses full - # Ruby regex syntax - def pre_filter(values = [], options = {}) - return values unless !options.empty? && PRE_FILTER_KEYS.any? { |k| options.key?(k) } - - # if there's a prefix in play for (non-pre) attribute filtration and - # attribute filtration won't allow that prefix, then don't even bother - # with pre filtration that could only result in values that would be - # blocked - if options.key?(:attribute_namespace) && - !NewRelic::Agent.instance.attribute_filter.might_allow_prefix?(options[:attribute_namespace]) - return values - end - - values.each_with_object([]) do |element, filtered| - object = pre_filter_object(element, options) - filtered << object unless discarded?(object) - end - end - - def pre_filter_object(object, options) - if object.is_a?(Hash) - pre_filter_hash(object, options) - elsif object.is_a?(Array) - pre_filter_array(object, options) - else - pre_filter_scalar(object, options) - end - end - - def pre_filter_hash(hash, options) - filtered_hash = hash.each_with_object({}) do |(key, value), filtered| - filtered_key = pre_filter_object(key, options) - next if discarded?(filtered_key) - - # If the key is permitted, skip include filtration for the value - # but still apply exclude filtration - if options.key?(:exclude) - exclude_only = options.dup - exclude_only.delete(:include) - filtered_value = pre_filter_object(value, exclude_only) - next if discarded?(filtered_value) - else - filtered_value = value - end - - filtered[filtered_key] = filtered_value - end - - filtered_hash.empty? && !hash.empty? ? DISCARDED : filtered_hash - end - - def pre_filter_array(array, options) - filtered_array = array.each_with_object([]) do |element, filtered| - filtered_element = pre_filter_object(element, options) - next if discarded?(filtered_element) - - filtered.push(filtered_element) - end - - filtered_array.empty? && !array.empty? ? DISCARDED : filtered_array - end - - def pre_filter_scalar(scalar, options) - return DISCARDED if options.key?(:include) && !scalar.to_s.match?(options[:include]) - return DISCARDED if options.key?(:exclude) && scalar.to_s.match?(options[:exclude]) - - scalar - end - - # `nil`, empty enumerable objects, and `false` are all valid in their - # own right as application data, so pre-filtering uses a special value - # to indicate that filtered out data has been discarded - def discarded?(object) - object == DISCARDED - end end end end diff --git a/lib/new_relic/agent/instrumentation/sidekiq/server.rb b/lib/new_relic/agent/instrumentation/sidekiq/server.rb index 2bb56e900f..9059c2e1c9 100644 --- a/lib/new_relic/agent/instrumentation/sidekiq/server.rb +++ b/lib/new_relic/agent/instrumentation/sidekiq/server.rb @@ -23,7 +23,7 @@ def call(worker, msg, queue, *_) perform_action_with_newrelic_trace(trace_args) do NewRelic::Agent::Transaction.merge_untrusted_agent_attributes( - NewRelic::Agent::AttributeProcessing.pre_filter(msg['args'], self.class.nr_attribute_options), + NewRelic::Agent::AttributePreFiltering.pre_filter(msg['args'], self.class.nr_attribute_options), ATTRIBUTE_JOB_NAMESPACE, NewRelic::Agent::AttributeFilter::DST_NONE ) @@ -48,7 +48,7 @@ def self.nr_attribute_options @nr_attribute_options ||= begin ATTRIBUTE_FILTER_TYPES.each_with_object({}) do |type, opts| pattern = - NewRelic::Agent::AttributeProcessing.formulate_regexp_union(:"#{ATTRIBUTE_BASE_NAMESPACE}.#{type}") + NewRelic::Agent::AttributePreFiltering.formulate_regexp_union(:"#{ATTRIBUTE_BASE_NAMESPACE}.#{type}") opts[type] = pattern if pattern end.merge(attribute_namespace: ATTRIBUTE_JOB_NAMESPACE) end diff --git a/test/new_relic/agent/attribute_pre_filtering_test.rb b/test/new_relic/agent/attribute_pre_filtering_test.rb new file mode 100644 index 0000000000..a7db1bfa0a --- /dev/null +++ b/test/new_relic/agent/attribute_pre_filtering_test.rb @@ -0,0 +1,242 @@ +# This file is distributed under New Relic's license terms. +# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. +# frozen_string_literal: true + +require_relative '../../test_helper' +require 'new_relic/agent/attribute_pre_filtering' + +class AttributePreFilteringTest < Minitest::Test + def test_string_to_regexp + regexp = NewRelic::Agent::AttributePreFiltering.string_to_regexp('(? [/^up$/, /\Alift\z/]} + with_stubbed_config(config) do + union = NewRelic::Agent::AttributePreFiltering.formulate_regexp_union(option) + + assert union.is_a?(Regexp) + assert_match union, 'up', "Expected the Regexp union to match 'up'" + assert_match union, 'lift', "Expected the Regexp union to match 'lift'" + end + end + + def test_formulate_regexp_union_with_single_regexp + option = :micro_machines + config = {option => [/4x4/]} + with_stubbed_config(config) do + union = NewRelic::Agent::AttributePreFiltering.formulate_regexp_union(option) + + assert union.is_a?(Regexp) + assert_match union, '4x4 set 20', "Expected the Regexp union to match '4x4 set 20'" + end + end + + def test_formulate_regexp_union_when_option_is_not_set + option = :soul_calibur2 + config = {option => []} + + with_stubbed_config(config) do + assert_nil NewRelic::Agent::AttributePreFiltering.formulate_regexp_union(option) + end + end + + def test_formulate_regexp_union_with_exception_is_raised + skip_unless_minitest5_or_above + + with_stubbed_config do + # formulate_regexp_union expects to be working with options that have an + # empty array for a default value. If it receives a bogus option, an + # exception will be raised, caught, and logged and a nil will be returned. + phony_logger = Minitest::Mock.new + phony_logger.expect :warn, nil, [/Failed to formulate/] + NewRelic::Agent.stub :logger, phony_logger do + assert_nil NewRelic::Agent::AttributePreFiltering.formulate_regexp_union(:option_name_with_typo) + phony_logger.verify + end + end + end + + def test_pre_filter + input = [{one: 1, two: 2}, [1, 2], 1, 2] + options = {include: /one|1/} + expected = [{one: 1}, [1], 1] + values = NewRelic::Agent::AttributePreFiltering.pre_filter(input, options) + + assert_equal expected, values, "pre_filter returned >>#{values}<<, expected >>#{expected}<<" + end + + def test_pre_filter_without_include_or_exclude + input = [{one: 1, two: 2}, [1, 2], 1, 2] + values = NewRelic::Agent::AttributePreFiltering.pre_filter(input, {}) + + assert_equal input, values, "pre_filter returned >>#{values}<<, expected >>#{input}<<" + end + + def test_pre_filter_with_prefix_that_will_be_filtered_out_after_pre_filter + skip_unless_minitest5_or_above + + input = [{one: 1, two: 2}, [1, 2], 1, 2] + namespace = 'something filtered out by default' + # config has specified an include pattern for pre filtration, but regular + # filtration will block all of the content anyhow, so expect a no-op result + options = {include: /one|1/, attribute_namespace: namespace} + NewRelic::Agent.instance.attribute_filter.stub :might_allow_prefix?, false, [namespace] do + values = NewRelic::Agent::AttributePreFiltering.pre_filter(input, options) + + assert_equal input, values, "pre_filter returned >>#{values}<<, expected >>#{input}<<" + end + end + + def test_pre_filter_hash + input = {one: 1, two: 2} + options = {exclude: /1/} + expected = {two: 2} + result = NewRelic::Agent::AttributePreFiltering.pre_filter_hash(input, options) + + assert_equal expected, result, "pre_filter_hash returned >>#{result}<<, expected >>#{expected}<<" + end + + # if a key matches an include, include the key/value pair even though the + # value itself doesn't match the include + def test_pre_filter_hash_includes_a_value_when_a_key_is_included + input = {one: 1, two: 2} + options = {include: /one/} + expected = {one: 1} + result = NewRelic::Agent::AttributePreFiltering.pre_filter_hash(input, options) + + assert_equal expected, result, "pre_filter_hash returned >>#{result}<<, expected >>#{expected}<<" + end + + # even if a key matches an include, withhold the key/value pair if the + # value matches an exclude + def test_pre_filter_hash_still_applies_exclusions_to_hash_values + input = {one: 1, two: 2} + options = {include: /one|two/, exclude: /1/} + expected = {two: 2} + result = NewRelic::Agent::AttributePreFiltering.pre_filter_hash(input, options) + + assert_equal expected, result, "pre_filter_hash returned >>#{result}<<, expected >>#{expected}<<" + end + + def test_pre_filter_hash_allows_an_empty_hash_to_pass_through + input = {} + options = {include: /one|two/} + result = NewRelic::Agent::AttributePreFiltering.pre_filter_hash(input, options) + + assert_equal input, result, "pre_filter_hash returned >>#{result}<<, expected >>#{input}<<" + end + + def test_pre_filter_hash_removes_the_hash_if_nothing_can_be_included + input = {one: 1, two: 2} + options = {include: /three/} + result = NewRelic::Agent::AttributePreFiltering.pre_filter_hash(input, options) + + assert_equal NewRelic::Agent::AttributePreFiltering::DISCARDED, result, "pre_filter_hash returned >>#{result}<<, expected a 'discarded' result" + end + + def test_pre_filter_array + input = %w[one two 1 2] + options = {exclude: /1|one/} + expected = %w[two 2] + result = NewRelic::Agent::AttributePreFiltering.pre_filter_array(input, options) + + assert_equal expected, result, "pre_filter_array returned >>#{result}<<, expected >>#{expected}<<" + end + + def test_pre_filter_array_allows_an_empty_array_to_pass_through + input = [] + options = {exclude: /1|one/} + result = NewRelic::Agent::AttributePreFiltering.pre_filter_array(input, options) + + assert_equal input, result, "pre_filter_array returned >>#{result}<<, expected >>#{input}<<" + end + + def test_pre_filter_array_removes_the_array_if_nothing_can_be_included + input = %w[one two 1 2] + options = {exclude: /1|one|2|two/} + result = NewRelic::Agent::AttributePreFiltering.pre_filter_array(input, options) + + assert_equal NewRelic::Agent::AttributePreFiltering::DISCARDED, result, "pre_filter_array returned >>#{result}<<, expected a 'discarded' result" + end + + def test_pre_filter_scalar + input = false + options = {include: /false/, exclude: /true/} + result = NewRelic::Agent::AttributePreFiltering.pre_filter_scalar(input, options) + + assert_equal input, result, "pre_filter_scalar returned >>#{result}<<, expected >>#{input}<<" + end + + def test_pre_filter_scalar_without_include + input = false + options = {exclude: /true/} + result = NewRelic::Agent::AttributePreFiltering.pre_filter_scalar(input, options) + + assert_equal input, result, "pre_filter_scalar returned >>#{result}<<, expected >>#{input}<<" + end + + def test_pre_filter_scalar_without_exclude + input = false + options = {exclude: /true/} + result = NewRelic::Agent::AttributePreFiltering.pre_filter_scalar(input, options) + + assert_equal input, result, "pre_filter_scalar returned >>#{result}<<, expected >>#{input}<<" + end + + def test_pre_filter_scalar_include_results_in_discarded + input = false + options = {include: /true/} + result = NewRelic::Agent::AttributePreFiltering.pre_filter_scalar(input, options) + + assert_equal NewRelic::Agent::AttributePreFiltering::DISCARDED, result, "pre_filter_scalar returned >>#{result}<<, expected a 'discarded' result" + end + + def test_pre_filter_scalar_exclude_results_in_discarded + input = false + options = {exclude: /false/} + result = NewRelic::Agent::AttributePreFiltering.pre_filter_scalar(input, options) + + assert_equal NewRelic::Agent::AttributePreFiltering::DISCARDED, result, "pre_filter_scalar returned >>#{result}<<, expected a 'discarded' result" + end + + def test_pre_filter_scalar_without_include_or_exclude + input = false + result = NewRelic::Agent::AttributePreFiltering.pre_filter_scalar(input, {}) + + assert_equal input, result, "pre_filter_scalar returned >>#{result}<<, expected >>#{input}<<" + end + + def test_discarded? + [nil, [], {}, false].each do |object| + refute NewRelic::Agent::AttributePreFiltering.discarded?(object) + end + end + + private + + def with_stubbed_config(config = {}, &blk) + NewRelic::Agent.stub :config, config do + yield + end + end +end diff --git a/test/new_relic/agent/attribute_processing_test.rb b/test/new_relic/agent/attribute_processing_test.rb index 1114e3b0d9..16a983af35 100644 --- a/test/new_relic/agent/attribute_processing_test.rb +++ b/test/new_relic/agent/attribute_processing_test.rb @@ -159,238 +159,4 @@ def test_flatten_and_coerce_leaves_nils_alone assert_equal expected, result end - - def test_string_to_regexp - regexp = NewRelic::Agent::AttributeProcessing.string_to_regexp('(? [/^up$/, /\Alift\z/]} - with_stubbed_config(config) do - union = NewRelic::Agent::AttributeProcessing.formulate_regexp_union(option) - - assert union.is_a?(Regexp) - assert_match union, 'up', "Expected the Regexp union to match 'up'" - assert_match union, 'lift', "Expected the Regexp union to match 'lift'" - end - end - - def test_formulate_regexp_union_with_single_regexp - option = :micro_machines - config = {option => [/4x4/]} - with_stubbed_config(config) do - union = NewRelic::Agent::AttributeProcessing.formulate_regexp_union(option) - - assert union.is_a?(Regexp) - assert_match union, '4x4 set 20', "Expected the Regexp union to match '4x4 set 20'" - end - end - - def test_formulate_regexp_union_when_option_is_not_set - option = :soul_calibur2 - config = {option => []} - - with_stubbed_config(config) do - assert_nil NewRelic::Agent::AttributeProcessing.formulate_regexp_union(option) - end - end - - def test_formulate_regexp_union_with_exception_is_raised - skip_unless_minitest5_or_above - - with_stubbed_config do - # formulate_regexp_union expects to be working with options that have an - # empty array for a default value. If it receives a bogus option, an - # exception will be raised, caught, and logged and a nil will be returned. - phony_logger = Minitest::Mock.new - phony_logger.expect :warn, nil, [/Failed to formulate/] - NewRelic::Agent.stub :logger, phony_logger do - assert_nil NewRelic::Agent::AttributeProcessing.formulate_regexp_union(:option_name_with_typo) - phony_logger.verify - end - end - end - - def test_pre_filter - input = [{one: 1, two: 2}, [1, 2], 1, 2] - options = {include: /one|1/} - expected = [{one: 1}, [1], 1] - values = NewRelic::Agent::AttributeProcessing.pre_filter(input, options) - - assert_equal expected, values, "pre_filter returned >>#{values}<<, expected >>#{expected}<<" - end - - def test_pre_filter_without_include_or_exclude - input = [{one: 1, two: 2}, [1, 2], 1, 2] - values = NewRelic::Agent::AttributeProcessing.pre_filter(input, {}) - - assert_equal input, values, "pre_filter returned >>#{values}<<, expected >>#{input}<<" - end - - def test_pre_filter_with_prefix_that_will_be_filtered_out_after_pre_filter - skip_unless_minitest5_or_above - - input = [{one: 1, two: 2}, [1, 2], 1, 2] - namespace = 'something filtered out by default' - # config has specified an include pattern for pre filtration, but regular - # filtration will block all of the content anyhow, so expect a no-op result - options = {include: /one|1/, attribute_namespace: namespace} - NewRelic::Agent.instance.attribute_filter.stub :might_allow_prefix?, false, [namespace] do - values = NewRelic::Agent::AttributeProcessing.pre_filter(input, options) - - assert_equal input, values, "pre_filter returned >>#{values}<<, expected >>#{input}<<" - end - end - - def test_pre_filter_hash - input = {one: 1, two: 2} - options = {exclude: /1/} - expected = {two: 2} - result = NewRelic::Agent::AttributeProcessing.pre_filter_hash(input, options) - - assert_equal expected, result, "pre_filter_hash returned >>#{result}<<, expected >>#{expected}<<" - end - - # if a key matches an include, include the key/value pair even though the - # value itself doesn't match the include - def test_pre_filter_hash_includes_a_value_when_a_key_is_included - input = {one: 1, two: 2} - options = {include: /one/} - expected = {one: 1} - result = NewRelic::Agent::AttributeProcessing.pre_filter_hash(input, options) - - assert_equal expected, result, "pre_filter_hash returned >>#{result}<<, expected >>#{expected}<<" - end - - # even if a key matches an include, withhold the key/value pair if the - # value matches an exclude - def test_pre_filter_hash_still_applies_exclusions_to_hash_values - input = {one: 1, two: 2} - options = {include: /one|two/, exclude: /1/} - expected = {two: 2} - result = NewRelic::Agent::AttributeProcessing.pre_filter_hash(input, options) - - assert_equal expected, result, "pre_filter_hash returned >>#{result}<<, expected >>#{expected}<<" - end - - def test_pre_filter_hash_allows_an_empty_hash_to_pass_through - input = {} - options = {include: /one|two/} - result = NewRelic::Agent::AttributeProcessing.pre_filter_hash(input, options) - - assert_equal input, result, "pre_filter_hash returned >>#{result}<<, expected >>#{input}<<" - end - - def test_pre_filter_hash_removes_the_hash_if_nothing_can_be_included - input = {one: 1, two: 2} - options = {include: /three/} - result = NewRelic::Agent::AttributeProcessing.pre_filter_hash(input, options) - - assert_equal NewRelic::Agent::AttributeProcessing::DISCARDED, result, "pre_filter_hash returned >>#{result}<<, expected a 'discarded' result" - end - - def test_pre_filter_array - input = %w[one two 1 2] - options = {exclude: /1|one/} - expected = %w[two 2] - result = NewRelic::Agent::AttributeProcessing.pre_filter_array(input, options) - - assert_equal expected, result, "pre_filter_array returned >>#{result}<<, expected >>#{expected}<<" - end - - def test_pre_filter_array_allows_an_empty_array_to_pass_through - input = [] - options = {exclude: /1|one/} - result = NewRelic::Agent::AttributeProcessing.pre_filter_array(input, options) - - assert_equal input, result, "pre_filter_array returned >>#{result}<<, expected >>#{input}<<" - end - - def test_pre_filter_array_removes_the_array_if_nothing_can_be_included - input = %w[one two 1 2] - options = {exclude: /1|one|2|two/} - result = NewRelic::Agent::AttributeProcessing.pre_filter_array(input, options) - - assert_equal NewRelic::Agent::AttributeProcessing::DISCARDED, result, "pre_filter_array returned >>#{result}<<, expected a 'discarded' result" - end - - def test_pre_filter_scalar - input = false - options = {include: /false/, exclude: /true/} - result = NewRelic::Agent::AttributeProcessing.pre_filter_scalar(input, options) - - assert_equal input, result, "pre_filter_scalar returned >>#{result}<<, expected >>#{input}<<" - end - - def test_pre_filter_scalar_without_include - input = false - options = {exclude: /true/} - result = NewRelic::Agent::AttributeProcessing.pre_filter_scalar(input, options) - - assert_equal input, result, "pre_filter_scalar returned >>#{result}<<, expected >>#{input}<<" - end - - def test_pre_filter_scalar_without_exclude - input = false - options = {exclude: /true/} - result = NewRelic::Agent::AttributeProcessing.pre_filter_scalar(input, options) - - assert_equal input, result, "pre_filter_scalar returned >>#{result}<<, expected >>#{input}<<" - end - - def test_pre_filter_scalar_include_results_in_discarded - input = false - options = {include: /true/} - result = NewRelic::Agent::AttributeProcessing.pre_filter_scalar(input, options) - - assert_equal NewRelic::Agent::AttributeProcessing::DISCARDED, result, "pre_filter_scalar returned >>#{result}<<, expected a 'discarded' result" - end - - def test_pre_filter_scalar_exclude_results_in_discarded - input = false - options = {exclude: /false/} - result = NewRelic::Agent::AttributeProcessing.pre_filter_scalar(input, options) - - assert_equal NewRelic::Agent::AttributeProcessing::DISCARDED, result, "pre_filter_scalar returned >>#{result}<<, expected a 'discarded' result" - end - - def test_pre_filter_scalar_without_include_or_exclude - input = false - result = NewRelic::Agent::AttributeProcessing.pre_filter_scalar(input, {}) - - assert_equal input, result, "pre_filter_scalar returned >>#{result}<<, expected >>#{input}<<" - end - - def test_discarded? - [nil, [], {}, false].each do |object| - refute NewRelic::Agent::AttributeProcessing.discarded?(object) - end - end - - private - - def with_stubbed_config(config = {}, &blk) - NewRelic::Agent.stub :config, config do - yield - end - end end From 941f6f14387d6a9e2555a23bbc7da2f1444109eb Mon Sep 17 00:00:00 2001 From: James Bunch Date: Wed, 30 Aug 2023 11:34:13 -0700 Subject: [PATCH 13/16] Update CHANGELOG.md SIdekiq args filtering entry: "has" -> "hash" typo fix Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com> --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c89d47cb67..c20398f828 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ Version allows the agent to record additional response information on a tr Previously, when `disable_middleware_instrumentation` was set to `true`, the agent would not record the value of the response code or content type on the transaction. This was due to the possibility that a middleware could alter the response, which would not be captured by the agent when the middleware instrumentation was disabled. However, based on customer feedback, the agent will now report the HTTP status code and content type on a transaction when middleware instrumentation is disabled. [PR#2175](https://github.com/newrelic/newrelic-ruby-agent/pull/2175) - **Feature: Permit capturing only certain Sidekiq job arguments** - New `:'sidekiq.args.include'` and `:'sidekiq.args.exclude'` configuration options have been introduced to permit fine grained control over which Sidekiq job arguments (args) are reported to New Relic. By default, no Sidekiq args are reported. To report any Sidekiq options, the `:'attributes.include'` array must include the string `'jobs.sidekiq.args.*'`. With that string in place, all arguments will be reported unless one or more of the new include/exclude options are used. The `:'sidekiq.args.include'` option can be set to an array of strings. Each of those strings will be passed to `Regexp.new` and collectively serve as an allowlist for desired args. For job arguments that are hashes, if a hash's key matches one of the include patterns, then both the key and its corresponding value will be included. For scalar arguments, the string representation of the scalar will need to match one of the include patterns to be captured. The `:'sidekiq.args.exclude'` option works similarly. It can be set to an array of strings that will each be passed to `Regexp.new` to create patterns. These patterns will collectively serve as a denylist for unwanted job args. Any hash key, has value, or scalar that matches an exclude pattern will be excluded (not sent to New Relic). [PR#2177](https://github.com/newrelic/newrelic-ruby-agent/pull/2177) + New `:'sidekiq.args.include'` and `:'sidekiq.args.exclude'` configuration options have been introduced to permit fine grained control over which Sidekiq job arguments (args) are reported to New Relic. By default, no Sidekiq args are reported. To report any Sidekiq options, the `:'attributes.include'` array must include the string `'jobs.sidekiq.args.*'`. With that string in place, all arguments will be reported unless one or more of the new include/exclude options are used. The `:'sidekiq.args.include'` option can be set to an array of strings. Each of those strings will be passed to `Regexp.new` and collectively serve as an allowlist for desired args. For job arguments that are hashes, if a hash's key matches one of the include patterns, then both the key and its corresponding value will be included. For scalar arguments, the string representation of the scalar will need to match one of the include patterns to be captured. The `:'sidekiq.args.exclude'` option works similarly. It can be set to an array of strings that will each be passed to `Regexp.new` to create patterns. These patterns will collectively serve as a denylist for unwanted job args. Any hash key, hash value, or scalar that matches an exclude pattern will be excluded (not sent to New Relic). [PR#2177](https://github.com/newrelic/newrelic-ruby-agent/pull/2177) `newrelic.yml` examples: From 8c8d9707608f8eaa21a9d1cf16564b055c32ca0d Mon Sep 17 00:00:00 2001 From: James Bunch Date: Wed, 30 Aug 2023 11:37:11 -0700 Subject: [PATCH 14/16] Update lib/new_relic/agent/configuration/default_source.rb sidekiq.args.include description text updates Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com> --- lib/new_relic/agent/configuration/default_source.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/new_relic/agent/configuration/default_source.rb b/lib/new_relic/agent/configuration/default_source.rb index 640a32b35b..bd1540f000 100644 --- a/lib/new_relic/agent/configuration/default_source.rb +++ b/lib/new_relic/agent/configuration/default_source.rb @@ -1725,8 +1725,8 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) allowed_from_server: false, description: <<~SIDEKIQ_ARGS_INCLUDE.chomp.tr("\n", ' ') An array of strings that will collectively serve as an allowlist for filtering which Sidekiq - job arguments get reported to New Relic. The capturing of any Sidekiq arguments requires that - 'job.sidekiq.args.*' be added to the separate :'attributes.include' configuration option. Each + job arguments get reported to New Relic. To capture any Sidekiq arguments, + 'job.sidekiq.args.*' must be added to the separate `:'attributes.include'` configuration option. Each string in this array will be turned into a regular expression via `Regexp.new` to permit advanced matching. For job argument hashes, if either a key or value matches the pair will be included. All matching job argument array elements and job argument scalars will be included. From a749434205ec88f4ff8f0f263bcccaf2f2a7f1e8 Mon Sep 17 00:00:00 2001 From: James Bunch Date: Wed, 30 Aug 2023 11:37:27 -0700 Subject: [PATCH 15/16] Update lib/new_relic/agent/configuration/default_source.rb sidekiq.args.exclude description text updates Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com> --- lib/new_relic/agent/configuration/default_source.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/new_relic/agent/configuration/default_source.rb b/lib/new_relic/agent/configuration/default_source.rb index bd1540f000..9d90f70012 100644 --- a/lib/new_relic/agent/configuration/default_source.rb +++ b/lib/new_relic/agent/configuration/default_source.rb @@ -1740,8 +1740,8 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) allowed_from_server: false, description: <<~SIDEKIQ_ARGS_EXCLUDE.chomp.tr("\n", ' ') An array of strings that will collectively serve as a denylist for filtering which Sidekiq - job arguments get reported to New Relic. The capturing of any Sidekiq arguments requires that - 'job.sidekiq.args.*' be added to the separate :'attributes.include' configuration option. Each string + job arguments get reported to New Relic. To capture any Sidekiq arguments, + 'job.sidekiq.args.*' must be added to the separate `:'attributes.include'` configuration option. Each string in this array will be turned into a regular expression via `Regexp.new` to permit advanced matching. For job argument hashes, if either a key or value matches the pair will be excluded. All matching job argument array elements and job argument scalars will be excluded. From 5fc164d0cda99fd92dd8dad576c8be950fde476a Mon Sep 17 00:00:00 2001 From: fallwith Date: Wed, 30 Aug 2023 12:29:54 -0700 Subject: [PATCH 16/16] attribute pre-filtering test fix the "without exclude" test should actually operate without an exclude list. thanks, @kaylareopelle --- test/new_relic/agent/attribute_pre_filtering_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/new_relic/agent/attribute_pre_filtering_test.rb b/test/new_relic/agent/attribute_pre_filtering_test.rb index a7db1bfa0a..e0d55fe8b0 100644 --- a/test/new_relic/agent/attribute_pre_filtering_test.rb +++ b/test/new_relic/agent/attribute_pre_filtering_test.rb @@ -197,7 +197,7 @@ def test_pre_filter_scalar_without_include def test_pre_filter_scalar_without_exclude input = false - options = {exclude: /true/} + options = {include: /false/} result = NewRelic::Agent::AttributePreFiltering.pre_filter_scalar(input, options) assert_equal input, result, "pre_filter_scalar returned >>#{result}<<, expected >>#{input}<<"