-
Notifications
You must be signed in to change notification settings - Fork 286
feat(app/inbound): http and grpc status code metrics #4313
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
Conversation
1dc3306 to
0d51e4e
Compare
0d51e4e to
9a3b864
Compare
|
ci is not currently passing. #4330 shows that this is happening on |
|
#4333 is addressing fixes required in |
in #4298, we introduced a new metrics telemetry layer that can measure and report status codes, in a protocol-agnostic fashion. this commit integrates this status code telemtry into the inbound proxy, so that HTTP and gRPC traffic can be observed. a new family of metrics is introduced to the `InboundMetrics` structure, and the inbound http\* router's metrics layer is accordingly updated to thread this metrics family into an extractor, which is in turn provided to the `NewRecordStatusCode` layer. \* as a note for reviewers, the inbound proxy does not model the http and grpc protocols as distinct concepts in the network stack's type system, unlike the outbound proxy. this means that while target types in the outbound proxy like `Http<()>` are specific to HTTP, the inbound proxy's distinction of HTTP/gRPC is determined by obtaining and inspecting the `PermitVariant`. #### 🔗 related some previous pull requests related to this change: * #4298 * #4180 * #4203 * #4127 * #4119 Signed-off-by: katelyn martin <kate@buoyant.io>
9a3b864 to
a731516
Compare
sfleen
left a comment
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 like this, the PR is clean and easy to follow.
One more general comment that's definitely outside the scope of this PR, it feels like we should have some sort of tests that these values are populated correctly in the actual metrics output. We already test the extracted values like in #4298, but something that checks the real metric output seems like it might be valuable. Maybe even at the linkerd2 repo level?
unleashed
left a comment
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.
LGTM
|
i did some reading yesterday to see if i could find a way to do integration tests on these metrics, but didn't see anything that could be easily reached for today. i do strongly agree that we should have better facilities for writing integration tests that can inspect metrics, both (a) to test metrics, and (b) as a mechanism for testing and observing other components. for now, i'm going to merge this and consider that concern out-of-scope. thank you both for your time reviewing this :) |
in #4298, we introduced a new metrics telemetry
layer that can measure and report status codes, in a protocol-agnostic
fashion. this commit integrates this status code telemtry into the
inbound proxy, so that HTTP and gRPC traffic can be observed.
a new family of metrics is introduced to the
InboundMetricsstructure,and the inbound http* router's metrics layer is accordingly updated to
thread this metrics family into an extractor, which is in turn provided
to the
NewRecordStatusCodelayer.* as a note for reviewers, the inbound proxy does not model the http and
grpc protocols as distinct concepts in the network stack's type system,
unlike the outbound proxy. this means that while target types in the
outbound proxy like
Http<()>are specific to HTTP, the inbound proxy'sdistinction of HTTP/gRPC is determined by obtaining and inspecting the
PermitVariant.🔗 related
some previous pull requests related to this change:
NewRecordStatusCodemiddleware #4298Permitted<T>target #4119