fix(opentelemetry source): implement standard gRPC health checking protocol - #26025
Open
stigglor wants to merge 2 commits into
Open
fix(opentelemetry source): implement standard gRPC health checking protocol#26025stigglor wants to merge 2 commits into
stigglor wants to merge 2 commits into
Conversation
…otocol A health check against the OTLP gRPC listener hit an unrouted method and got back a bare HTTP 404 with no grpc-status, so anything that speaks gRPC health checking had nothing to evaluate. In practice that means an ALB target group or a Kubernetes probe has to check an Export method instead and accept a range of statuses wide enough to cover the decoder rejecting an empty body, which is a workaround riding on an error path rather than a health signal. Every such probe also logged a Grpc error at ERROR level against the source. The vector source already registers grpc.health.v1.Health via tonic_health (vectordotdev#24916), and the API replaced its custom Health RPC with the same service (vectordotdev#25139), so this applies the established pattern to the source that missed out. The reporter serves the aggregate (empty) service name, which is what a probe sends by default, and each of the three OTLP services is reported by name so a probe can target a single signal. Those names come from the generated NamedService impls so they cannot drift from the registered routes. Health checks now answer SERVING on the OTLP gRPC port, an unregistered name gets NOT_FOUND rather than a 404, and grpc-health-probe works against port 4317. Status is static SERVING, matching the vector source: the reporter handle is dropped after registration, so the source does not flip to NOT_SERVING while draining. Worth revisiting for both sources, but out of scope here. The HTTP listener is untouched.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2734cab6b9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Registering the health service on a source's gRPC listener put probes through DecompressionAndMetricsLayer, which emits BytesReceived for every successful request, so each check added a few bytes to that source's component_received_bytes_total. A probe carries no events, so an installation probing every few seconds saw the metric drift upward on traffic it never accepted. The layer wraps the whole server and cannot be applied per route, so it now skips byte emission when the request path belongs to grpc.health.v1.Health. Probes still go through decompression, because a client is free to compress one. The gRPC server request and duration metrics continue to count probes, since those describe server activity rather than received data. This applies to the vector source as well, which has registered the health service since vectordotdev#24916.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A gRPC health check against the
opentelemetrysource's gRPC listener hits an unroutedmethod and gets back a bare HTTP 404 with no
grpc-status, so anything speaking the gRPChealth checking protocol has nothing to evaluate. An ALB target group or a Kubernetes
probe has to check an
Exportmethod instead and accept a range of statuses wide enoughto cover the decoder rejecting an empty body, which rides on an error path rather than a
health signal. Every such probe also logs
Grpc error. error=Received 404 Not FoundatERROR level against the source.
The
vectorsource already registersgrpc.health.v1.Healthviatonic_health(#24916)and the API replaced its custom Health RPC with the same service (#25139), so this applies
the established pattern to the source that missed out. The reporter serves the aggregate
(empty) service name, which is what a probe sends unless told otherwise, and each of the
three OTLP services is reported by name so a probe can target a single signal. Those names
come from the generated
NamedServiceimplementations rather than string literals so theycannot drift from the routes actually registered.
Status is static
SERVING, matching thevectorsource: the reporter handle is droppedafter registration, so the source does not flip to
NOT_SERVINGwhile draining. Thatseems worth revisiting for both sources but is out of scope here. The HTTP listener on
4318 is untouched.
Vector configuration
How did you test this PR?
Added
standard_grpc_health_check_works, modelled on thevectorsource test of the samename. It asserts
SERVINGfor the aggregate and the three OTLP service names, andNOT_FOUNDfor an unregistered one.I also ran the config above against a locally built binary and probed it over raw HTTP/2
with curl, since it is the wire behaviour that matters to a load balancer:
Health/Check, empty serviceHTTP/2 200,content-type: application/grpc, trailergrpc-status: 0, body08 01(SERVING)Health/Check, each OTLP service namegrpc-status: 0, SERVINGHealth/Check,not.a.real.Servicegrpc-status: 5,service not registeredTraceService/Export, empty messagegrpc-status: 3,Invalid OTLP data: expected one of {Traces}, unchangeddoes.not.Exist/MethodHTTP/2 404, nogrpc-status, which is what health checks used to getgrpc_health_probeis the practical follow-on for Kubernetes probes; I did not have itinstalled, hence the raw HTTP/2 probes above.
Is this a breaking change?
Does this PR include user facing changes?
no-changeloglabel to this PR.References