metrics: add a new 'reason' tag #501
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new 'reason' tag to HTTP metrics to provide better observability into why requests failed or succeeded. The change introduces a ProxyResponseReason enum that categorizes different response scenarios and integrates it into the telemetry system.
- Introduces
ProxyResponseReasonenum with categories like Upstream, NotFound, Internal, etc. - Adds reason field to HTTP metrics labels and request logging
- Refactors error handling to use the new external authorization error type and removes transformation failure errors
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/agentgateway/src/telemetry/metrics.rs | Adds reason field to HTTPLabels struct and reorganizes imports |
| crates/agentgateway/src/telemetry/log.rs | Adds reason field to RequestLog struct and populates it in metrics |
| crates/agentgateway/src/proxy/mod.rs | Introduces ProxyResponseReason enum with Display implementation and mapping logic |
| crates/agentgateway/src/proxy/httpproxy.rs | Integrates reason tracking and removes transformation error handling |
| crates/agentgateway/src/http/transformation_cel_tests.rs | Updates test calls to match new transformation API |
| crates/agentgateway/src/http/transformation_cel.rs | Removes error returns from transformation methods |
| crates/agentgateway/src/http/ext_authz.rs | Updates to use new ExternalAuthorizationFailed error type |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| /// The upstream request failed | ||
| UpstreamFailure, |
There was a problem hiding this comment.
The UpstreamFailure variant is defined but never used in the as_reason method. All upstream-related errors map to ProxyResponseReason::Upstream instead. Consider removing this unused variant or updating the mapping logic to use it.
| /// The upstream request failed | |
| UpstreamFailure, |
| if let Some(j) = &policies.transformation { | ||
| j.apply_request(req, exec.deref().as_ref().map_err(cel_err)?) | ||
| .map_err(|_| ProxyError::TransformationFailure)?; | ||
| j.apply_request(req, exec.deref().as_ref().map_err(cel_err)?); |
There was a problem hiding this comment.
The error handling with map_err(cel_err)? is unnecessary since apply_request no longer returns a Result. The call can be simplified to just j.apply_request(req, exec.deref().as_ref().unwrap()).
| j.apply_request(req, exec.deref().as_ref().map_err(cel_err)?); | |
| j.apply_request(req, exec.deref().as_ref().unwrap()); |
| .map_err(|_| ProxyError::ProcessingString("failed to build cel context".to_string()))?; | ||
| j.apply_request(req, &exec) | ||
| .map_err(|_| ProxyError::TransformationFailure)?; | ||
| j.apply_request(req, &exec); |
There was a problem hiding this comment.
Similar to the previous comment, the error handling setup with cel_err closure is defined but no longer needed since apply_request doesn't return errors. The exec variable and error handling can be simplified.
| let cel_err = |_| ProxyError::ProcessingString("failed to build cel context".to_string()); | ||
| if let Some(j) = &self.transformation { | ||
| j.apply_response(resp, log.cel.ctx()) | ||
| .map_err(|_| ProxyError::TransformationFailure)?; | ||
| j.apply_response(resp, exec.deref().as_ref().map_err(cel_err)?); | ||
| } | ||
| if let Some(j) = &self.gateway_transformation { | ||
| j.apply_response(resp, log.cel.ctx()) | ||
| .map_err(|_| ProxyError::TransformationFailure)?; | ||
| j.apply_response(resp, exec.deref().as_ref().map_err(cel_err)?); |
There was a problem hiding this comment.
The cel_err closure is defined but never used since the transformation methods no longer return errors. This dead code should be removed along with the associated error handling.
3e172b5 to
56c93ae
Compare
56c93ae to
282973b
Compare
No description provided.