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

fix: Omit nil net.peer.name attributes #693

Merged
merged 1 commit into from
Oct 16, 2023
Merged

fix: Omit nil net.peer.name attributes #693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions ...tation/faraday/lib/opentelemetry/instrumentation/faraday/middlewares/tracer_middleware.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@ def call(env)
def span_creation_attributes(http_method:, url:)
instrumentation_attrs = {
'http.method' => http_method,
'http.url' => OpenTelemetry::Common::Utilities.cleanse_url(url.to_s),
'net.peer.name' => url.host
'http.url' => OpenTelemetry::Common::Utilities.cleanse_url(url.to_s)
}
instrumentation_attrs['net.peer.name'] = url.host if url.host
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than check for individual attributes and conditionally set them, what do you think about using instrumentation_attrs.compact! before returning the hash?

That will remove any entries with nil values in the hash:

irb(main):003> h = { "a" => 1, "b" => nil  }
=> {"a"=>1, "b"=>nil}
irb(main):004> h.compact!
=> {"a"=>1}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that it is more elegant to use compact! but when I ran benchmark, it performed slower than using if:

def use_compact(b)
  h = { "a" => 1, "b" => b }
  h.compact!
end

def use_if(b)
  h = { "a" => 1 }
  h["b"] = b if b
end

Benchmark.ips do |x|
  x.report("compact! with nil") { use_compact(nil) }
  x.report("if with nil") { use_if(nil) }
  x.report("compact! with value") { use_compact(3) }
  x.report("if with value") { use_if(3) }

  x.compare!
end
Comparison:
         if with nil: 12207916.0 i/s
       if with value:  8454274.8 i/s - 1.44x  slower
   compact! with nil:  6958723.8 i/s - 1.75x  slower
 compact! with value:  6357076.8 i/s - 1.92x  slower

I don't mind switching to compact! but I feel current implementation is better... What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for doing that benchmark!

In that case I think we are good to go. Thank you again for this contribution.

