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
Enables gRPC Health Checking Protocol on Open Telemetry Collector #8955
base: main
Are you sure you want to change the base?
Conversation
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.
+1
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Any feedback here, folks? |
I triggered them for you. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8955 +/- ##
==========================================
- Coverage 91.63% 91.56% -0.08%
==========================================
Files 356 360 +4
Lines 16849 16695 -154
==========================================
- Hits 15439 15286 -153
- Misses 1068 1073 +5
+ Partials 342 336 -6 ☔ View full report in Codecov by Sentry. |
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.
Sounds like a good idea to me. I think a changelog is appropriate.
Hi folks, I have added the changelog entry (file). |
Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
Thank you for the suggestions, @TylerHelmuth. |
@pantuza I don't have the right permissions in this repo. @open-telemetry/collector-approvers please take a look. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Hi, @ksandrmatveyev. Thank you for the reply. I see that by now this PR is outdated from Master/Main branch. I would be really glad If someone can trigger the CI so we can check if everything is ok. |
@pantuza I'm adding this PR to the collector's SIG agenda on Monday. |
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 think this makes sense.
One question: is there any reason a user wouldn't want to have this enabled? It doesn't look like it is configurable.
In general, it's good practice to have admin-related endpoints at a different port than the workload, but for this specific one, I think it's ok and desirable to have it the way it's implemented in this PR. |
Seems it fails on test phase https://github.com/open-telemetry/opentelemetry-collector/actions/runs/8838230921/job/24299483330?pr=8955 FYI @pantuza |
Hi, @ksandrmatveyev . Thank you for the reply. Looking at the error, it is related to contrib project. That Since we are enabling it directly on the Collector, no extension needs to enable it. It will become a builtin feature of the As a follow up for this PR, I can send a Pull Request to contrib project disabling this on |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Description: Adding a feature - Enabling gRPC Health Checking Protocol on Open Telemetry Collector.
Link to tracking Issue: Not Applicable
Testing:
Before this PR, if you use any gRPC tool [1,2,3] to probe otelcol gRPC port (4317) you would get as response:
$> grpc-health-probe -addr=localhost:4317 error: this server does not implement the grpc health protocol (grpc.health.v1.Health): unknown service grpc.health.v1.Health
Now, the gRPC server responds properly to gRPC Health Protocol:
$> grpc-health-probe -addr=localhost:4317 status: SERVING
Documentation:
I will be glad to add documentation somewhere in the project.
By now, I am considering these changes simply complies with the gRPC industry standard gRPC Health Checking Protocol.
Thus, I think it worths an entry on the Changelog but not necessarily a documentation entry.
Why these changes matter?
It is quite common for gRPC users to probe their gRPC endpoints using the tools described above.
It helps checking if your remote server is up and running before even knowing details of its remote routes/methods.
Therefore, this PR improves developer experience while using or getting started with Open Telemetry Collector.
Here follow some of arguments for this PR:
google.golang.org/grpc
library which was already imported in the Collector code.Please let me know if there is anything missing here or anything I can improve in this contribution.