Skip to content
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: parts of the gRPC istrumentation #1303

Closed
wants to merge 15 commits into from

Conversation

michal-kazmierczak
Copy link
Contributor

@michal-kazmierczak michal-kazmierczak commented Dec 13, 2024

This PR includes parts of the gRPC instrumentation adaptation outlined in: #1261

This PR does not make the gem ready to release.

The primary change is that the instrumentation leverages the use of interceptors (a concept parallel to middlewares).

Also, there is a patch that instruments every stub (client) created after the OpenTelemetry installation. This aligns the instrumentation behavior with other libraries, so end-users do not need to manually add interceptors.

I decided not to include relatively easy changes that would make the diff quite more difficult to follow (i.e. migration to Rubocop). However, this PR also includes the migration from RSpec to minitest and other minor alignments (like installation of Appraisals or unified Rakefile).

It would be great if this PR could be reviewed and merged, allowing me to proceed with the remaining adaptations.

Copy link
Contributor

@kaylareopelle kaylareopelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @michal-kazmierczak! This looks great! I have a few comments on the files.

Also, I noticed is this PR is from your main branch. We recommend contributors put their work on a branch other than main because the repo's strategy is to squash when we merge, which can cause problems when you update your fork after your main branch is merged.

instrumentation/grpc/Appraisals Show resolved Hide resolved
instrumentation/grpc/Gemfile Show resolved Hide resolved
instrumentation/grpc/README.md Outdated Show resolved Hide resolved
instrumentation/grpc/test/test_helper.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@kaylareopelle kaylareopelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, incremental progress. In my opinion, once the branch is updated, this is ready to merge, @michal-kazmierczak.

@michal-kazmierczak
Copy link
Contributor Author

This is great, incremental progress. In my opinion, once the branch is updated, this is ready to merge, @michal-kazmierczak.

Thank you. I've rebased it now.

@kaylareopelle
Copy link
Contributor

Thanks for the updates, @michal-kazmierczak! I wanted to follow up on this comment:

Also, I noticed is this PR is from your main branch. We recommend contributors put their work on a branch other than main because the repo's strategy is to squash when we merge, which can cause problems when you update your fork after your main branch is merged.

Here's the section of our contributing guide that goes over this concept.

Would you copy your work onto a different branch within your fork and reopen the PR from that branch?

@michal-kazmierczak
Copy link
Contributor Author

@kaylareopelle thanks for pointing that out. Initially, I assumed that since the issue was only potentially problematic on my end, I could manage and leave it on the main branch. However, to avoid any confusion, I’ve now created a new PR (#1330), with the only difference being that it’s based on a feature branch from my fork. I'm closing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants