Feat: aws-sdk instrumentation #1014
Conversation
…nto feat/aws-sdk-instrumentation
…nto feat/aws-sdk-instrumentation
YanivD
requested review from
ahayworth,
arielvalentin,
dazuma,
ericmustin,
fbogsany,
mwear and
robertlaurin
as code owners
| end | ||
| } | ||
|
|
||
| if instrumentation_config[:suppress_internal_instrumentation] |
There was a problem hiding this comment.
✨ I really like this!
@open-telemetry/ruby-maintainers I do realize we don't love using untraced but I think this would be great to include as an option for other instrumentation where we may have redundant client instrumentation (if we are not doing so already)
There was a problem hiding this comment.
@arielvalentin
Agree!
It reduce the amount of redundant spans and making the trace much clearer.
Many nodejs otel instrumentations accept this option, like opentelemetry-instrumentation-aws-sdk.
suppress_internal_instrumentation option should always be false by default to make sure adding new instrumentations won't remove existing spans.
instrumentation/aws_sdk/lib/opentelemetry/instrumentation/aws_sdk/instrumentation.rb
Outdated
Show resolved
Hide resolved
instrumentation/aws_sdk/lib/opentelemetry/instrumentation/aws_sdk/services.rb
Show resolved
Hide resolved
| class Plugin < Seahorse::Client::Plugin | ||
| def add_handlers(handlers, config) | ||
| # run before Seahorse::Client::Plugin::ParamValidator (priority 50) | ||
| handlers.add Handler, step: :validate, priority: 49 |
There was a problem hiding this comment.
Question for AWS users... is there anyway to look up the position/priority of an existing Handler like we can do with Rails middleware?
instrumentation/aws_sdk/lib/opentelemetry/instrumentation/aws_sdk/handler.rb
Outdated
Show resolved
Hide resolved
instrumentation/aws_sdk/lib/opentelemetry/instrumentation/aws_sdk/handler.rb
Outdated
Show resolved
Hide resolved
instrumentation/aws_sdk/lib/opentelemetry/instrumentation/aws_sdk/handler.rb
Show resolved
Hide resolved
instrumentation/aws_sdk/lib/opentelemetry/instrumentation/aws_sdk/handler.rb
Show resolved
Hide resolved
instrumentation/aws_sdk/lib/opentelemetry/instrumentation/aws_sdk/instrumentation.rb
Outdated
Show resolved
Hide resolved
…sdk/instrumentation.rb Co-authored-by: Eric Mustin <mustin.eric@gmail.com>
|
Lgtm pending some of the feedback from @mwear getting addressed/resolved. nice work here ty for the contribution! |
instrumentation/aws_sdk/test/opentelemetry/instrumentation_test.rb
Outdated
Show resolved
Hide resolved
instrumentation/aws_sdk/test/opentelemetry/instrumentation_test.rb
Outdated
Show resolved
Hide resolved
instrumentation/aws_sdk/test/opentelemetry/instrumentation_test.rb
Outdated
Show resolved
Hide resolved
…t.rb Co-authored-by: Ariel Valentin <arielvalentin@users.noreply.github.com>
…t.rb Co-authored-by: Ariel Valentin <arielvalentin@users.noreply.github.com>
…t.rb Co-authored-by: Ariel Valentin <arielvalentin@users.noreply.github.com>
|
Is there anything else missing before merge and release? @open-telemetry/ruby-maintainers |
|
I believe everything has been addressed. Any objections to me merging this @robertlaurin and @fbogsany? |
Related to issue open-telemetry/opentelemetry-ruby-contrib#13
During the development I looked on 2 examples:
This is my first PR in Ruby, so please review this PR carefully 🙏