-
Notifications
You must be signed in to change notification settings - Fork 179
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.
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
feat(excon)! Add a connect span to excon and add more span attributes to the tracer middleware #712
feat(excon)! Add a connect span to excon and add more span attributes to the tracer middleware #712
Conversation
|
instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb
Outdated
Show resolved
Hide resolved
instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb
Outdated
Show resolved
Hide resolved
instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb
Show resolved
Hide resolved
instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb
Outdated
Show resolved
Hide resolved
instrumentation/excon/lib/opentelemetry/instrumentation/excon/patches/socket.rb
Show resolved
Hide resolved
instrumentation/excon/test/opentelemetry/instrumentation/excon/instrumentation_test.rb
Show resolved
Hide resolved
instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb
Outdated
Show resolved
Hide resolved
instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb
Outdated
Show resolved
Hide resolved
instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb
Outdated
Show resolved
Hide resolved
instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb
Outdated
Show resolved
Hide resolved
Hi @misalcedo, the excon suite is raising some rubocop offenses:
|
instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb
Outdated
Show resolved
Hide resolved
@misalcedo check out this PR which has identified another use case where |
@kaylareopelle thanks for the tip, I addressed the rubocop lints. |
…exceptions as an event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this contribution 🎉
I have a few nits in there but otherwise LGTM!
instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb
Outdated
Show resolved
Hide resolved
instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb
Outdated
Show resolved
Hide resolved
…middlewares/tracer_middleware.rb Co-authored-by: Ariel Valentin <arielvalentin@users.noreply.github.com>
@misalcedo we added the rubcop-performance linters on main and now there is a conflict sorry 😿 |
@arielvalentin I addressed the merge conflict. Seems like I just needed to change |
Added a span for TLS and TCP socket connections that matches the one in net_http. Also, I switched to using semantic convention constants over hard-coded strings where applicable. Lastly, I added a few more attributes to the request span.
The untraced_hosts option was added to mirror net_http and allow for selective exclusion of instrumentation.
The attributes before were:
http.host
,http.method
,http.scheme
,http.target
andpeer.service
. I added peer name and port as those were also included innet_http
.Testing
The tests are based off the ones found in the
net_http
instrumentation for the connect span. In writing the tests I found 2 things worth calling out: