From 08abba81aeda69f232f4cc3d0d7e2beabde6cb44 Mon Sep 17 00:00:00 2001 From: Natik Gadzhi Date: Sat, 25 Nov 2023 21:41:59 -0800 Subject: [PATCH 1/6] feat(performance): don't record HTTP OPTIONS and HEAD transactions --- CHANGELOG.md | 1 + sentry-ruby/lib/sentry/rack/capture_exceptions.rb | 13 ++++++++++++- sentry-ruby/lib/sentry/transaction.rb | 2 ++ .../spec/sentry/rack/capture_exceptions_spec.rb | 2 ++ 4 files changed, 17 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4207eec59..b0b16d3df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -87,6 +87,7 @@ ```rb config.enabled_patches += [:sidekiq_scheduler] ``` +- Sentry will not record traces of HTTP OPTIONS and HEAD requests in Rack and Rails apps [#2181](https://github.com/getsentry/sentry-ruby/pull/2181) ### Bug Fixes diff --git a/sentry-ruby/lib/sentry/rack/capture_exceptions.rb b/sentry-ruby/lib/sentry/rack/capture_exceptions.rb index 2248799a3..3b70db41b 100644 --- a/sentry-ruby/lib/sentry/rack/capture_exceptions.rb +++ b/sentry-ruby/lib/sentry/rack/capture_exceptions.rb @@ -4,6 +4,7 @@ module Sentry module Rack class CaptureExceptions ERROR_EVENT_ID_KEY = "sentry.error_event_id" + IGNORED_HTTP_METHODS = ["HEAD", "OPTIONS"].freeze def initialize(app) @app = app @@ -62,7 +63,17 @@ def capture_exception(exception, env) end def start_transaction(env, scope) - options = { name: scope.transaction_name, source: scope.transaction_source, op: transaction_op } + options = { + name: scope.transaction_name, + source: scope.transaction_source, + op: transaction_op + } + + # Tell Sentry to not sample this transaction if this is an HTTP OPTIONS or HEAD request. + if IGNORED_HTTP_METHODS.include?(env["REQUEST_METHOD"]) + options.merge!(sampled: false) + end + transaction = Sentry.continue_trace(env, **options) Sentry.start_transaction(transaction: transaction, custom_sampling_context: { env: env }, **options) end diff --git a/sentry-ruby/lib/sentry/transaction.rb b/sentry-ruby/lib/sentry/transaction.rb index b22b1735b..862ddf7b4 100644 --- a/sentry-ruby/lib/sentry/transaction.rb +++ b/sentry-ruby/lib/sentry/transaction.rb @@ -188,6 +188,8 @@ def set_initial_sample_decision(sampling_context:) return end + # This block would return either true / false (if parent was sampled or not) + # or the sample rate from the configuration, presumably between 0 and 1. sample_rate = if @traces_sampler.is_a?(Proc) @traces_sampler.call(sampling_context) diff --git a/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb b/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb index 24a90c87b..75a14d855 100644 --- a/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb +++ b/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb @@ -178,6 +178,7 @@ def inspect expect(event.breadcrumbs.count).to eq(1) expect(event.breadcrumbs.peek.message).to eq("request breadcrumb") end + it "doesn't pollute the top-level scope" do request_1 = lambda do |e| Sentry.configure_scope { |s| s.set_tags({tag_1: "foo"}) } @@ -192,6 +193,7 @@ def inspect expect(event.tags).to eq(tag_1: "foo") expect(Sentry.get_current_scope.tags).to eq(tag_1: "don't change me") end + it "doesn't pollute other request's scope" do request_1 = lambda do |e| Sentry.configure_scope { |s| s.set_tags({tag_1: "foo"}) } From ac55fdd55454f541515eb7753973b2a66479a8dd Mon Sep 17 00:00:00 2001 From: Natik Gadzhi Date: Mon, 27 Nov 2023 12:04:15 -0800 Subject: [PATCH 2/6] Move changelog entry to Unreleased --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b0b16d3df..4c0bcb28a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -65,6 +65,7 @@ ### Bug Fixes - Network errors raised in `Sentry::HTTPTransport` will no longer be reported to Sentry [#2178](https://github.com/getsentry/sentry-ruby/pull/2178) +- Sentry will not record traces of HTTP OPTIONS and HEAD requests in Rack and Rails apps [#2181](https://github.com/getsentry/sentry-ruby/pull/2181) ## 5.14.0 @@ -87,7 +88,6 @@ ```rb config.enabled_patches += [:sidekiq_scheduler] ``` -- Sentry will not record traces of HTTP OPTIONS and HEAD requests in Rack and Rails apps [#2181](https://github.com/getsentry/sentry-ruby/pull/2181) ### Bug Fixes From 81164a307b269da430d675a040dda2e2408c5dbf Mon Sep 17 00:00:00 2001 From: Natik Gadzhi Date: Mon, 27 Nov 2023 12:17:26 -0800 Subject: [PATCH 3/6] Add unit tests --- .../spec/sentry/rack/capture_exceptions_spec.rb | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb b/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb index 75a14d855..f8cbae0e5 100644 --- a/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb +++ b/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb @@ -297,6 +297,20 @@ def will_be_sampled_by_sdk verify_transaction_attributes(transaction) verify_transaction_inherits_external_transaction(transaction, external_transaction) end + + context "performing an HTTP OPTIONS request" do + let(:additional_headers) do + { method: "OPTIONS" } + end + + it "doesn't sample transaction" do + wont_be_sampled_by_sdk + + stack.call(env) + + expect(transaction).to be_nil + end + end end context "with unsampled trace" do From 180e9343af0e17f8f2db33e698c1e03323ed0b95 Mon Sep 17 00:00:00 2001 From: Natik Gadzhi Date: Wed, 29 Nov 2023 21:34:10 -0800 Subject: [PATCH 4/6] Early return in rack middleware for HTTP OPTIONS requests --- sentry-ruby/lib/sentry/rack/capture_exceptions.rb | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/sentry-ruby/lib/sentry/rack/capture_exceptions.rb b/sentry-ruby/lib/sentry/rack/capture_exceptions.rb index 3b70db41b..f5c7862b4 100644 --- a/sentry-ruby/lib/sentry/rack/capture_exceptions.rb +++ b/sentry-ruby/lib/sentry/rack/capture_exceptions.rb @@ -63,16 +63,17 @@ def capture_exception(exception, env) end def start_transaction(env, scope) + # Tell Sentry to not sample this transaction if this is an HTTP OPTIONS or HEAD request. + # Return early to avoid extra work that's not useful anyway, because this + # transaction won't be sampled. + # If we return nil here, it'll be passed to `finish_transaction` later, which is safe. + return nil if IGNORED_HTTP_METHODS.include?(env["REQUEST_METHOD"]) + options = { name: scope.transaction_name, source: scope.transaction_source, op: transaction_op } - - # Tell Sentry to not sample this transaction if this is an HTTP OPTIONS or HEAD request. - if IGNORED_HTTP_METHODS.include?(env["REQUEST_METHOD"]) - options.merge!(sampled: false) - end transaction = Sentry.continue_trace(env, **options) Sentry.start_transaction(transaction: transaction, custom_sampling_context: { env: env }, **options) From 7afd73d6cc835cb316e16bd46de6ac4fbdfdef20 Mon Sep 17 00:00:00 2001 From: Natik Gadzhi Date: Wed, 3 Jan 2024 20:56:22 -0800 Subject: [PATCH 5/6] remove stray comment --- sentry-ruby/lib/sentry/transaction.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/sentry-ruby/lib/sentry/transaction.rb b/sentry-ruby/lib/sentry/transaction.rb index 862ddf7b4..b22b1735b 100644 --- a/sentry-ruby/lib/sentry/transaction.rb +++ b/sentry-ruby/lib/sentry/transaction.rb @@ -188,8 +188,6 @@ def set_initial_sample_decision(sampling_context:) return end - # This block would return either true / false (if parent was sampled or not) - # or the sample rate from the configuration, presumably between 0 and 1. sample_rate = if @traces_sampler.is_a?(Proc) @traces_sampler.call(sampling_context) From d8891f386f26efc175317276a20ae46f6f8869fb Mon Sep 17 00:00:00 2001 From: Natik Gadzhi Date: Wed, 3 Jan 2024 21:11:03 -0800 Subject: [PATCH 6/6] Added rails and actioncable exceptions, and moved changelog --- CHANGELOG.md | 5 ++++- sentry-rails/lib/sentry/rails/action_cable.rb | 3 +++ sentry-rails/lib/sentry/rails/capture_exceptions.rb | 3 +++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4c0bcb28a..0ad988113 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,10 @@ - Pick up config.cron.default_timezone from Rails config ([#2213](https://github.com/getsentry/sentry-ruby/pull/2213)) +### Bug Fixes + +- Sentry will not record traces of HTTP OPTIONS and HEAD requests in Rack and Rails apps [#2181](https://github.com/getsentry/sentry-ruby/pull/2181) + ## 5.15.2 ### Bug Fixes @@ -65,7 +69,6 @@ ### Bug Fixes - Network errors raised in `Sentry::HTTPTransport` will no longer be reported to Sentry [#2178](https://github.com/getsentry/sentry-ruby/pull/2178) -- Sentry will not record traces of HTTP OPTIONS and HEAD requests in Rack and Rails apps [#2181](https://github.com/getsentry/sentry-ruby/pull/2181) ## 5.14.0 diff --git a/sentry-rails/lib/sentry/rails/action_cable.rb b/sentry-rails/lib/sentry/rails/action_cable.rb index 155377466..b8dd51a1b 100644 --- a/sentry-rails/lib/sentry/rails/action_cable.rb +++ b/sentry-rails/lib/sentry/rails/action_cable.rb @@ -3,6 +3,7 @@ module Rails module ActionCableExtensions class ErrorHandler OP_NAME = "websocket.server".freeze + IGNORED_HTTP_METHODS = ["HEAD", "OPTIONS"].freeze class << self def capture(connection, transaction_name:, extra_context: nil, &block) @@ -33,6 +34,8 @@ def capture(connection, transaction_name:, extra_context: nil, &block) end def start_transaction(env, scope) + return nil if IGNORED_HTTP_METHODS.include?(env["REQUEST_METHOD"]) + options = { name: scope.transaction_name, source: scope.transaction_source, op: OP_NAME } transaction = Sentry.continue_trace(env, **options) Sentry.start_transaction(transaction: transaction, **options) diff --git a/sentry-rails/lib/sentry/rails/capture_exceptions.rb b/sentry-rails/lib/sentry/rails/capture_exceptions.rb index 8d93784ed..5b7c28abe 100644 --- a/sentry-rails/lib/sentry/rails/capture_exceptions.rb +++ b/sentry-rails/lib/sentry/rails/capture_exceptions.rb @@ -2,6 +2,7 @@ module Sentry module Rails class CaptureExceptions < Sentry::Rack::CaptureExceptions RAILS_7_1 = Gem::Version.new(::Rails.version) >= Gem::Version.new("7.1.0.alpha") + IGNORED_HTTP_METHODS = ["HEAD", "OPTIONS"].freeze def initialize(_) super @@ -32,6 +33,8 @@ def capture_exception(exception, env) end def start_transaction(env, scope) + return nil if IGNORED_HTTP_METHODS.include?(env["REQUEST_METHOD"]) + options = { name: scope.transaction_name, source: scope.transaction_source, op: transaction_op } if @assets_regexp && scope.transaction_name.match?(@assets_regexp)