[Excon] Fix error with upper-case HTTP methods

[hosamaly]

When Excon is called with an HTTP method in upper-case, the instrumentation fails to capture the 'http.method' and sets it to nil. This causes the response instrumentation to log the following error:

OpenTelemetry error: invalid span attribute value type NilClass for key 'http.method' on span 'HTTP '

This PR fixes the issue and captures the 'http.method' regardless of its casing.

[ericmustin]

👍

Thanks for the contribution. Indeed, reviewing the docs. Excon appears to accept either a string or a symbol as it's :method option input, so this is a more robust approach imo, I suppose there's some very very minor performance tradeoff from calling .to_s.upcase instead of just looking up a Hash value, but I think this is probably negligible and I doubt even measurable in a production system.

Minor programming note that the excon instrumentation has been pretty unloved so if you notice any other issues, feel free to push up a PR.

Cheers!

[arielvalentin]

👋🏼 This was indeed converted into a constant hash to reduce string allocation:

#175 (comment)

Does duplicating entires in the hash to use both symbols and strings solve this issue?

[ericmustin]

^ ditto what @arielvalentin said, missed that in initial review sorry about that

[hosamaly]

We need to support both symbols and strings. :get is as valid as 'get' and 'GET'. Do we want to add all of these combinations to the hash?

I would imagine that using an ActiveSupport::HashWithIndifferentAccess would still call to_s when symbols are used for lookup, so it would still have a memory impact.

What do you suggest?

[robertlaurin]

We need to support both symbols and strings. :get is as valid as 'get' and 'GET'. Do we want to add all of these combinations to the hash?

There's two options here I think. We can setup an adaptive hash like what was done in the net/http instrumentation here.

Or we could simply .to_s.upcase which we are doing here.

I think it's better that we use the adaptive hash as in the worst case (an app using :get, 'get', "GET" interchangeable for all methods), the hash will only grow to a fixed size.

[robertlaurin]

I would imagine that using an ActiveSupport::HashWithIndifferentAccess would still call to_s when symbols are used for lookup, so it would still have a memory impact.

We should in general, avoid pulling in ActiveSupport (or any additional gem) as a dependency of any instrumentation gem in this repo.

[hosamaly]

If we want to avoid all memory allocation, we'll want an adaptive hash for the HTTP method and another for the span name...

[arielvalentin]

I think it's better that we use the adaptive hash as in the worst case (an app using :get, 'get', "GET" interchangeable for all methods), the hash will only grow to a fixed size.

+1

[hosamaly]

I added adaptive hashes for the HTTP method and the span name.

[mwear]

LGTM

[hosamaly]

Hi, @arielvalentin, @mwear and @plantfansam. This PR has been approved a week ago. Is there anything missing before merging it?

[arielvalentin]

cc: @open-telemetry/ruby-maintainers