From 2e6b9abf054995b2a51009dfe678432be9087ba7 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Wed, 14 Aug 2024 13:13:30 -0700 Subject: [PATCH 01/10] 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 02/10] 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 03/10] 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 ed4a354e96e7189ed0dbdb62172fa17987aa116b Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Mon, 19 Aug 2024 09:47:14 -0700 Subject: [PATCH 04/10] fix: Make sure duration is a float Otherwise it may 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 a38aa875a1904e4f1974625c92030edcfb9eda6f Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Mon, 19 Aug 2024 15:08:34 -0700 Subject: [PATCH 05/10] feat: Create MetricsPatch modules to hold metrics Like the Metrics ConfigurationPatch, include the MetricsPatch modules if Metrics is enabled/present --- .../base/lib/opentelemetry/instrumentation.rb | 1 + .../lib/opentelemetry/instrumentation/base.rb | 26 ++++--- .../instrumentation/metrics_patch.rb | 21 ++++++ .../instrumentation/rack/instrumentation.rb | 7 ++ .../rack/middlewares/event_handler.rb | 67 ++----------------- .../rack/middlewares/metrics_patch.rb | 58 ++++++++++++++++ 6 files changed, 108 insertions(+), 72 deletions(-) create mode 100644 instrumentation/base/lib/opentelemetry/instrumentation/metrics_patch.rb create mode 100644 instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/metrics_patch.rb diff --git a/instrumentation/base/lib/opentelemetry/instrumentation.rb b/instrumentation/base/lib/opentelemetry/instrumentation.rb index c02712b64..98b5e6f8f 100644 --- a/instrumentation/base/lib/opentelemetry/instrumentation.rb +++ b/instrumentation/base/lib/opentelemetry/instrumentation.rb @@ -7,6 +7,7 @@ require 'opentelemetry' require 'opentelemetry-registry' require 'opentelemetry/instrumentation/base' +require 'opentelemetry/instrumentation/metrics_patch' if defined?(OpenTelemetry::Metrics) # maybe also add Env var check? module OpenTelemetry # The instrumentation module contains functionality to register and install diff --git a/instrumentation/base/lib/opentelemetry/instrumentation/base.rb b/instrumentation/base/lib/opentelemetry/instrumentation/base.rb index 13de6fea4..a3a75d028 100644 --- a/instrumentation/base/lib/opentelemetry/instrumentation/base.rb +++ b/instrumentation/base/lib/opentelemetry/instrumentation/base.rb @@ -4,6 +4,9 @@ # # SPDX-License-Identifier: Apache-2.0 +# TODO: Maybe add an env var for globally enabling metrics as second switch? +require_relative 'metrics_patch' if defined?(OpenTelemetry::Metrics) + module OpenTelemetry module Instrumentation # The Base class holds all metadata and configuration for an @@ -205,19 +208,12 @@ 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? + create_meter end # rubocop:enable Metrics/ParameterLists - def metrics_enabled? - # 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 + # no-op, overridden in metrics patch + def create_meter; end # Install instrumentation with the given config. The present? and compatible? # will be run first, and install will return false if either fail. Will @@ -232,10 +228,18 @@ def install(config = {}) instance_exec(@config, &@install_blk) @tracer = OpenTelemetry.tracer_provider.tracer(name, version) - @meter = OpenTelemetry.meter_provider.meter(name, version: version) if metrics_enabled? + install_meter @installed = true end + # no-op, defined in metrics patch to use if metrics installed + def install_meter; end + + # Metrics should not be enabled if we're trying to load them directly from base + def metrics_enabled? + false + end + # Whether or not this instrumentation is installable in the current process. Will # be true when the instrumentation defines an install block, is not disabled # by environment or config, and the target library present and compatible. diff --git a/instrumentation/base/lib/opentelemetry/instrumentation/metrics_patch.rb b/instrumentation/base/lib/opentelemetry/instrumentation/metrics_patch.rb new file mode 100644 index 000000000..3450c1d36 --- /dev/null +++ b/instrumentation/base/lib/opentelemetry/instrumentation/metrics_patch.rb @@ -0,0 +1,21 @@ +module OpenTelemetry + module Instrumentation + module MetricsPatch + def create_meter + @meter = OpenTelemetry::Metrics::Meter.new + end + + def install_meter + @meter = OpenTelemetry.meter_provider.meter(name, version: version) if metrics_enabled? + end + + def metrics_enabled? + return @metrics_enabled if defined?(@metrics_enabled) + + @metrics_enabled ||= defined?(OpenTelemetry::Metrics) && @config[:send_metrics] + end + end + end +end + +OpenTelemetry::Instrumentation::Base.prepend(OpenTelemetry::Instrumentation::MetricsPatch) diff --git a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/instrumentation.rb b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/instrumentation.rb index 1bd811082..fb0102eb1 100644 --- a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/instrumentation.rb +++ b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/instrumentation.rb @@ -30,6 +30,7 @@ class Instrumentation < OpenTelemetry::Instrumentation::Base 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 + # TODO: This option currently exclusively uses the event handler, should we support old and new Rack? option :send_metrics, default: false, validate: :boolean # Temporary Helper for Sinatra and ActionPack middleware to use during installation # @@ -47,10 +48,16 @@ def middleware_args end end + # def metrics_enabled? + # super + # # other conditions unique to Rack? Like Events also being available? + # end + private def require_dependencies require_relative 'middlewares/event_handler' if defined?(::Rack::Events) + require_relative 'middlewares/metrics_patch' if metrics_enabled? require_relative 'middlewares/tracer_middleware' end 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..e1a0fb8c4 100644 --- a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb +++ b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb @@ -158,43 +158,8 @@ 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) / Float(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 + # no-op, defined in MetricsPatch and required if metrics enabled + def record_http_server_request_duration_metric(span); end # https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-http.md#name # @@ -286,31 +251,11 @@ 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) + # no-op, overwritten by metrics patch if metrics is enabled + def meter; end - @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 + # no-op, overwritten by metrics patch if metrics is enabled + def http_server_request_duration_histogram; end def config OpenTelemetry::Instrumentation::Rack::Instrumentation.instance.config diff --git a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/metrics_patch.rb b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/metrics_patch.rb new file mode 100644 index 000000000..8429bb167 --- /dev/null +++ b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/metrics_patch.rb @@ -0,0 +1,58 @@ +module OpenTelemetry + module Instrumentation + module Rack + module Middlewares + module MetricsPatch + # Don't check in here to see if metrics is enabled, trust that if it's required, metrics will be enabled + def meter + OpenTelemetry::Instrumentation::Rack::Instrumentation.instance.meter + end + + def http_server_request_duration_histogram + # TODO: Add Advice to set small explicit histogram bucket boundaries + # TODO: Does this need to be memoized? + @http_server_request_duration_histogram ||= meter.create_histogram('http.server.request.duration', unit: 's', description: 'Duration of HTTP server requests.') + end + + 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) / Float(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_request_duration_histogram.record(duration, attributes: attrs) + end + end + end + end + end +end + +OpenTelemetry::Instrumentation::Rack::Middlewares::EventHandler.prepend(OpenTelemetry::Instrumentation::Rack::Middlewares::MetricsPatch) From 92d9ef6cf1bce0822c3d35e6ee8bdc1c80dfae7b Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Thu, 19 Sep 2024 17:33:43 -0700 Subject: [PATCH 06/10] style: Update comments, add file header --- .../instrumentation/metrics_patch.rb | 10 ++++++++++ .../rack/middlewares/metrics_patch.rb | 16 +++++++++++++--- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/instrumentation/base/lib/opentelemetry/instrumentation/metrics_patch.rb b/instrumentation/base/lib/opentelemetry/instrumentation/metrics_patch.rb index 3450c1d36..b479bf36c 100644 --- a/instrumentation/base/lib/opentelemetry/instrumentation/metrics_patch.rb +++ b/instrumentation/base/lib/opentelemetry/instrumentation/metrics_patch.rb @@ -1,5 +1,15 @@ +# frozen_string_literal: true + +# Copyright The OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + module OpenTelemetry module Instrumentation + # MetricsPatch is a module that provides functionality to create a meter + # and record metrics if both the opentelemetry-metrics-api is present + # and the instrumentation to emit metrics has enabled metrics by setting + # :send_metrics to true module MetricsPatch def create_meter @meter = OpenTelemetry::Metrics::Meter.new diff --git a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/metrics_patch.rb b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/metrics_patch.rb index 8429bb167..b4c1299e1 100644 --- a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/metrics_patch.rb +++ b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/metrics_patch.rb @@ -1,9 +1,18 @@ +# frozen_string_literal: true + +# Copyright The OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + module OpenTelemetry module Instrumentation module Rack module Middlewares + # MetricsPatch is a module that provides functionality to record metrics + # if both the opentelemetry-metrics-api is present and the rack + # instrumentation is configured to emit metrics by setting + # :send_metrics to true module MetricsPatch - # Don't check in here to see if metrics is enabled, trust that if it's required, metrics will be enabled def meter OpenTelemetry::Instrumentation::Rack::Instrumentation.instance.meter end @@ -14,6 +23,7 @@ def http_server_request_duration_histogram @http_server_request_duration_histogram ||= meter.create_histogram('http.server.request.duration', unit: 's', description: 'Duration of HTTP server requests.') end + # TODO: Update this to define attributes based on SEMCONV_STABILITY_OPT_IN once available def record_http_server_request_duration_metric(span) # find span duration # end - start / a billion to convert nanoseconds to seconds @@ -21,8 +31,8 @@ def record_http_server_request_duration_metric(span) # Create attributes # attrs = {} - # pattern below goes - # stable convention + # pattern below goes: + # # stable convention # current span convention # attrs['http.request.method'] From 3eef72a647bef5059382072d10425bd3f07b070d Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Thu, 3 Oct 2024 16:24:06 -0700 Subject: [PATCH 07/10] refactor: Switch from prepending to conditions --- .../base/lib/opentelemetry/instrumentation.rb | 1 - .../lib/opentelemetry/instrumentation/base.rb | 18 +++-- .../instrumentation/metrics_patch.rb | 31 --------- .../instrumentation/rack/instrumentation.rb | 5 -- .../rack/middlewares/event_handler.rb | 43 +++++++++--- .../rack/middlewares/metrics_patch.rb | 68 ------------------- 6 files changed, 43 insertions(+), 123 deletions(-) delete mode 100644 instrumentation/base/lib/opentelemetry/instrumentation/metrics_patch.rb delete mode 100644 instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/metrics_patch.rb diff --git a/instrumentation/base/lib/opentelemetry/instrumentation.rb b/instrumentation/base/lib/opentelemetry/instrumentation.rb index 98b5e6f8f..c02712b64 100644 --- a/instrumentation/base/lib/opentelemetry/instrumentation.rb +++ b/instrumentation/base/lib/opentelemetry/instrumentation.rb @@ -7,7 +7,6 @@ require 'opentelemetry' require 'opentelemetry-registry' require 'opentelemetry/instrumentation/base' -require 'opentelemetry/instrumentation/metrics_patch' if defined?(OpenTelemetry::Metrics) # maybe also add Env var check? module OpenTelemetry # The instrumentation module contains functionality to register and install diff --git a/instrumentation/base/lib/opentelemetry/instrumentation/base.rb b/instrumentation/base/lib/opentelemetry/instrumentation/base.rb index a3a75d028..7972f2acb 100644 --- a/instrumentation/base/lib/opentelemetry/instrumentation/base.rb +++ b/instrumentation/base/lib/opentelemetry/instrumentation/base.rb @@ -4,8 +4,6 @@ # # SPDX-License-Identifier: Apache-2.0 -# TODO: Maybe add an env var for globally enabling metrics as second switch? -require_relative 'metrics_patch' if defined?(OpenTelemetry::Metrics) module OpenTelemetry module Instrumentation @@ -208,13 +206,11 @@ def initialize(name, version, install_blk, present_blk, @installed = false @options = options @tracer = OpenTelemetry::Trace::Tracer.new - create_meter + # check to see if the API is defined here because the config isn't available yet + @meter = OpenTelemetry::Metrics::Meter.new if defined?(OpenTelemetry::Metrics) end # rubocop:enable Metrics/ParameterLists - # no-op, overridden in metrics patch - def create_meter; 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. @@ -232,12 +228,14 @@ def install(config = {}) @installed = true end - # no-op, defined in metrics patch to use if metrics installed - def install_meter; end + def install_meter + @meter = OpenTelemetry.meter_provider.meter(name, version: version) if metrics_enabled? + end - # Metrics should not be enabled if we're trying to load them directly from base def metrics_enabled? - false + return @metrics_enabled if defined?(@metrics_enabled) + + @metrics_enabled ||= defined?(OpenTelemetry::Metrics) && @config[:send_metrics] end # Whether or not this instrumentation is installable in the current process. Will diff --git a/instrumentation/base/lib/opentelemetry/instrumentation/metrics_patch.rb b/instrumentation/base/lib/opentelemetry/instrumentation/metrics_patch.rb deleted file mode 100644 index b479bf36c..000000000 --- a/instrumentation/base/lib/opentelemetry/instrumentation/metrics_patch.rb +++ /dev/null @@ -1,31 +0,0 @@ -# frozen_string_literal: true - -# Copyright The OpenTelemetry Authors -# -# SPDX-License-Identifier: Apache-2.0 - -module OpenTelemetry - module Instrumentation - # MetricsPatch is a module that provides functionality to create a meter - # and record metrics if both the opentelemetry-metrics-api is present - # and the instrumentation to emit metrics has enabled metrics by setting - # :send_metrics to true - module MetricsPatch - def create_meter - @meter = OpenTelemetry::Metrics::Meter.new - end - - def install_meter - @meter = OpenTelemetry.meter_provider.meter(name, version: version) if metrics_enabled? - end - - def metrics_enabled? - return @metrics_enabled if defined?(@metrics_enabled) - - @metrics_enabled ||= defined?(OpenTelemetry::Metrics) && @config[:send_metrics] - end - end - end -end - -OpenTelemetry::Instrumentation::Base.prepend(OpenTelemetry::Instrumentation::MetricsPatch) diff --git a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/instrumentation.rb b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/instrumentation.rb index fb0102eb1..31e5cebb6 100644 --- a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/instrumentation.rb +++ b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/instrumentation.rb @@ -48,16 +48,11 @@ def middleware_args end end - # def metrics_enabled? - # super - # # other conditions unique to Rack? Like Events also being available? - # end private def require_dependencies require_relative 'middlewares/event_handler' if defined?(::Rack::Events) - require_relative 'middlewares/metrics_patch' if metrics_enabled? require_relative 'middlewares/tracer_middleware' end 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 e1a0fb8c4..0e17f6fa2 100644 --- a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb +++ b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb @@ -158,8 +158,6 @@ def untraced_request?(env) false end - # no-op, defined in MetricsPatch and required if metrics enabled - def record_http_server_request_duration_metric(span); end # https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-http.md#name # @@ -251,12 +249,6 @@ def tracer OpenTelemetry::Instrumentation::Rack::Instrumentation.instance.tracer end - # no-op, overwritten by metrics patch if metrics is enabled - def meter; end - - # no-op, overwritten by metrics patch if metrics is enabled - def http_server_request_duration_histogram; end - def config OpenTelemetry::Instrumentation::Rack::Instrumentation.instance.config end @@ -272,6 +264,41 @@ def create_span(parent_context, request) span.add_event('http.proxy.request.started', timestamp: request_start_time) unless request_start_time.nil? span end + + # Metrics stuff + def metrics_enabled? + OpenTelemetry::Instrumentation::Rack::Instrumentation.instance.metrics_enabled? + end + + def meter + return unless metrics_enabled? + + OpenTelemetry::Instrumentation::Rack::Instrumentation.instance.meter + end + + def http_server_request_duration_histogram + return unless metrics_enabled? + + @http_server_request_duration_histogram ||= meter.create_histogram('http.server.request.duration', unit: 's', description: 'Duration of HTTP server requests.') + end + + def record_http_server_request_duration_metric(span) + return unless metrics_enabled? + # find span duration + # end - start / a billion to convert nanoseconds to seconds + duration = (span.end_timestamp - span.start_timestamp) / Float(10**9) + # Create attributes + # + attrs = {} + attrs['http.method'] = span.attributes['http.method'] + attrs['http.scheme'] = span.attributes['http.scheme'] + attrs['http.route'] = span.attributes['http.route'] + attrs['http.status_code'] = span.attributes['http.status_code'] + attrs['http.host'] = span.attributes['http.host'] + attrs['error.type'] = span.status.description if span.status.code == OpenTelemetry::Trace::Status::ERROR + + http_server_request_duration_histogram.record(duration, attributes: attrs) + end end end end diff --git a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/metrics_patch.rb b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/metrics_patch.rb deleted file mode 100644 index b4c1299e1..000000000 --- a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/metrics_patch.rb +++ /dev/null @@ -1,68 +0,0 @@ -# frozen_string_literal: true - -# Copyright The OpenTelemetry Authors -# -# SPDX-License-Identifier: Apache-2.0 - -module OpenTelemetry - module Instrumentation - module Rack - module Middlewares - # MetricsPatch is a module that provides functionality to record metrics - # if both the opentelemetry-metrics-api is present and the rack - # instrumentation is configured to emit metrics by setting - # :send_metrics to true - module MetricsPatch - def meter - OpenTelemetry::Instrumentation::Rack::Instrumentation.instance.meter - end - - def http_server_request_duration_histogram - # TODO: Add Advice to set small explicit histogram bucket boundaries - # TODO: Does this need to be memoized? - @http_server_request_duration_histogram ||= meter.create_histogram('http.server.request.duration', unit: 's', description: 'Duration of HTTP server requests.') - end - - # TODO: Update this to define attributes based on SEMCONV_STABILITY_OPT_IN once available - 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) / Float(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_request_duration_histogram.record(duration, attributes: attrs) - end - end - end - end - end -end - -OpenTelemetry::Instrumentation::Rack::Middlewares::EventHandler.prepend(OpenTelemetry::Instrumentation::Rack::Middlewares::MetricsPatch) From 258b5874a640dfd9f9b88861f79b788ca4ecd418 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Thu, 3 Oct 2024 16:31:22 -0700 Subject: [PATCH 08/10] refactor: Assign attributes using select --- .../rack/middlewares/event_handler.rb | 13 +++++-------- 1 file changed, 5 insertions(+), 8 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 0e17f6fa2..f959031ec 100644 --- a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb +++ b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb @@ -266,6 +266,8 @@ def create_span(parent_context, request) end # Metrics stuff + HTTP_SERVER_REQUEST_DURATION_ATTRS_FROM_SPAN = %w[http.method http.scheme http.route http.status_code http.host].freeze + def metrics_enabled? OpenTelemetry::Instrumentation::Rack::Instrumentation.instance.metrics_enabled? end @@ -287,14 +289,9 @@ 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) / Float(10**9) - # Create attributes - # - attrs = {} - attrs['http.method'] = span.attributes['http.method'] - attrs['http.scheme'] = span.attributes['http.scheme'] - attrs['http.route'] = span.attributes['http.route'] - attrs['http.status_code'] = span.attributes['http.status_code'] - attrs['http.host'] = span.attributes['http.host'] + # glean attributes + attrs = span.attributes.select {|k, _v| HTTP_SERVER_REQUEST_DURATION_ATTRS_FROM_SPAN.include?(k) } + # set error attrs['error.type'] = span.status.description if span.status.code == OpenTelemetry::Trace::Status::ERROR http_server_request_duration_histogram.record(duration, attributes: attrs) From ba9d06d8085cfbe36a61622698461a5db64b54d7 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle <87386821+kaylareopelle@users.noreply.github.com> Date: Thu, 3 Oct 2024 16:58:37 -0700 Subject: [PATCH 09/10] style: Spacing updates --- .../base/lib/opentelemetry/instrumentation/base.rb | 1 - .../instrumentation/rack/instrumentation.rb | 2 +- .../instrumentation/rack/middlewares/event_handler.rb | 9 ++++++--- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/instrumentation/base/lib/opentelemetry/instrumentation/base.rb b/instrumentation/base/lib/opentelemetry/instrumentation/base.rb index 7972f2acb..42c6f0bb3 100644 --- a/instrumentation/base/lib/opentelemetry/instrumentation/base.rb +++ b/instrumentation/base/lib/opentelemetry/instrumentation/base.rb @@ -4,7 +4,6 @@ # # SPDX-License-Identifier: Apache-2.0 - module OpenTelemetry module Instrumentation # The Base class holds all metadata and configuration for an diff --git a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/instrumentation.rb b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/instrumentation.rb index 31e5cebb6..91cd44577 100644 --- a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/instrumentation.rb +++ b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/instrumentation.rb @@ -32,6 +32,7 @@ class Instrumentation < OpenTelemetry::Instrumentation::Base option :use_rack_events, default: true, validate: :boolean # TODO: This option currently exclusively uses the event handler, should we support old and new Rack? option :send_metrics, default: false, validate: :boolean + # Temporary Helper for Sinatra and ActionPack middleware to use during installation # # @example Default usage @@ -48,7 +49,6 @@ def middleware_args end end - private def require_dependencies 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 f959031ec..2bb9f6236 100644 --- a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb +++ b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb @@ -158,7 +158,6 @@ def untraced_request?(env) false 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., @@ -281,7 +280,11 @@ def meter def http_server_request_duration_histogram return unless metrics_enabled? - @http_server_request_duration_histogram ||= meter.create_histogram('http.server.request.duration', unit: 's', description: 'Duration of HTTP server requests.') + @http_server_request_duration_histogram ||= meter.create_histogram( + 'http.server.request.duration', + unit: 's', + description: 'Duration of HTTP server requests.' + ) end def record_http_server_request_duration_metric(span) @@ -290,7 +293,7 @@ def record_http_server_request_duration_metric(span) # end - start / a billion to convert nanoseconds to seconds duration = (span.end_timestamp - span.start_timestamp) / Float(10**9) # glean attributes - attrs = span.attributes.select {|k, _v| HTTP_SERVER_REQUEST_DURATION_ATTRS_FROM_SPAN.include?(k) } + attrs = span.attributes.select { |k, _v| HTTP_SERVER_REQUEST_DURATION_ATTRS_FROM_SPAN.include?(k) } # set error attrs['error.type'] = span.status.description if span.status.code == OpenTelemetry::Trace::Status::ERROR From 7f8ce164182467381e9219fbebeee5dbc88cf79b Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Thu, 3 Oct 2024 17:11:40 -0700 Subject: [PATCH 10/10] style: Rubocop --- .../instrumentation/rack/middlewares/event_handler.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 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 2bb9f6236..e7dd6a351 100644 --- a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb +++ b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb @@ -281,14 +281,15 @@ def http_server_request_duration_histogram return unless metrics_enabled? @http_server_request_duration_histogram ||= meter.create_histogram( - 'http.server.request.duration', - unit: 's', + 'http.server.request.duration', + unit: 's', description: 'Duration of HTTP server requests.' ) end def record_http_server_request_duration_metric(span) return unless metrics_enabled? + # find span duration # end - start / a billion to convert nanoseconds to seconds duration = (span.end_timestamp - span.start_timestamp) / Float(10**9)