-
Notifications
You must be signed in to change notification settings - Fork 170
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: add grpc instrumentation #1164
base: main
Are you sure you want to change the base?
feat: add grpc instrumentation #1164
Conversation
See <https://github.com/hibachrach/opentelemetry-instrumentation-grpc/> for previous git history
The committers listed above are authorized under a signed CLA. |
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.
Thank you for opening this, @hibachrach! I'm excited to take a deeper look!
As far as I know, this is the first time we've brought a gem into contrib that was originally created outside the repository. There's a few things about consistency in tooling that I'd like to talk about with the SIG as part of the review. More soon.
Sounds good--happy to align to y'all in any way; no attachment to anything in this PR as it stands. |
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.
Hi @hibachrach! Thanks for your patience! Great to hear you're happy to align with the rest of the repo! 😄
The main differences we see between your gRPC gem and the rest of the instrumentation are:
- Rspec instead of minitest
- Standard instead of Rubocop
- Changelog format
- Unique code of conduct
- RBS
We also want to add this gem to the GitHub actions for this repo to run tests and release the gem.
The SIG had two ideas about how to make the alignment work:
- Merge your PR as-is and the approvers/maintainers will submit subsequent PRs to update the tooling to make it compatible with our CI and more closely mirror the other gems.
- Make the updates as part of this PR, merging it in once everything is aligned. The approvers/maintainers might make branches off yours to simplify the movement, or we could describe the changes to you and let you take care of them.
I know you've mentioned you have limited time to work on this, so let us know how you'd prefer to move forward. We're really grateful for the work you've done to create the gem and for bringing it into contrib. We want to make things as smooth as we can for you.
p.s. - I left a few suggestions related to some Gruf leftovers I found when looking this over!
spec.summary = 'Gruf instrumentation for the OpenTelemetry framework' | ||
spec.description = 'Gruf instrumentation for the OpenTelemetry framework' |
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.
spec.summary = 'Gruf instrumentation for the OpenTelemetry framework' | |
spec.description = 'Gruf instrumentation for the OpenTelemetry framework' | |
spec.summary = 'gRPC instrumentation for the OpenTelemetry framework' | |
spec.description = 'gRPC instrumentation for the OpenTelemetry framework' |
|
||
module OpenTelemetry | ||
module Instrumentation | ||
# Contains the OpenTelemetry instrumentation for the Gruf gem |
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.
# Contains the OpenTelemetry instrumentation for the Gruf gem | |
# Contains the OpenTelemetry instrumentation for the gRPC gem |
|
||
gem "opentelemetry-api" | ||
gem "opentelemetry-common" | ||
gem "opentelemetry-instrumentation-gruf", path: "../../gruf" |
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.
It looks like this example may have been a carryover from the gruf instrumentation! Would you rather update it for the gRPC gem, or remove it and create an issue to add an example later on?
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.
Added a few suggestions to help the markdown-related CI actions pass!
|
||
## Contributing | ||
|
||
Bug reports and pull requests are welcome on GitHub at https://github.com/hibachrach/opentelemetry-instrumentation-grpc. This project is intended to be a safe, welcoming space for collaboration, and contributors are expected to adhere to the [code of conduct](https://github.com/[USERNAME]/opentelemetry-instrumentation-grpc/blob/main/CODE_OF_CONDUCT.md). |
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.
Bug reports and pull requests are welcome on GitHub at https://github.com/hibachrach/opentelemetry-instrumentation-grpc. This project is intended to be a safe, welcoming space for collaboration, and contributors are expected to adhere to the [code of conduct](https://github.com/[USERNAME]/opentelemetry-instrumentation-grpc/blob/main/CODE_OF_CONDUCT.md). | |
Bug reports and pull requests are welcome on GitHub at https://github.com/open-telemetry/opentelemetry-ruby-contrib. This project is intended to be a safe, welcoming space for collaboration, and contributors are expected to adhere to the [code of conduct](https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/main/CODE_OF_CONDUCT.md). |
|
||
## Code of Conduct | ||
|
||
Everyone interacting in the OpenTelemetry::Instrumentation::Grpc project's codebases, issue trackers, chat rooms and mailing lists is expected to follow the [code of conduct](https://github.com/[USERNAME]/opentelemetry-instrumentation-grpc/blob/main/CODE_OF_CONDUCT.md). |
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.
Everyone interacting in the OpenTelemetry::Instrumentation::Grpc project's codebases, issue trackers, chat rooms and mailing lists is expected to follow the [code of conduct](https://github.com/[USERNAME]/opentelemetry-instrumentation-grpc/blob/main/CODE_OF_CONDUCT.md). | |
Everyone interacting in the OpenTelemetry::Instrumentation::Grpc project's codebases, issue trackers, chat rooms and mailing lists is expected to follow the [code of conduct](https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/main/CODE_OF_CONDUCT.md). |
OpenTelemetry::SDK.configure do |c| | ||
c.use 'OpenTelemetry::Instrumentation::Grpc', { | ||
peer_service: "Example", | ||
allowed_metadata_headers: [], | ||
} | ||
end |
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.
OpenTelemetry::SDK.configure do |c| | |
c.use 'OpenTelemetry::Instrumentation::Grpc', { | |
peer_service: "Example", | |
allowed_metadata_headers: [], | |
} | |
end | |
OpenTelemetry::SDK.configure do |c| | |
c.use 'OpenTelemetry::Instrumentation::Grpc', { | |
peer_service: "Example", | |
allowed_metadata_headers: [], | |
} | |
end |
OpenTelemetry::SDK.configure do |c| | ||
c.use_all | ||
end |
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.
OpenTelemetry::SDK.configure do |c| | |
c.use_all | |
end | |
OpenTelemetry::SDK.configure do |c| | |
c.use_all | |
end |
otel = OpenTelemetry::Instrumentation::Grpc.client_interceptor | ||
SomeService::Stub.new(host, credentials, *args, **kwargs, interceptors: [otel]) |
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.
otel = OpenTelemetry::Instrumentation::Grpc.client_interceptor | |
SomeService::Stub.new(host, credentials, *args, **kwargs, interceptors: [otel]) | |
otel = OpenTelemetry::Instrumentation::Grpc.client_interceptor | |
SomeService::Stub.new(host, credentials, *args, **kwargs, interceptors: [otel]) |
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
See https://github.com/hibachrach/opentelemetry-instrumentation-grpc/ for previous git history
Closes #1041