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.

Sign up for GitHub

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

Jump to bottom

feat: add sem_conv opt in #1166

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

feat: add sem_conv opt in #1166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6d0da5a
feat: add sem_conv opt in
hannahramadan Sep 13, 2024
8f28f64
Cleanup code with `tap`
hannahramadan Sep 16, 2024
c6b45fb
Apply suggestions from code review
hannahramadan Sep 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions instrumentation/http/lib/opentelemetry/instrumentation/http/instrumentation.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,12 @@ module Instrumentation
module HTTP
# The Instrumentation class contains logic to detect and install the Http instrumentation
class Instrumentation < OpenTelemetry::Instrumentation::Base
attr_reader :sem_conv

install do |_config|
require_dependencies
patch
@sem_conv = OpenTelemetry::SemanticConventions::StabilityMode.new
end

present do
Expand Down
30 changes: 20 additions & 10 deletions instrumentation/http/lib/opentelemetry/instrumentation/http/patches/client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,17 @@ def perform(req, options)
request_method = req.verb.to_s.upcase
span_name = create_request_span_name(request_method, uri.path)

attributes = {
'http.method' => request_method,
'http.scheme' => uri.scheme,
'http.target' => uri.path,
'http.url' => "#{uri.scheme}://#{uri.host}",
'net.peer.name' => uri.host,
'net.peer.port' => uri.port
}.merge!(OpenTelemetry::Common::HTTP::ClientContext.attributes)
attributes = {}
attributes.tap do |attrs|
sem_conv.set_http_method(attrs, request_method)
sem_conv.set_http_scheme(attrs, uri.scheme)
sem_conv.set_http_target(attrs, uri.path, uri.query)
sem_conv.set_http_url(attrs, "#{uri.scheme}://#{uri.host}")
sem_conv.set_http_net_peer_name_client(attrs, uri.host)
sem_conv.set_http_peer_port_client(attrs, uri.port)
end

attributes.merge!(OpenTelemetry::Common::HTTP::ClientContext.attributes)

tracer.in_span(span_name, attributes: attributes, kind: :client) do |span|
OpenTelemetry.propagation.inject(req.headers)
Expand All @@ -42,8 +45,11 @@ def annotate_span_with_response!(span, response)
return unless response&.status

status_code = response.status.to_i
span.set_attribute('http.status_code', status_code)
span.status = OpenTelemetry::Trace::Status.error unless (100..399).cover?(status_code.to_i)
sem_conv_status_code = {}
sem_conv.set_http_status_code(sem_conv_status_code, status_code)
span.add_attributes(sem_conv_status_code)

span.status = OpenTelemetry::Trace::Status.error unless (100..399).cover?(status_code)
kaylareopelle marked this conversation as resolved.
Show resolved Hide resolved
end

def create_request_span_name(request_method, request_path)
Expand All @@ -60,6 +66,10 @@ def create_request_span_name(request_method, request_path)
def tracer
HTTP::Instrumentation.instance.tracer
end

def sem_conv
HTTP::Instrumentation.instance.sem_conv
end
end
end
end
Expand Down
13 changes: 9 additions & 4 deletions instrumentation/http/lib/opentelemetry/instrumentation/http/patches/connection.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,11 @@ module Patches
# Module to prepend to HTTP::Connection for instrumentation
module Connection
def initialize(req, options)
attributes = {
'net.peer.name' => req.uri.host,
'net.peer.port' => req.uri.port
}.merge!(OpenTelemetry::Common::HTTP::ClientContext.attributes)
attributes = {}

sem_conv.set_http_net_peer_name_client(attributes, req.uri.host)
sem_conv.set_http_peer_port_client(attributes, req.uri.port)
attributes.merge!(OpenTelemetry::Common::HTTP::ClientContext.attributes)

tracer.in_span('HTTP CONNECT', attributes: attributes) do
super
Expand All @@ -26,6 +27,10 @@ def initialize(req, options)
def tracer
HTTP::Instrumentation.instance.tracer
end

def sem_conv
HTTP::Instrumentation.instance.sem_conv
end
end
end
end
Expand Down
1 change: 1 addition & 0 deletions instrumentation/http/opentelemetry-instrumentation-http.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ Gem::Specification.new do |spec|

spec.add_dependency 'opentelemetry-api', '~> 1.0'
spec.add_dependency 'opentelemetry-instrumentation-base', '~> 0.22.1'
spec.add_dependency 'opentelemetry-semantic_conventions', '>= 1.8.0'

spec.add_development_dependency 'appraisal', '~> 2.5'
spec.add_development_dependency 'bundler', '~> 2.4'
Expand Down
46 changes: 45 additions & 1 deletion instrumentation/http/test/instrumentation/http/patches/client_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
end
let(:span_name_formatter) { nil }

before do
def reset
exporter.reset
@orig_propagation = OpenTelemetry.propagation
propagator = OpenTelemetry::Trace::Propagation::TraceContext.text_map_propagator
Expand All @@ -33,6 +33,10 @@
stub_request(:get, 'https://example.com/timeout').to_timeout
end

before do
reset
end

after do
# Force re-install of instrumentation
instrumentation.instance_variable_set(:@installed, false)
Expand Down Expand Up @@ -182,5 +186,45 @@
)
end
end

describe 'Semantic conventions http stability' do
Copy link
Contributor

Choose a reason for hiding this comment

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

I get that we're blocked to run the tests on the CI right now without a lot of temporary code since this relies on work that is not yet part of the semantic conventions gem.

Do you have anything that demonstrates the tests passing that we could use for now? Maybe a minitest screenshot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mini screenshoot ◡̈ :
Screenshot 2024-09-16 at 4 22 03 PM

it 'reports stable http attributes when OTEL_SEMCONV_STABILITY_OPT_IN = `http`' do
OpenTelemetry::TestHelpers.with_env('OTEL_SEMCONV_STABILITY_OPT_IN' => 'http') do
reset
HTTP.get('http://example.com/success')

_(exporter.finished_spans.size).must_equal(1)
_(span.name).must_equal 'HTTP GET'
_(span.attributes['http.request.method']).must_equal 'GET'
_(span.attributes['url.scheme']).must_equal 'http'
_(span.attributes['http.response.status_code']).must_equal 200
_(span.attributes['url.path']).must_equal '/success'
_(span.attributes['server.address']).must_equal 'example.com'
_(span.attributes['server.port']).must_equal 80
end
end

it 'reports stable http attributes and old http attributes when OTEL_SEMCONV_STABILITY_OPT_IN = `http/dup`' do
OpenTelemetry::TestHelpers.with_env('OTEL_SEMCONV_STABILITY_OPT_IN' => 'http/dup') do
reset
HTTP.get('http://example.com/success')

_(exporter.finished_spans.size).must_equal(1)
_(span.name).must_equal 'HTTP GET'
_(span.attributes['http.method']).must_equal 'GET'
_(span.attributes['http.request.method']).must_equal 'GET'
_(span.attributes['http.scheme']).must_equal 'http'
_(span.attributes['url.scheme']).must_equal 'http'
_(span.attributes['http.status_code']).must_equal 200
_(span.attributes['http.response.status_code']).must_equal 200
_(span.attributes['http.target']).must_equal '/success'
_(span.attributes['url.path']).must_equal '/success'
_(span.attributes['net.peer.name']).must_equal 'example.com'
_(span.attributes['server.address']).must_equal 'example.com'
_(span.attributes['net.peer.port']).must_equal 80
_(span.attributes['server.port']).must_equal 80
end
end
end
end
end
Loading