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: add grpc instrumentation #1164

Merged

Conversation

hibachrach
Copy link
Contributor

Copy link

linux-foundation-easycla bot commented Sep 12, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

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 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.

@hibachrach
Copy link
Contributor Author

Sounds good--happy to align to y'all in any way; no attachment to anything in this PR as it stands.

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.

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:

  1. 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.
  2. 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!

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.

Added a few suggestions to help the markdown-related CI actions pass!

instrumentation/grpc/README.md Outdated Show resolved Hide resolved
instrumentation/grpc/README.md Outdated Show resolved Hide resolved
instrumentation/grpc/README.md Outdated Show resolved Hide resolved
instrumentation/grpc/README.md Outdated Show resolved Hide resolved
instrumentation/grpc/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Nov 2, 2024

👋 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 keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot

@github-actions github-actions bot added the stale Marks an issue/PR stale label Nov 2, 2024
@kaylareopelle kaylareopelle added keep Ensures stale-bot keeps this issue/PR open and removed stale Marks an issue/PR stale labels Nov 5, 2024
@michal-kazmierczak
Copy link
Contributor

Hello @hibachrach and @kaylareopelle

I’d like to express my interest in contributing to this PR. Of course, @hibachrach has the precedence in responding to the PR review.

My potential contribution could involve either:

  • making changes directly in the fork (for which I’d likely need write access), or
  • implementing changes after this PR is merged.

Let me know how I can best assist!

@hibachrach
Copy link
Contributor Author

hibachrach commented Nov 20, 2024

Go for it! My personal life has been quite busy so I haven't had the opportunity to contribute here.

@kaylareopelle
Copy link
Contributor

@michal-kazmierczak, thanks for expressing your interest! And thank you, @hibachrach, for chiming in!

@michal-kazmierczak - to make things easier for you to contribute, I think we should merge this PR, knowing that the gem isn't ready for OpenTelemetry to release just yet. I'll open up an issue tomorrow with more detailed steps on what we need to take care of before we can release a new version under the open-telemetry org.

If any maintainers/approvers disagree with this approach, please let me know!

@arielvalentin
Copy link
Collaborator

I'm ok with that as long as the unfinished gem isn't releasable or a dependency of the all gem.

@kaylareopelle kaylareopelle requested review from a team as code owners November 21, 2024 20:25
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.

We keep hitting Markdown Link Check failures for different links that are actually up and reporting just fine (ex: https://opensource.org/licenses/MIT). It seems like there's a timeout issue with the action itself. Instead of trying to run the failed job again, I'm going to consider this good enough and merge in the PR.

@kaylareopelle kaylareopelle merged commit 2886fae into open-telemetry:main Nov 26, 2024
56 of 57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keep Ensures stale-bot keeps this issue/PR open
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to handle gRPC repo w.r.t. attribution/licensing
4 participants