Add excon instrumentation adapter
#175
Conversation
grantbdev
requested review from
bai,
dazuma,
elskwid,
fbogsany,
luvtechno and
mwear
as code owners
mwear
left a comment
mwear
left a comment
There was a problem hiding this comment.
This is in good shape. I have the one comment about saving a string allocation by using a map, and the larger question about quantization.
adapters/excon/lib/opentelemetry/adapters/excon/middlewares/tracer_middleware.rb
Outdated
Show resolved
Hide resolved
adapters/excon/lib/opentelemetry/adapters/excon/middlewares/tracer_middleware.rb
Outdated
Show resolved
Hide resolved
mwear
left a comment
mwear
left a comment
There was a problem hiding this comment.
I have the same comments here as on the Rest::Client instrumentation regarding name and the middleware configuration. We should also set span status. According to the spec, it's required for requests resulting in an error.
|
@mwear PR feedback addressed. These spec statements seem conflicting:
|
|
The api-tracing spec has this to say:
My interpretation is that a canonical status code should be set, but the optional descriptive message shouldn't be if it can be inferred from the http status code (or message). |
mwear
left a comment
mwear
left a comment
There was a problem hiding this comment.
This looks good to me. Thanks for working through my suggestions @grantbdev!
|
PR updated to use |
mwear
left a comment
mwear
left a comment
There was a problem hiding this comment.
Like rest-client, this will need a rebase and the suggested change since the context prop PR merged.
adapters/excon/lib/opentelemetry/adapters/excon/middlewares/tracer_middleware.rb
Outdated
Show resolved
Hide resolved
adapters/excon/lib/opentelemetry/adapters/excon/middlewares/tracer_middleware.rb
Outdated
Show resolved
Hide resolved
|
@mwear PR updated. Thanks! |
|
Like #172 (comment), we should test propagation here as well. This is the corresponding datadog test: https://github.com/DataDog/dd-trace-rb/blob/master/spec/ddtrace/contrib/excon/instrumentation_spec.rb#L166-L188. |
|
👀 ing good here too @grantbdev. Let me know if you want to take a look here as well @fbogsany. |
|
Same as #172, this could a use a rebase and I'll merge it. |
* Add `rest-client` instrumentation adapter (#172) Add `rest-client` instrumentation adapter based on the adapter for Faraday and `ddtrace` code for `rest-client`. The `rest-client` gem uses `restclient` internally for file and folder naming, so I used that style for most of the internal adapter code. `rest-client` is used for names referring to the gem name itself. Related to #67 * Update `faraday` adapter Update `faraday` adapter to match changes in the `rest-client` adapter. In the example code, the install call is replaced to work with the structure changes from #175. Support for disabling span reporting via a custom override is no longer provided since there is not a need for it at this time. This also removes the need to support injection of custom middleware. In the tracer middleware, the HTTP method attribute value is set to an uppercase string to be compliant with the specification. The name of the span is set to "HTTP #{verb}" to reduce the cardinality of the span names as described in the spec. The span status is now set when the response is received. The span propagation is now done directly. Propagated headers are now tested.
|
I used to have a button where I could merge in master, but it's no longer here. I guess we'll need another rebase. |
Add `excon` instrumentation adapter based on the adapter for `rest-client` and `ddtrace` code for `excon`. Related to #67
|
@mwear PR rebased. |
2 participants
Overview
Add
exconinstrumentation adapter similar to the existing adapter for Faraday.Related