-
Notifications
You must be signed in to change notification settings - Fork 239
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: aws-sdk instrumentation #1014
Feat: aws-sdk instrumentation #1014
Conversation
…nto feat/aws-sdk-instrumentation
…nto feat/aws-sdk-instrumentation
end | ||
} | ||
|
||
if instrumentation_config[:suppress_internal_instrumentation] |
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.
✨ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
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.
This looks good to me! Thank you very much for this excellent contribution.
I will defer to any other reviewers who have more experience than I using the AWS instrumentation.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question for AWS users... is there anyway to look up the position/priority of an existing Handler like we can do with Rails middleware?
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.
I just have a couple of comments / questions, but otherwise this looks really good. Thanks for the contribution @YanivD!
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
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.
A few small nits, generally lgtm nice work!
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! |
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.
LGTM. Thanks for addressing my feedback!
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 🙏