diff --git a/CHANGELOG.md b/CHANGELOG.md index f7f79919df..1c8eb0adb3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,12 +2,16 @@ ## dev -Version fixes a bug related to expected errors not bearing a "true" value for the "expected" attribute if expected as a result of an HTTP status code match. +Version fixes a bug related to expected errors not bearing a "true" value for the "expected" attribute if expected as a result of an HTTP status code match and changes the way Stripe instrumentation metrics are named to prevent high-cardinality issues. - **Bugfix: HTTP status code based expected errors will now have an "expected" value of "true"** Previously when an error was treated as expected by the agent as a result of a matching HTTP status code being found in the :'error_collector.expected_status_codes' configuration setting, the error would not appear with an "expected" attribute value of "true" in the errors in the errors inbox. [PR#2710](https://github.com/newrelic/newrelic-ruby-agent/pull/2710) +- **Bugfix: Stripe metric names will no longer include full request paths to limit the unique name count** + + The Stripe instrumentation introduced in agent version v9.5.0 produced instrumentation metric names that used the full Stripe request path. For any significant Stripe usage, this could quickly lead to very large number of distinct metric names. Now only the API version and the category part of the request path are included in the metric name which still includes the "Stripe" opener and method (ex: "get") closer. Thanks to [@jdelStrother](https://github.com/jdelStrother) and [@jsneedles](https://github.com/jsneedles) for bringing this issue to our attention and providing terrific information explaining the problem and potential paths to resolution. [PR#2716](https://github.com/newrelic/newrelic-ruby-agent/pull/2716) + ## v9.10.2 Version 9.10.2 fixes a bug related to the new DynamoDB instrumentation and removes `Rails::Command::RakeCommand` from the default list of denylisted constants. diff --git a/lib/new_relic/agent/instrumentation/stripe_subscriber.rb b/lib/new_relic/agent/instrumentation/stripe_subscriber.rb index 46dd6a7dea..4738f147c8 100644 --- a/lib/new_relic/agent/instrumentation/stripe_subscriber.rb +++ b/lib/new_relic/agent/instrumentation/stripe_subscriber.rb @@ -10,6 +10,7 @@ class StripeSubscriber EVENT_ATTRIBUTES = %i[http_status method num_retries path request_id].freeze ATTRIBUTE_NAMESPACE = 'stripe.user_data' ATTRIBUTE_FILTER_TYPES = %i[include exclude].freeze + PATH_PORTION_PATTERN = %r{^/([^/]+/[^/]+)(?:/|\z)}.freeze def start_segment(event) return unless is_execution_traced? @@ -39,7 +40,27 @@ def is_execution_traced? end def metric_name(event) - "Stripe#{event.path}/#{event.method}" + # Grab only the first 2 items from the slash (/) delimited event path. + # These items are the API version string and the category. Grabbing + # any more of the path will result in unique method names that will + # easily grow to be too numerous to sort through in the UI and + # possibly even violate default New Relic metric count thresholds. + # See newrelic/newrelic-ruby-agent#2654 and + # newrelic/newrelic-ruby-agent#2709 for more details. + # + # In Ruby v3.4 benchmarks, using regex to get at the first two path + # elements was seen as more performant than using String#split. + # + # Regex legend: + # + # ^ = starts with + # / = a literal '/' + # () = capture + # (?:) = don't capture + # [^/]+ = 1 or more characters that are not '/' + # /|\z = a literal '/' OR the end of the string + path_portion = event.path =~ PATH_PORTION_PATTERN ? Regexp.last_match(1) : NewRelic::UNKNOWN + "Stripe/#{path_portion}/#{event.method}" end def add_stripe_attributes(segment, event) diff --git a/test/multiverse/suites/stripe/stripe_instrumentation_test.rb b/test/multiverse/suites/stripe/stripe_instrumentation_test.rb index 6abc54541e..c6b5bd207e 100644 --- a/test/multiverse/suites/stripe/stripe_instrumentation_test.rb +++ b/test/multiverse/suites/stripe/stripe_instrumentation_test.rb @@ -10,6 +10,7 @@ class StripeInstrumentation < Minitest::Test API_KEY = '123456789' STRIPE_URL = 'Stripe/v1/customers/get' + DummyEvent = Struct.new(:path, :method) def setup Stripe.api_key = API_KEY @@ -168,6 +169,27 @@ def test_start_segment_records_error end end + def test_metric_names_are_not_specific_enough_to_cause_a_cardinality_explosion + categories = %w[Trzy_kolory The_Apu_Trilogy The_Lord_of_the_Rings] + paths = ["/v1/#{categories[0]}/Niebieski", + "/v1/#{categories[0]}/Biały", + "/v1/#{categories[0]}/Czerwony", + "/v1/#{categories[1]}/Pather_Panchali", + "/v1/#{categories[1]}/Aparajito", + "/v1/#{categories[1]}/The_World_of_Apu", + "/v1/#{categories[2]}/The_Fellowship_of_the_Ring", + "/v1/#{categories[2]}/The_Two_Towers", + "/v1/#{categories[2]}/The_Return_of_the_King"] + method = 'get' + + subscriber = NewRelic::Agent::Instrumentation::StripeSubscriber.new + expected = categories.map { |c| Array.new(3) { "Stripe/v1/#{c}/#{method}" } }.flatten + actual = paths.map { |p| subscriber.send(:metric_name, DummyEvent.new(p, method)) } + + assert_equal expected, actual, + "Expected everything after the category in each path to be stripped away. Expected: #{expected} Actual: #{actual}" + end + def start_stripe_event Stripe::Customer.list({limit: 3}) end