Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
### Ensure `http.server.response.body.size` metric is recorded ([PR #8697](https://github.com/apollographql/router/pull/8697))

Previously, the `http.server.response.body.size` metric was not recorded as we attempted to read from the `Content-Length` header of the response before it had been set. We now use the `size_hint` of the body if it is exact.

By [@rohan-b99](https://github.com/rohan-b99) in https://github.com/apollographql/router/pull/8697
Original file line number Diff line number Diff line change
Expand Up @@ -8332,6 +8332,20 @@ expression: "&schema"
],
"type": "object"
},
{
"additionalProperties": false,
"description": "The size hint of the body.\nThis is used as the Content-Length header has not yet been populated\nwhen selectors are evaluated",
"properties": {
"response_size_hint": {
"description": "Extract response size hint in bytes",
"type": "boolean"
}
},
"required": [
"response_size_hint"
],
"type": "object"
},
{
"additionalProperties": false,
"description": "Router overhead duration (time not spent in subgraph requests)",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ telemetry:
router:
http.server.active_requests: false
http.server.request.duration: false
http.server.response.body.size: false
"custom_counter":
description: "count of requests"
type: counter
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ telemetry:
router:
http.server.active_requests: false
http.server.request.duration: false
http.server.response.body.size: false
"custom_counter":
description: "count of requests"
type: counter
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ telemetry:
router:
http.server.active_requests: false
http.server.request.duration: false
http.server.response.body.size: false
"custom_counter":
description: "count of requests"
type: counter
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ telemetry:
router:
http.server.active_requests: false
http.server.request.duration: false
http.server.response.body.size: false
"custom_counter":
description: "count of requests"
type: counter
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ telemetry:
router:
http.server.active_requests: false
http.server.request.duration: false
http.server.response.body.size: false
"custom.histogram":
description: "histogram of requests"
type: histogram
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ telemetry:
router:
http.server.active_requests: false
http.server.request.duration: false
http.server.response.body.size: false
"custom.histogram.duration":
description: "histogram of requests"
type: histogram
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ telemetry:
router:
http.server.active_requests: false
http.server.request.duration: false
http.server.response.body.size: false
"custom.histogram":
description: "histogram of requests"
type: histogram
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ telemetry:
router:
http.server.active_requests: false
http.server.request.duration: false
http.server.response.body.size: false
"custom.histogram":
description: "histogram of requests"
type: histogram
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ telemetry:
router:
http.server.active_requests: false
http.server.request.duration: false
http.server.response.body.size: false
"custom.histogram":
description: "histogram of requests"
type: histogram
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@ telemetry:
router:
http.server.active_requests: true
http.server.request.duration: false
http.server.response.body.size: false
subgraph:
http.client.request.duration: false
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,5 @@ telemetry:
router:
http.server.active_requests: false
http.server.request.duration: false
http.server.request.body.size: true
http.server.request.body.size: true
http.server.response.body.size: false
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,5 @@ telemetry:
router:
http.server.active_requests: false
http.server.request.duration: false
http.server.request.body.size: true
http.server.request.body.size: true
http.server.response.body.size: false
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@ telemetry:
instruments:
router:
http.server.active_requests: false
http.server.request.duration: true
http.server.request.duration: true
http.server.response.body.size: false
Comment on lines +6 to +7
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason these are going from true to false?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default requirement level (Required) enabled them by default, they didn't show up before as content-length was not specified in the test yaml but since we now do it based on the body it would show up if not disabled here. I figured it would be best to disable it for the tests not relating to response body size so it doesn't show up in the snapshots

Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ telemetry:
instruments:
router:
http.server.active_requests: false
http.server.response.body.size: false
http.server.request.duration:
attributes:
http.request.method:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ info:
unit: By
data:
datapoints:
- sum: 35
- sum: 18
count: 1
attributes:
http.request.method: GET
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ events:
body: |
hello
- router_response:
headers:
"content-length": "35"
body: |
hello
status: 200
15 changes: 4 additions & 11 deletions apollo-router/src/plugins/telemetry/config_new/instruments.rs
Original file line number Diff line number Diff line change
Expand Up @@ -401,11 +401,7 @@ impl InstrumentsConfig {
)
),
attributes: Vec::with_capacity(nb_attributes),
selector: Some(Arc::new(RouterSelector::ResponseHeader {
response_header: "content-length".to_string(),
redact: None,
default: None,
})),
selector: Some(Arc::new(RouterSelector::ResponseSizeHint { response_size_hint: true })),
selectors,
updated: false,
_phantom: PhantomData,
Expand Down Expand Up @@ -3463,7 +3459,6 @@ mod tests {
.status_code(StatusCode::BAD_REQUEST)
.header("content-type", "application/json")
.header("x-my-header", "TEST")
.header("content-length", "35")
.data(json!({"errors": [{"message": "nope"}]}))
.build()
.unwrap();
Expand All @@ -3483,7 +3478,7 @@ mod tests {
assert_histogram_sum!("http.server.request.body.size", 35.0);
assert_histogram_sum!(
"http.server.response.body.size",
35.0,
40.0,
"acme.my_attribute" = "TEST"
);

Expand All @@ -3500,7 +3495,6 @@ mod tests {
.context(router_req.context.clone())
.status_code(StatusCode::BAD_REQUEST)
.header("content-type", "application/json")
.header("content-length", "35")
.data(json!({"errors": [{"message": "nope"}]}))
.build()
.unwrap();
Expand All @@ -3520,12 +3514,12 @@ mod tests {
assert_histogram_sum!("http.server.request.body.size", 70.0);
assert_histogram_sum!(
"http.server.response.body.size",
35.0,
40.0,
"acme.my_attribute" = "TEST"
);
assert_histogram_sum!(
"http.server.response.body.size",
35.0,
40.0,
"acme.my_attribute" = "unknown"
);

Expand All @@ -3541,7 +3535,6 @@ mod tests {
.context(router_req.context.clone())
.status_code(StatusCode::OK)
.header("content-type", "application/json")
.header("content-length", "35")
.data(json!({"errors": [{"message": "nope"}]}))
.build()
.unwrap();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use derivative::Derivative;
use http_body::Body;
use schemars::JsonSchema;
use serde::Deserialize;
use serde_json::from_str;
Expand Down Expand Up @@ -159,6 +160,13 @@ pub(crate) enum RouterSelector {
/// The http response status code.
response_status: ResponseStatus,
},
/// The size hint of the body.
/// This is used as the Content-Length header has not yet been populated
/// when selectors are evaluated
ResponseSizeHint {
/// Extract response size hint in bytes
response_size_hint: bool,
},
/// Router overhead duration (time not spent in subgraph requests)
RouterOverhead {
/// Extract router overhead duration in seconds
Expand Down Expand Up @@ -311,6 +319,13 @@ impl Selector for RouterSelector {
.canonical_reason()
.map(|reason| reason.to_string().into()),
},
RouterSelector::ResponseSizeHint { response_size_hint } if *response_size_hint => {
response
.response
.size_hint()
.exact()
.map(|size| opentelemetry::Value::I64(size as i64))
}
RouterSelector::RouterOverhead { router_overhead } if *router_overhead => response
.context
.extensions()
Expand Down Expand Up @@ -459,6 +474,7 @@ impl Selector for RouterSelector {
| RouterSelector::ResponseHeader { .. }
| RouterSelector::ResponseContext { .. }
| RouterSelector::ResponseStatus { .. }
| RouterSelector::ResponseSizeHint { .. }
| RouterSelector::RouterOverhead { .. }
| RouterSelector::ActiveSubgraphRequests { .. }
| RouterSelector::OnGraphQLError { .. }
Expand Down Expand Up @@ -975,6 +991,25 @@ mod test {
);
}

#[test]
fn router_response_size_hint() {
let selector = RouterSelector::ResponseSizeHint {
response_size_hint: true,
};
assert_eq!(
selector
.on_response(
&crate::services::RouterResponse::fake_builder()
.data("value")
.build()
.unwrap()
)
.unwrap(),
// The actual response body is {"data":"value"}
opentelemetry::Value::I64(16)
);
}

#[test]
fn router_response_body() {
let selector = RouterSelector::ResponseBody {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
* `http.server.active_requests` - The number of active requests in flight.
* `http.server.request.body.size` - A histogram of request body sizes for requests handled by the router.
* `http.server.request.duration` - A histogram of request durations for requests handled by the router.
* `http.server.response.body.size` - A histogram of response body sizes for requests handled by the router.

Check warning on line 27 in docs/source/routing/observability/router-telemetry-otel/enabling-telemetry/instruments.mdx

View check run for this annotation

Apollo Librarian / AI Style Review

docs/source/routing/observability/router-telemetry-otel/enabling-telemetry/instruments.mdx#L27

Use hyphens (-) for list bullets instead of asterisks (*). Omit ending punctuation for list items that are sentence fragments. ```suggestion - `http.server.response.body.size` - A histogram of response body sizes for requests handled by the router ```

+ In the [subgraph service](#router-request-lifecycle-services):

Expand Down Expand Up @@ -53,6 +54,7 @@
http.server.active_requests: true # (default false)
http.server.request.body.size: true # (default false)
http.server.request.duration: true # (default false)
http.server.response.body.size: true # (default false)
subgraph:
http.client.request.body.size: true # (default false)
http.client.request.duration: true # (default false)
Expand Down