From 2e6b9abf054995b2a51009dfe678432be9087ba7 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Wed, 14 Aug 2024 13:13:30 -0700 Subject: [PATCH 1/5] feat: Add experimental metrics to Rack Emit http.server.request.duration metric if the application has the metrics API installed and enables the feature via config --- .../lib/opentelemetry/instrumentation/base.rb | 15 ++++- .../instrumentation/rack/instrumentation.rb | 4 +- .../rack/middlewares/event_handler.rb | 58 +++++++++++++++++++ 3 files changed, 73 insertions(+), 4 deletions(-) diff --git a/instrumentation/base/lib/opentelemetry/instrumentation/base.rb b/instrumentation/base/lib/opentelemetry/instrumentation/base.rb index ddf828955..594e8c947 100644 --- a/instrumentation/base/lib/opentelemetry/instrumentation/base.rb +++ b/instrumentation/base/lib/opentelemetry/instrumentation/base.rb @@ -189,7 +189,7 @@ def infer_version end end - attr_reader :name, :version, :config, :installed, :tracer + attr_reader :name, :version, :config, :installed, :tracer, :meter alias installed? installed @@ -205,9 +205,20 @@ def initialize(name, version, install_blk, present_blk, @installed = false @options = options @tracer = OpenTelemetry::Trace::Tracer.new + # Do we want to conditionally create a meter overall? + @meter = OpenTelemetry::Metrics::Meter.new if metrics_enabled? end # rubocop:enable Metrics/ParameterLists + def metrics_enabled? + # We need the API as a dependency to call metrics + # But, we can check for the SDK before we do any metrics-y things + # might be able to shore this up to run only on init to preven re-eval + result = defined?(OpenTelemetry::Metrics) && @config[:send_metrics] + puts "***** metrics_enabled? result = #{result.inspect}" + result + end + # Install instrumentation with the given config. The present? and compatible? # will be run first, and install will return false if either fail. Will # return true if install was completed successfully. @@ -215,12 +226,12 @@ def initialize(name, version, install_blk, present_blk, # @param [Hash] config The config for this instrumentation def install(config = {}) return true if installed? - @config = config_options(config) return false unless installable?(config) instance_exec(@config, &@install_blk) @tracer = OpenTelemetry.tracer_provider.tracer(name, version) + @meter = OpenTelemetry.meter_provider.meter(name, version: version) if metrics_enabled? @installed = true end diff --git a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/instrumentation.rb b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/instrumentation.rb index 3bfb68a3c..1bd811082 100644 --- a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/instrumentation.rb +++ b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/instrumentation.rb @@ -29,8 +29,8 @@ class Instrumentation < OpenTelemetry::Instrumentation::Base option :untraced_requests, default: nil, validate: :callable option :response_propagators, default: [], validate: :array # This option is only valid for applications using Rack 2.0 or greater - option :use_rack_events, default: true, validate: :boolean - + option :use_rack_events, default: true, validate: :boolean + option :send_metrics, default: false, validate: :boolean # Temporary Helper for Sinatra and ActionPack middleware to use during installation # # @example Default usage diff --git a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb index 5475a4fc5..a49b3dee5 100644 --- a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb +++ b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb @@ -158,6 +158,43 @@ def untraced_request?(env) false end + def record_http_server_request_duration_metric(span) + return unless metrics_enabled? && http_server_duration_histogram + # find span duration + # end - start / a billion to convert nanoseconds to seconds + duration = (span.end_timestamp - span.start_timestamp) / 10**9 + # Create attributes + # + attrs = {} + # pattern below goes + # stable convention + # current span convention + + # attrs['http.request.method'] + attrs['http.method'] = span.attributes['http.method'] + + # attrs['url.scheme'] + attrs['http.scheme'] = span.attributes['http.scheme'] + + # same in stable semconv + attrs['http.route'] = span.attributes['http.route'] + + # attrs['http.response.status.code'] + attrs['http.status_code'] = span.attributes['http.status_code'] + + # attrs['server.address'] ??? + # attrs['server.port'] ??? + # span includes host and port + attrs['http.host'] = span.attributes['http.host'] + + # attrs not currently in span payload + # attrs['network.protocol.version'] + # attrs['network.protocol.name'] + attrs['error.type'] = span.status.description if span.status.code == OpenTelemetry::Trace::Status::ERROR + + http_server_duration_histogram.record(duration, attributes: attrs) + end + # https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-http.md#name # # recommendation: span.name(s) should be low-cardinality (e.g., @@ -203,6 +240,7 @@ def detach_context(request) token, span = request.env[OTEL_TOKEN_AND_SPAN] span.finish OpenTelemetry::Context.detach(token) + record_http_server_request_duration_metric(span) rescue StandardError => e OpenTelemetry.handle_error(exception: e) end @@ -247,6 +285,26 @@ def tracer OpenTelemetry::Instrumentation::Rack::Instrumentation.instance.tracer end + def metrics_enabled? + OpenTelemetry::Instrumentation::Rack::Instrumentation.instance.metrics_enabled? + end + + def meter + # warn if no meter? + return @meter if defined?(@meter) + @meter = metrics_enabled? ? OpenTelemetry::Instrumentation::Rack::Instrumentation.instance.meter : nil + end + + def http_server_duration_histogram + # only want to make the view and the histogram once + # OpenTelemetry.meter_provider.add_view('http.server.request.duration', aggregation: OpenTelemetry::SDK::Metrics::Aggregation::ExplicitBucketHistogram.new(boundaries: [0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1, 2.5, 5, 7.5, 10])) + # Meter might be nil if metrics API isn't installed or isn't configured to send data + return @http_server_duration_histogram if defined?(@http_server_duration_histogram) + + @http_server_duration_histogram = nil unless meter + @http_server_duration_histogram = meter.create_histogram('http.server.request.duration', unit: 's', description: 'Duration of HTTP server requests.') + end + def config OpenTelemetry::Instrumentation::Rack::Instrumentation.instance.config end From 8338fc4da43f369e2210f4f0f5a313da5db34295 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Wed, 14 Aug 2024 13:17:04 -0700 Subject: [PATCH 2/5] chore: Remove puts --- .../base/lib/opentelemetry/instrumentation/base.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/instrumentation/base/lib/opentelemetry/instrumentation/base.rb b/instrumentation/base/lib/opentelemetry/instrumentation/base.rb index 594e8c947..9bcfee5b7 100644 --- a/instrumentation/base/lib/opentelemetry/instrumentation/base.rb +++ b/instrumentation/base/lib/opentelemetry/instrumentation/base.rb @@ -214,9 +214,7 @@ def metrics_enabled? # We need the API as a dependency to call metrics # But, we can check for the SDK before we do any metrics-y things # might be able to shore this up to run only on init to preven re-eval - result = defined?(OpenTelemetry::Metrics) && @config[:send_metrics] - puts "***** metrics_enabled? result = #{result.inspect}" - result + defined?(OpenTelemetry::Metrics) && @config[:send_metrics] end # Install instrumentation with the given config. The present? and compatible? From b031ceb8d308a1eda4e2655e23af2b0d43a82a9f Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Thu, 15 Aug 2024 15:33:57 -0700 Subject: [PATCH 3/5] chore: Misc --- .../base/lib/opentelemetry/instrumentation/base.rb | 13 ++++++++----- .../rack/middlewares/event_handler.rb | 11 +++++++++-- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/instrumentation/base/lib/opentelemetry/instrumentation/base.rb b/instrumentation/base/lib/opentelemetry/instrumentation/base.rb index 9bcfee5b7..13de6fea4 100644 --- a/instrumentation/base/lib/opentelemetry/instrumentation/base.rb +++ b/instrumentation/base/lib/opentelemetry/instrumentation/base.rb @@ -206,15 +206,17 @@ def initialize(name, version, install_blk, present_blk, @options = options @tracer = OpenTelemetry::Trace::Tracer.new # Do we want to conditionally create a meter overall? - @meter = OpenTelemetry::Metrics::Meter.new if metrics_enabled? + # @meter = OpenTelemetry::Metrics::Meter.new if metrics_enabled? end # rubocop:enable Metrics/ParameterLists def metrics_enabled? - # We need the API as a dependency to call metrics - # But, we can check for the SDK before we do any metrics-y things - # might be able to shore this up to run only on init to preven re-eval - defined?(OpenTelemetry::Metrics) && @config[:send_metrics] + # We need the API as a dependency to call metrics-y things in instrumentation + # However, the user needs to install it separately from base, because we + # do not want base to rely on experimental code + return @metrics_enabled if defined?(@metrics_enabled) + + @metrics_enabled ||= defined?(OpenTelemetry::Metrics) && @config[:send_metrics] end # Install instrumentation with the given config. The present? and compatible? @@ -224,6 +226,7 @@ def metrics_enabled? # @param [Hash] config The config for this instrumentation def install(config = {}) return true if installed? + @config = config_options(config) return false unless installable?(config) diff --git a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb index a49b3dee5..492b2809a 100644 --- a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb +++ b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb @@ -160,9 +160,10 @@ def untraced_request?(env) def record_http_server_request_duration_metric(span) return unless metrics_enabled? && http_server_duration_histogram + # find span duration # end - start / a billion to convert nanoseconds to seconds - duration = (span.end_timestamp - span.start_timestamp) / 10**9 + duration = (span.end_timestamp - span.start_timestamp) / (10**9) # Create attributes # attrs = {} @@ -292,12 +293,18 @@ def metrics_enabled? def meter # warn if no meter? return @meter if defined?(@meter) + @meter = metrics_enabled? ? OpenTelemetry::Instrumentation::Rack::Instrumentation.instance.meter : nil end def http_server_duration_histogram # only want to make the view and the histogram once - # OpenTelemetry.meter_provider.add_view('http.server.request.duration', aggregation: OpenTelemetry::SDK::Metrics::Aggregation::ExplicitBucketHistogram.new(boundaries: [0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1, 2.5, 5, 7.5, 10])) + # OpenTelemetry.meter_provider.add_view( + # 'http.server.request.duration', + # aggregation: OpenTelemetry::SDK::Metrics::Aggregation::ExplicitBucketHistogram.new( + # boundaries: [0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1, 2.5, 5, 7.5, 10] + # ) + # ) # Meter might be nil if metrics API isn't installed or isn't configured to send data return @http_server_duration_histogram if defined?(@http_server_duration_histogram) From d9521371e0a8db3e3189bcd19d0a26bf07091111 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Mon, 19 Aug 2024 09:47:43 -0700 Subject: [PATCH 4/5] fix: Make sure duration is a float Otherwise it might round to zero --- .../instrumentation/rack/middlewares/event_handler.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb index 492b2809a..b9b457805 100644 --- a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb +++ b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb @@ -163,7 +163,7 @@ def record_http_server_request_duration_metric(span) # find span duration # end - start / a billion to convert nanoseconds to seconds - duration = (span.end_timestamp - span.start_timestamp) / (10**9) + duration = (span.end_timestamp - span.start_timestamp) / Float(10**9) # Create attributes # attrs = {} From 36854b8279f8c7db2f716fd3ca3ba8be4e25e94f Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Mon, 19 Aug 2024 15:23:54 -0700 Subject: [PATCH 5/5] chore: Add comment, replace view with advice --- .../rack/middlewares/event_handler.rb | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb index b9b457805..cb64bd6f5 100644 --- a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb +++ b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb @@ -158,6 +158,8 @@ def untraced_request?(env) false end + # TODO: This one is long because I wanted to keep the stable semantic + # conventions, and (for now) emit attributes that matched the span def record_http_server_request_duration_metric(span) return unless metrics_enabled? && http_server_duration_histogram @@ -169,7 +171,7 @@ def record_http_server_request_duration_metric(span) attrs = {} # pattern below goes # stable convention - # current span convention + # attribute that matches rack spans (old convention) # attrs['http.request.method'] attrs['http.method'] = span.attributes['http.method'] @@ -298,14 +300,8 @@ def meter end def http_server_duration_histogram - # only want to make the view and the histogram once - # OpenTelemetry.meter_provider.add_view( - # 'http.server.request.duration', - # aggregation: OpenTelemetry::SDK::Metrics::Aggregation::ExplicitBucketHistogram.new( - # boundaries: [0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1, 2.5, 5, 7.5, 10] - # ) - # ) - # Meter might be nil if metrics API isn't installed or isn't configured to send data + # Only want to make the histogram once + # Need to implement advice so we can update the buckets to match seconds instead of ms return @http_server_duration_histogram if defined?(@http_server_duration_histogram) @http_server_duration_histogram = nil unless meter