fix: Omit nil net.peer.name attributes

[yoheyk]

Copied from open-telemetry/opentelemetry-ruby#1280

Faraday uses a base URL to derive the host name which is used by the tracer middleware as the span attribute value for net.peer.name.

Although it seems incorrect, Faraday does not require a base URL when it is initialized and is a commonly used when using Faraday test stubs.

When the base URL is absent, the net.peer.name is set to nil triggering and invalid attribute error report and creates a bit of noise for test cases.

This change omits the net.peer.name attribute when it is nil to suppress error reporting when the host is absent.

Also, we have an app calling itself by not setting a base URL so we are observing this error in runtime.

[arielvalentin]

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}

[yoheyk]

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?

[arielvalentin]

Thanks for doing that benchmark!

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

[arielvalentin]

Thank you for your contribution and including automated tests!

I have one suggestion for you to apply before merging. Let me know what you think.