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

feat: 🦠 Metrics prototype for Rack instrumentation #1129

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open
16 changes: 15 additions & 1 deletion instrumentation/base/lib/opentelemetry/instrumentation/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#
# SPDX-License-Identifier: Apache-2.0


kaylareopelle marked this conversation as resolved.
Show resolved Hide resolved
module OpenTelemetry
module Instrumentation
# The Base class holds all metadata and configuration for an
Expand Down Expand Up @@ -189,7 +190,7 @@ def infer_version
end
end

attr_reader :name, :version, :config, :installed, :tracer
attr_reader :name, :version, :config, :installed, :tracer, :meter

alias installed? installed

Expand All @@ -205,6 +206,8 @@ def initialize(name, version, install_blk, present_blk,
@installed = false
@options = options
@tracer = OpenTelemetry::Trace::Tracer.new
# 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

Expand All @@ -221,9 +224,20 @@ def install(config = {})

instance_exec(@config, &@install_blk)
@tracer = OpenTelemetry.tracer_provider.tracer(name, version)
install_meter
@installed = true
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

# 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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,9 @@ 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
# TODO: This option currently exclusively uses the event handler, should we support old and new Rack?
option :send_metrics, default: false, validate: :boolean
kaylareopelle marked this conversation as resolved.
Show resolved Hide resolved
# Temporary Helper for Sinatra and ActionPack middleware to use during installation
#
# @example Default usage
Expand All @@ -47,6 +48,7 @@ def middleware_args
end
end


kaylareopelle marked this conversation as resolved.
Show resolved Hide resolved
private

def require_dependencies
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ def untraced_request?(env)
false
end


kaylareopelle marked this conversation as resolved.
Show resolved Hide resolved
# https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-http.md#name
#
# recommendation: span.name(s) should be low-cardinality (e.g.,
Expand Down Expand Up @@ -203,6 +204,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
Expand Down Expand Up @@ -262,6 +264,38 @@ 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
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

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.')
kaylareopelle marked this conversation as resolved.
Show resolved Hide resolved
end

def record_http_server_request_duration_metric(span)
return unless metrics_enabled?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a bunch of ideas and questions floating around in my head that could use your input being a metrics SDK expert.

It may result in a separate dispatch call, but have you considered creating a separate handler for metric generation? That would remove the need for having conditionals in the tracer middleware.

What if our user has an o11y 1.0 mindset and only wanted to generate metrics and not traces? Would that not mean the span would be non_recording and the metric would always generate invalid histogram entries?

Is there anything in the spec that states this should be generated from span timestamps?

Is there an alternative implementation that generates metrics in the span processor pipeline?

E.g. there is a a processor that implements on_finish generated metrics for server and client spans regardless of the instrumentation?

What about one that decorates the BatchSpanProcessor export loop and generates metrics in the bsp thread to minimize the metrics from being generated in the critical path of the users request?

# find span duration
# 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) }
kaylareopelle marked this conversation as resolved.
Show resolved Hide resolved
# 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)
end
end
end
end
Expand Down
Loading