diff --git a/CHANGELOG.md b/CHANGELOG.md index ff1163894..49d11bda1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,10 +3,11 @@ ### Features - Add `include_sentry_event` matcher for RSpec [#2424](https://github.com/getsentry/sentry-ruby/pull/2424) -- Add support for Sentry Cache instrumentation, when using Rails.cache ([#2380](https://github.com/getsentry/sentry-ruby/pull/2380)) +- Add support for Sentry Cache instrumentation, when using Rails.cache [#2380](https://github.com/getsentry/sentry-ruby/pull/2380) - Add support for Queue Instrumentation for Sidekiq. [#2403](https://github.com/getsentry/sentry-ruby/pull/2403) - Add support for string errors in error reporter ([#2464](https://github.com/getsentry/sentry-ruby/pull/2464)) - Reset trace_id and add root transaction for sidekiq-cron [#2446](https://github.com/getsentry/sentry-ruby/pull/2446) +- Add support for Excon HTTP client instrumentation ([#2383](https://github.com/getsentry/sentry-ruby/pull/2383)) Note: MemoryStore and FileStore require Rails 8.0+ diff --git a/sentry-ruby/Gemfile b/sentry-ruby/Gemfile index 5ca0f99c2..5371ddbf2 100644 --- a/sentry-ruby/Gemfile +++ b/sentry-ruby/Gemfile @@ -28,5 +28,6 @@ gem "benchmark-memory" gem "yard", github: "lsegal/yard" gem "webrick" gem "faraday" +gem "excon" eval_gemfile File.expand_path("../Gemfile", __dir__) diff --git a/sentry-ruby/lib/sentry-ruby.rb b/sentry-ruby/lib/sentry-ruby.rb index 4ebf5b299..b7ecdea53 100644 --- a/sentry-ruby/lib/sentry-ruby.rb +++ b/sentry-ruby/lib/sentry-ruby.rb @@ -614,3 +614,4 @@ def utc_now require "sentry/puma" require "sentry/graphql" require "sentry/faraday" +require "sentry/excon" diff --git a/sentry-ruby/lib/sentry/excon.rb b/sentry-ruby/lib/sentry/excon.rb new file mode 100644 index 000000000..39559a992 --- /dev/null +++ b/sentry-ruby/lib/sentry/excon.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +Sentry.register_patch(:excon) do + if defined?(::Excon) + require "sentry/excon/middleware" + if Excon.defaults[:middlewares] + Excon.defaults[:middlewares] << Sentry::Excon::Middleware unless Excon.defaults[:middlewares].include?(Sentry::Excon::Middleware) + end + end +end diff --git a/sentry-ruby/lib/sentry/excon/middleware.rb b/sentry-ruby/lib/sentry/excon/middleware.rb new file mode 100644 index 000000000..9c6e59467 --- /dev/null +++ b/sentry-ruby/lib/sentry/excon/middleware.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +module Sentry + module Excon + OP_NAME = "http.client" + + class Middleware < ::Excon::Middleware::Base + def initialize(stack) + super + @instrumenter = Instrumenter.new + end + + def request_call(datum) + @instrumenter.start_transaction(datum) + @stack.request_call(datum) + end + + def response_call(datum) + @instrumenter.finish_transaction(datum) + @stack.response_call(datum) + end + end + + class Instrumenter + SPAN_ORIGIN = "auto.http.excon" + BREADCRUMB_CATEGORY = "http" + + include Utils::HttpTracing + + def start_transaction(env) + return unless Sentry.initialized? + + current_span = Sentry.get_current_scope&.span + @span = current_span&.start_child(op: OP_NAME, start_timestamp: Sentry.utc_now.to_f, origin: SPAN_ORIGIN) + + request_info = extract_request_info(env) + + if propagate_trace?(request_info[:url]) + set_propagation_headers(env[:headers]) + end + end + + def finish_transaction(response) + return unless @span + + response_status = response[:response][:status] + request_info = extract_request_info(response) + + if record_sentry_breadcrumb? + record_sentry_breadcrumb(request_info, response_status) + end + + set_span_info(@span, request_info, response_status) + ensure + @span&.finish + end + + private + + def extract_request_info(env) + url = env[:scheme] + "://" + env[:hostname] + env[:path] + result = { method: env[:method].to_s.upcase, url: url } + + if Sentry.configuration.send_default_pii + result[:query] = env[:query] + + # Handle excon 1.0.0+ + result[:query] = build_nested_query(result[:query]) unless result[:query].is_a?(String) + + result[:body] = env[:body] + end + + result + end + end + end +end diff --git a/sentry-ruby/lib/sentry/utils/http_tracing.rb b/sentry-ruby/lib/sentry/utils/http_tracing.rb index abf6a9141..136e78d99 100644 --- a/sentry-ruby/lib/sentry/utils/http_tracing.rb +++ b/sentry-ruby/lib/sentry/utils/http_tracing.rb @@ -19,7 +19,7 @@ def record_sentry_breadcrumb(request_info, response_status) crumb = Sentry::Breadcrumb.new( level: :info, category: self.class::BREADCRUMB_CATEGORY, - type: :info, + type: "info", data: { status: response_status, **request_info } ) @@ -36,6 +36,25 @@ def propagate_trace?(url) Sentry.configuration.propagate_traces && Sentry.configuration.trace_propagation_targets.any? { |target| url.match?(target) } end + + # Kindly borrowed from Rack::Utils + def build_nested_query(value, prefix = nil) + case value + when Array + value.map { |v| + build_nested_query(v, "#{prefix}[]") + }.join("&") + when Hash + value.map { |k, v| + build_nested_query(v, prefix ? "#{prefix}[#{k}]" : k) + }.delete_if(&:empty?).join("&") + when nil + URI.encode_www_form_component(prefix) + else + raise ArgumentError, "value must be a Hash" if prefix.nil? + "#{URI.encode_www_form_component(prefix)}=#{URI.encode_www_form_component(value)}" + end + end end end end diff --git a/sentry-ruby/spec/sentry/excon_spec.rb b/sentry-ruby/spec/sentry/excon_spec.rb new file mode 100644 index 000000000..96574622a --- /dev/null +++ b/sentry-ruby/spec/sentry/excon_spec.rb @@ -0,0 +1,251 @@ +# frozen_string_literal: true + +require "spec_helper" +require "contexts/with_request_mock" +require "excon" + +RSpec.describe "Sentry::Excon" do + include Sentry::Utils::HttpTracing + include_context "with request mock" + + before do + Excon.defaults[:mock] = true + end + + after(:each) do + Excon.stubs.clear + end + + let(:string_io) { StringIO.new } + let(:logger) do + ::Logger.new(string_io) + end + + context "with IPv6 addresses" do + before do + perform_basic_setup do |config| + config.traces_sample_rate = 1.0 + config.enabled_patches += [:excon] unless config.enabled_patches.include?(:excon) + end + end + + it "correctly parses the short-hand IPv6 addresses" do + Excon.stub({}, { body: "", status: 200 }) + + transaction = Sentry.start_transaction + Sentry.get_current_scope.set_span(transaction) + + _ = Excon.get("http://[::1]:8080/path", mock: true) + + expect(transaction.span_recorder.spans.count).to eq(2) + + request_span = transaction.span_recorder.spans.last + expect(request_span.data).to eq( + { "url" => "http://::1/path", "http.request.method" => "GET", "http.response.status_code" => 200 } + ) + end + end + + context "with tracing enabled" do + before do + perform_basic_setup do |config| + config.traces_sample_rate = 1.0 + config.transport.transport_class = Sentry::HTTPTransport + config.logger = logger + # the dsn needs to have a real host so we can make a real connection before sending a failed request + config.dsn = "http://foobarbaz@o447951.ingest.sentry.io/5434472" + config.enabled_patches += [:excon] unless config.enabled_patches.include?(:excon) + end + end + + context "with config.send_default_pii = true" do + before do + Sentry.configuration.send_default_pii = true + Sentry.configuration.breadcrumbs_logger = [:http_logger] + end + + it "records the request's span with query string in data" do + Excon.stub({}, { body: "", status: 200 }) + + transaction = Sentry.start_transaction + Sentry.get_current_scope.set_span(transaction) + + response = Excon.get("http://example.com/path?foo=bar", mock: true) + + expect(response.status).to eq(200) + expect(transaction.span_recorder.spans.count).to eq(2) + + request_span = transaction.span_recorder.spans.last + expect(request_span.op).to eq("http.client") + expect(request_span.origin).to eq("auto.http.excon") + expect(request_span.start_timestamp).not_to be_nil + expect(request_span.timestamp).not_to be_nil + expect(request_span.start_timestamp).not_to eq(request_span.timestamp) + expect(request_span.description).to eq("GET http://example.com/path") + expect(request_span.data).to eq({ + "http.response.status_code" => 200, + "url" => "http://example.com/path", + "http.request.method" => "GET", + "http.query" => "foo=bar" + }) + end + + it "records the request's span with advanced query string in data" do + Excon.stub({}, { body: "", status: 200 }) + + transaction = Sentry.start_transaction + Sentry.get_current_scope.set_span(transaction) + + connection = Excon.new("http://example.com/path") + response = connection.get(mock: true, query: build_nested_query({ foo: "bar", baz: [1, 2], qux: { a: 1, b: 2 } })) + + expect(response.status).to eq(200) + expect(transaction.span_recorder.spans.count).to eq(2) + + request_span = transaction.span_recorder.spans.last + expect(request_span.op).to eq("http.client") + expect(request_span.origin).to eq("auto.http.excon") + expect(request_span.start_timestamp).not_to be_nil + expect(request_span.timestamp).not_to be_nil + expect(request_span.start_timestamp).not_to eq(request_span.timestamp) + expect(request_span.description).to eq("GET http://example.com/path") + expect(request_span.data).to eq({ + "http.response.status_code" => 200, + "url" => "http://example.com/path", + "http.request.method" => "GET", + "http.query" => "foo=bar&baz%5B%5D=1&baz%5B%5D=2&qux%5Ba%5D=1&qux%5Bb%5D=2" + }) + end + + it "records breadcrumbs" do + Excon.stub({}, { body: "", status: 200 }) + + transaction = Sentry.start_transaction + Sentry.get_current_scope.set_span(transaction) + + _response = Excon.get("http://example.com/path?foo=bar", mock: true) + + transaction.span_recorder.spans.last + + crumb = Sentry.get_current_scope.breadcrumbs.peek + + expect(crumb.category).to eq("http") + expect(crumb.data[:status]).to eq(200) + expect(crumb.data[:method]).to eq("GET") + expect(crumb.data[:url]).to eq("http://example.com/path") + expect(crumb.data[:query]).to eq("foo=bar") + expect(crumb.data[:body]).to be(nil) + end + end + + context "with config.send_default_pii = false" do + before do + Sentry.configuration.send_default_pii = false + end + + it "records the request's span without query string" do + Excon.stub({}, { body: "", status: 200 }) + + transaction = Sentry.start_transaction + Sentry.get_current_scope.set_span(transaction) + + response = Excon.get("http://example.com/path?foo=bar", mock: true) + + expect(response.status).to eq(200) + expect(transaction.span_recorder.spans.count).to eq(2) + + request_span = transaction.span_recorder.spans.last + expect(request_span.op).to eq("http.client") + expect(request_span.origin).to eq("auto.http.excon") + expect(request_span.start_timestamp).not_to be_nil + expect(request_span.timestamp).not_to be_nil + expect(request_span.start_timestamp).not_to eq(request_span.timestamp) + expect(request_span.description).to eq("GET http://example.com/path") + expect(request_span.data).to eq({ + "http.response.status_code" => 200, + "url" => "http://example.com/path", + "http.request.method" => "GET" + }) + end + end + + context "when there're multiple requests" do + let(:transaction) { Sentry.start_transaction } + + before do + Sentry.get_current_scope.set_span(transaction) + end + + def verify_spans(transaction) + expect(transaction.span_recorder.spans.count).to eq(3) + expect(transaction.span_recorder.spans[0]).to eq(transaction) + + request_span = transaction.span_recorder.spans[1] + expect(request_span.op).to eq("http.client") + expect(request_span.origin).to eq("auto.http.excon") + expect(request_span.start_timestamp).not_to be_nil + expect(request_span.timestamp).not_to be_nil + expect(request_span.start_timestamp).not_to eq(request_span.timestamp) + expect(request_span.description).to eq("GET http://example.com/path") + expect(request_span.data).to eq({ + "http.response.status_code" => 200, + "url" => "http://example.com/path", + "http.request.method" => "GET" + }) + + request_span = transaction.span_recorder.spans[2] + expect(request_span.op).to eq("http.client") + expect(request_span.origin).to eq("auto.http.excon") + expect(request_span.start_timestamp).not_to be_nil + expect(request_span.timestamp).not_to be_nil + expect(request_span.start_timestamp).not_to eq(request_span.timestamp) + expect(request_span.description).to eq("GET http://example.com/path") + expect(request_span.data).to eq({ + "http.response.status_code" => 404, + "url" => "http://example.com/path", + "http.request.method" => "GET" + }) + end + + it "doesn't mess different requests' data together" do + Excon.stub({}, { body: "", status: 200 }) + response = Excon.get("http://example.com/path?foo=bar", mock: true) + expect(response.status).to eq(200) + + Excon.stub({}, { body: "", status: 404 }) + response = Excon.get("http://example.com/path?foo=bar", mock: true) + expect(response.status).to eq(404) + + verify_spans(transaction) + end + + context "with nested span" do + let(:span) { transaction.start_child(op: "child span") } + + before do + Sentry.get_current_scope.set_span(span) + end + + it "attaches http spans to the span instead of top-level transaction" do + Excon.stub({}, { body: "", status: 200 }) + response = Excon.get("http://example.com/path?foo=bar", mock: true) + expect(response.status).to eq(200) + + expect(transaction.span_recorder.spans.count).to eq(3) + expect(span.parent_span_id).to eq(transaction.span_id) + http_span = transaction.span_recorder.spans.last + expect(http_span.parent_span_id).to eq(span.span_id) + end + end + end + end + + context "without SDK" do + it "doesn't affect the HTTP lib anything" do + Excon.stub({}, { body: "", status: 200 }) + + response = Excon.get("http://example.com/path") + expect(response.status).to eq(200) + end + end +end diff --git a/sentry-ruby/spec/sentry_spec.rb b/sentry-ruby/spec/sentry_spec.rb index dabdcd07e..9a188e9f3 100644 --- a/sentry-ruby/spec/sentry_spec.rb +++ b/sentry-ruby/spec/sentry_spec.rb @@ -1235,5 +1235,15 @@ def foo; end expect(target_class.instance_methods).to include(:foo) end end + + context "with patch and block" do + it "raises error" do + expect do + described_class.register_patch(:bad_patch, module_patch, target_class) do + target_class.send(:prepend, module_patch) + end + end.to raise_error(ArgumentError, "Please provide either a patch and its target OR a block, but not both") + end + end end end