config = Faraday::Instrumentation.instance.config
instrumentation_attrs['peer.service'] = config[:peer_service] if config[:peer_service]
instrumentation_attrs.merge!(
Expand Down
161 changes: 93 additions & 68 deletions .../faraday/test/opentelemetry/instrumentation/faraday/middlewares/tracer_middleware_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,89 +42,114 @@
instrumentation.install
end

it 'has http 200 attributes' do
response = client.get('/success')

_(span.name).must_equal 'HTTP GET'
_(span.attributes['http.method']).must_equal 'GET'
_(span.attributes['http.status_code']).must_equal 200
_(span.attributes['http.url']).must_equal 'http://example.com/success'
_(span.attributes['net.peer.name']).must_equal 'example.com'
_(response.env.request_headers['Traceparent']).must_equal(
"00-#{span.hex_trace_id}-#{span.hex_span_id}-01"
)
end
describe 'given a client with a base url' do
it 'has http 200 attributes' do
response = client.get('/success')

_(span.name).must_equal 'HTTP GET'
_(span.attributes['http.method']).must_equal 'GET'
_(span.attributes['http.status_code']).must_equal 200
_(span.attributes['http.url']).must_equal 'http://example.com/success'
_(span.attributes['net.peer.name']).must_equal 'example.com'
_(response.env.request_headers['Traceparent']).must_equal(
"00-#{span.hex_trace_id}-#{span.hex_span_id}-01"
)
end

it 'has http.status_code 404' do
response = client.get('/not_found')

_(span.name).must_equal 'HTTP GET'
_(span.attributes['http.method']).must_equal 'GET'
_(span.attributes['http.status_code']).must_equal 404
_(span.attributes['http.url']).must_equal 'http://example.com/not_found'
_(span.attributes['net.peer.name']).must_equal 'example.com'
_(response.env.request_headers['Traceparent']).must_equal(
"00-#{span.hex_trace_id}-#{span.hex_span_id}-01"
)
end
it 'has http.status_code 404' do
response = client.get('/not_found')

_(span.name).must_equal 'HTTP GET'
_(span.attributes['http.method']).must_equal 'GET'
_(span.attributes['http.status_code']).must_equal 404
_(span.attributes['http.url']).must_equal 'http://example.com/not_found'
_(span.attributes['net.peer.name']).must_equal 'example.com'
_(response.env.request_headers['Traceparent']).must_equal(
"00-#{span.hex_trace_id}-#{span.hex_span_id}-01"
)
end

it 'has http.status_code 500' do
response = client.get('/failure')

_(span.name).must_equal 'HTTP GET'
_(span.attributes['http.method']).must_equal 'GET'
_(span.attributes['http.status_code']).must_equal 500
_(span.attributes['http.url']).must_equal 'http://example.com/failure'
_(span.attributes['net.peer.name']).must_equal 'example.com'
_(response.env.request_headers['Traceparent']).must_equal(
"00-#{span.hex_trace_id}-#{span.hex_span_id}-01"
)
end
it 'has http.status_code 500' do
response = client.get('/failure')

_(span.name).must_equal 'HTTP GET'
_(span.attributes['http.method']).must_equal 'GET'
_(span.attributes['http.status_code']).must_equal 500
_(span.attributes['http.url']).must_equal 'http://example.com/failure'
_(span.attributes['net.peer.name']).must_equal 'example.com'
_(response.env.request_headers['Traceparent']).must_equal(
"00-#{span.hex_trace_id}-#{span.hex_span_id}-01"
)
end

it 'merges http client attributes' do
client_context_attrs = {
'test.attribute' => 'test.value', 'http.method' => 'OVERRIDE'
}
response = OpenTelemetry::Common::HTTP::ClientContext.with_attributes(client_context_attrs) do
client.get('/success')
it 'merges http client attributes' do
client_context_attrs = {
'test.attribute' => 'test.value', 'http.method' => 'OVERRIDE'
}
response = OpenTelemetry::Common::HTTP::ClientContext.with_attributes(client_context_attrs) do
client.get('/success')
end

_(span.name).must_equal 'HTTP GET'
_(span.attributes['http.method']).must_equal 'OVERRIDE'
_(span.attributes['http.status_code']).must_equal 200
_(span.attributes['http.url']).must_equal 'http://example.com/success'
_(span.attributes['net.peer.name']).must_equal 'example.com'
_(span.attributes['test.attribute']).must_equal 'test.value'
_(response.env.request_headers['Traceparent']).must_equal(
"00-#{span.hex_trace_id}-#{span.hex_span_id}-01"
)
end

_(span.name).must_equal 'HTTP GET'
_(span.attributes['http.method']).must_equal 'OVERRIDE'
_(span.attributes['http.status_code']).must_equal 200
_(span.attributes['http.url']).must_equal 'http://example.com/success'
_(span.attributes['net.peer.name']).must_equal 'example.com'
_(span.attributes['test.attribute']).must_equal 'test.value'
_(response.env.request_headers['Traceparent']).must_equal(
"00-#{span.hex_trace_id}-#{span.hex_span_id}-01"
)
end
it 'accepts peer service name from config' do
instrumentation.instance_variable_set(:@installed, false)
instrumentation.install(peer_service: 'example:faraday')

it 'accepts peer service name from config' do
instrumentation.instance_variable_set(:@installed, false)
instrumentation.install(peer_service: 'example:faraday')
client.get('/success')

client.get('/success')
_(span.attributes['peer.service']).must_equal 'example:faraday'
end

_(span.attributes['peer.service']).must_equal 'example:faraday'
end
it 'prioritizes context attributes over config for peer service name' do
instrumentation.instance_variable_set(:@installed, false)
instrumentation.install(peer_service: 'example:faraday')

it 'prioritizes context attributes over config for peer service name' do
instrumentation.instance_variable_set(:@installed, false)
instrumentation.install(peer_service: 'example:faraday')
client_context_attrs = { 'peer.service' => 'example:custom' }
OpenTelemetry::Common::HTTP::ClientContext.with_attributes(client_context_attrs) do
client.get('/success')
end

client_context_attrs = { 'peer.service' => 'example:custom' }
OpenTelemetry::Common::HTTP::ClientContext.with_attributes(client_context_attrs) do
client.get('/success')
_(span.attributes['peer.service']).must_equal 'example:custom'
end

_(span.attributes['peer.service']).must_equal 'example:custom'
it 'does not leak authentication credentials' do
client.run_request(:get, 'http://username:password@example.com/success', nil, {})

_(span.attributes['http.url']).must_equal 'http://example.com/success'
end
end

it 'does not leak authentication credentials' do
client.run_request(:get, 'http://username:password@example.com/success', nil, {})
describe 'given a client without a base url' do
let(:client) do
Faraday.new do |builder|
builder.adapter(:test) do |stub|
stub.get('/success') { |_| [200, {}, 'OK'] }
end
end
end

_(span.attributes['http.url']).must_equal 'http://example.com/success'
it 'omits missing attributes' do
response = client.get('/success')

_(span.name).must_equal 'HTTP GET'
_(span.attributes['http.method']).must_equal 'GET'
_(span.attributes['http.status_code']).must_equal 200
_(span.attributes['http.url']).must_equal 'http:/success'
_(span.attributes).wont_include('net.peer.name')
_(response.env.request_headers['Traceparent']).must_equal(
"00-#{span.hex_trace_id}-#{span.hex_span_id}-01"
)
end
end
end
end
Loading