Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracing: don't record HTTP OPTIONS and HEAD transactions #2181

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions sentry-rails/lib/sentry/rails/action_cable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
3 changes: 3 additions & 0 deletions sentry-rails/lib/sentry/rails/capture_exceptions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
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
Expand Down Expand Up @@ -32,6 +33,8 @@
end

def start_transaction(env, scope)
return nil if IGNORED_HTTP_METHODS.include?(env["REQUEST_METHOD"])

Check warning on line 36 in sentry-rails/lib/sentry/rails/capture_exceptions.rb

View check run for this annotation

Codecov / codecov/patch

sentry-rails/lib/sentry/rails/capture_exceptions.rb#L36

Added line #L36 was not covered by tests

options = { name: scope.transaction_name, source: scope.transaction_source, op: transaction_op }

if @assets_regexp && scope.transaction_name.match?(@assets_regexp)
Expand Down
14 changes: 13 additions & 1 deletion sentry-ruby/lib/sentry/rack/capture_exceptions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -62,7 +63,18 @@ def capture_exception(exception, env)
end

def start_transaction(env, scope)
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.
# 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
}

transaction = Sentry.continue_trace(env, **options)
Sentry.start_transaction(transaction: transaction, custom_sampling_context: { env: env }, **options)
end
Expand Down
16 changes: 16 additions & 0 deletions sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"}) }
Expand All @@ -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"}) }
Expand Down Expand Up @@ -295,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
Expand Down
Loading