From ef1eeb3a25fa8d557c2f2a60de843d4697dd6352 Mon Sep 17 00:00:00 2001 From: Peter Solnica Date: Mon, 5 Aug 2024 12:00:07 +0200 Subject: [PATCH] Unify Net::HTTP and Faraday implementations (#2360) * Unify Net::HTTP and Faraday implementations * Fix early return in Faraday when Sentry is not initialized --- sentry-ruby/Gemfile | 2 +- sentry-ruby/lib/sentry/faraday.rb | 50 +++---------------- sentry-ruby/lib/sentry/net/http.rb | 52 ++++++-------------- sentry-ruby/lib/sentry/utils/http_tracing.rb | 41 +++++++++++++++ sentry-ruby/spec/sentry/faraday_spec.rb | 22 +++++++++ 5 files changed, 87 insertions(+), 80 deletions(-) create mode 100644 sentry-ruby/lib/sentry/utils/http_tracing.rb diff --git a/sentry-ruby/Gemfile b/sentry-ruby/Gemfile index c6e1db011..a5e4967b9 100644 --- a/sentry-ruby/Gemfile +++ b/sentry-ruby/Gemfile @@ -15,7 +15,7 @@ gem "puma" gem "timecop" gem "stackprof" unless RUBY_PLATFORM == "java" -gem "graphql", ">= 2.2.6" if RUBY_VERSION.to_f >= 2.7 +gem "graphql", ">= 2.2.6", "< 2.3.11" if RUBY_VERSION.to_f >= 2.7 gem "benchmark-ips" gem "benchmark_driver" diff --git a/sentry-ruby/lib/sentry/faraday.rb b/sentry-ruby/lib/sentry/faraday.rb index e2608cd97..05d5f45bd 100644 --- a/sentry-ruby/lib/sentry/faraday.rb +++ b/sentry-ruby/lib/sentry/faraday.rb @@ -3,8 +3,6 @@ module Sentry module Faraday OP_NAME = "http.client" - SPAN_ORIGIN = "auto.http.faraday" - BREADCRUMB_CATEGORY = "http" module Connection # Since there's no way to preconfigure Faraday connections and add our instrumentation @@ -25,14 +23,13 @@ def initialize(url = nil, options = nil) end class Instrumenter - attr_reader :configuration + SPAN_ORIGIN = "auto.http.faraday" + BREADCRUMB_CATEGORY = "http" - def initialize - @configuration = Sentry.configuration - end + include Utils::HttpTracing def instrument(op_name, env, &block) - return unless Sentry.initialized? + return block.call unless Sentry.initialized? Sentry.with_child_span(op: op_name, start_timestamp: Sentry.utc_now.to_f, origin: SPAN_ORIGIN) do |sentry_span| request_info = extract_request_info(env) @@ -42,17 +39,14 @@ def instrument(op_name, env, &block) end res = block.call + response_status = res.status if record_sentry_breadcrumb? - record_sentry_breadcrumb(request_info, res) + record_sentry_breadcrumb(request_info, response_status) end if sentry_span - sentry_span.set_description("#{request_info[:method]} #{request_info[:url]}") - sentry_span.set_data(Span::DataConventions::URL, request_info[:url]) - sentry_span.set_data(Span::DataConventions::HTTP_METHOD, request_info[:method]) - sentry_span.set_data(Span::DataConventions::HTTP_QUERY, request_info[:query]) if request_info[:query] - sentry_span.set_data(Span::DataConventions::HTTP_STATUS_CODE, res.status) + set_span_info(sentry_span, request_info, response_status) end res @@ -65,41 +59,13 @@ def extract_request_info(env) url = env[:url].scheme + "://" + env[:url].host + env[:url].path result = { method: env[:method].to_s.upcase, url: url } - if configuration.send_default_pii + if Sentry.configuration.send_default_pii result[:query] = env[:url].query result[:body] = env[:body] end result end - - def record_sentry_breadcrumb(request_info, res) - crumb = Sentry::Breadcrumb.new( - level: :info, - category: BREADCRUMB_CATEGORY, - type: :info, - data: { - status: res.status, - **request_info - } - ) - - Sentry.add_breadcrumb(crumb) - end - - def record_sentry_breadcrumb? - configuration.breadcrumbs_logger.include?(:http_logger) - end - - def propagate_trace?(url) - url && - configuration.propagate_traces && - configuration.trace_propagation_targets.any? { |target| url.match?(target) } - end - - def set_propagation_headers(headers) - Sentry.get_trace_propagation_headers&.each { |k, v| headers[k] = v } - end end end end diff --git a/sentry-ruby/lib/sentry/net/http.rb b/sentry-ruby/lib/sentry/net/http.rb index 205c79c55..c769b4c3d 100644 --- a/sentry-ruby/lib/sentry/net/http.rb +++ b/sentry-ruby/lib/sentry/net/http.rb @@ -2,11 +2,14 @@ require "net/http" require "resolv" +require "sentry/utils/http_tracing" module Sentry # @api private module Net module HTTP + include Utils::HttpTracing + OP_NAME = "http.client" SPAN_ORIGIN = "auto.http.net_http" BREADCRUMB_CATEGORY = "net.http" @@ -21,8 +24,7 @@ module HTTP # req['connection'] ||= 'close' # return request(req, body, &block) # <- request will be called for the second time from the first call # } - # end - # # ..... + # end # ..... # end # ``` # @@ -34,44 +36,26 @@ def request(req, body = nil, &block) Sentry.with_child_span(op: OP_NAME, start_timestamp: Sentry.utc_now.to_f, origin: SPAN_ORIGIN) do |sentry_span| request_info = extract_request_info(req) - if propagate_trace?(request_info[:url], Sentry.configuration) + if propagate_trace?(request_info[:url]) set_propagation_headers(req) end - super.tap do |res| - record_sentry_breadcrumb(request_info, res) + res = super + response_status = res.code.to_i - if sentry_span - sentry_span.set_description("#{request_info[:method]} #{request_info[:url]}") - sentry_span.set_data(Span::DataConventions::URL, request_info[:url]) - sentry_span.set_data(Span::DataConventions::HTTP_METHOD, request_info[:method]) - sentry_span.set_data(Span::DataConventions::HTTP_QUERY, request_info[:query]) if request_info[:query] - sentry_span.set_data(Span::DataConventions::HTTP_STATUS_CODE, res.code.to_i) - end + if record_sentry_breadcrumb? + record_sentry_breadcrumb(request_info, response_status) end - end - end - private + if sentry_span + set_span_info(sentry_span, request_info, response_status) + end - def set_propagation_headers(req) - Sentry.get_trace_propagation_headers&.each { |k, v| req[k] = v } + res + end end - def record_sentry_breadcrumb(request_info, res) - return unless Sentry.initialized? && Sentry.configuration.breadcrumbs_logger.include?(:http_logger) - - crumb = Sentry::Breadcrumb.new( - level: :info, - category: BREADCRUMB_CATEGORY, - type: :info, - data: { - status: res.code.to_i, - **request_info - } - ) - Sentry.add_breadcrumb(crumb) - end + private def from_sentry_sdk? dsn = Sentry.configuration.dsn @@ -94,12 +78,6 @@ def extract_request_info(req) result end - - def propagate_trace?(url, configuration) - url && - configuration.propagate_traces && - configuration.trace_propagation_targets.any? { |target| url.match?(target) } - end end end end diff --git a/sentry-ruby/lib/sentry/utils/http_tracing.rb b/sentry-ruby/lib/sentry/utils/http_tracing.rb new file mode 100644 index 000000000..abf6a9141 --- /dev/null +++ b/sentry-ruby/lib/sentry/utils/http_tracing.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +module Sentry + module Utils + module HttpTracing + def set_span_info(sentry_span, request_info, response_status) + sentry_span.set_description("#{request_info[:method]} #{request_info[:url]}") + sentry_span.set_data(Span::DataConventions::URL, request_info[:url]) + sentry_span.set_data(Span::DataConventions::HTTP_METHOD, request_info[:method]) + sentry_span.set_data(Span::DataConventions::HTTP_QUERY, request_info[:query]) if request_info[:query] + sentry_span.set_data(Span::DataConventions::HTTP_STATUS_CODE, response_status) + end + + def set_propagation_headers(req) + Sentry.get_trace_propagation_headers&.each { |k, v| req[k] = v } + end + + def record_sentry_breadcrumb(request_info, response_status) + crumb = Sentry::Breadcrumb.new( + level: :info, + category: self.class::BREADCRUMB_CATEGORY, + type: :info, + data: { status: response_status, **request_info } + ) + + Sentry.add_breadcrumb(crumb) + end + + def record_sentry_breadcrumb? + Sentry.initialized? && Sentry.configuration.breadcrumbs_logger.include?(:http_logger) + end + + def propagate_trace?(url) + url && + Sentry.initialized? && + Sentry.configuration.propagate_traces && + Sentry.configuration.trace_propagation_targets.any? { |target| url.match?(target) } + end + end + end +end diff --git a/sentry-ruby/spec/sentry/faraday_spec.rb b/sentry-ruby/spec/sentry/faraday_spec.rb index abb63ddb5..f4632032f 100644 --- a/sentry-ruby/spec/sentry/faraday_spec.rb +++ b/sentry-ruby/spec/sentry/faraday_spec.rb @@ -254,4 +254,26 @@ expect(transaction.span_recorder.spans.map(&:origin)).not_to include("auto.http.faraday") end end + + context "when Sentry is not initialized" do + let(:http) do + Faraday.new(url) do |f| + f.adapter Faraday::Adapter::Test do |stub| + stub.get("/test") do + [200, { "Content-Type" => "text/html" }, "

hello world

"] + end + end + end + end + + let(:url) { "http://example.com" } + + it "skips instrumentation" do + allow(Sentry).to receive(:initialized?).and_return(false) + + response = http.get("/test") + + expect(response.status).to eq(200) + end + end end