Merge pull request #2716 from newrelic/stripefix

Stripe: do not include path ids in metric names

CHANGELOG.md

@@ -2,12 +2,16 @@
 
 ## dev
 
-Version <dev> 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 <dev> 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.

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)

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