Skip to content
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

Review apollo_router_http_requests_total metric usage #4703

Closed
Geal opened this issue Feb 22, 2024 · 5 comments
Closed

Review apollo_router_http_requests_total metric usage #4703

Geal opened this issue Feb 22, 2024 · 5 comments
Labels
potentially-breaking Requires an incompatible change

Comments

@Geal
Copy link
Contributor

Geal commented Feb 22, 2024

The behaviour of this metric is confusing right now. Looking at the telemetry plugin, it is supposed to be recorded on every response:

// http_requests_total - the total number of HTTP requests received
u64_counter!(
"apollo_router_http_requests_total",
"Total number of HTTP requests made.",
1,
metric_attrs
);

but is set in multiple places:

u64_counter!(
"apollo_router_http_requests_total",
"Total number of HTTP requests made.",
1,
metric_attrs
);

but it is also directly set in other places like

"apollo_router_http_requests_total",
"Total number of HTTP requests made.",
1,
status = StatusCode::BAD_REQUEST.as_u16() as i64,
error = "Must provide query string"
);

I looked into that because the content-negotiation layer does not set this metric when returning a HTTP 415:

let response: http::Response<hyper::Body> = http::Response::builder()
.status(StatusCode::UNSUPPORTED_MEDIA_TYPE)
.header(CONTENT_TYPE, APPLICATION_JSON.essence_str())
.body(hyper::Body::from(
serde_json::json!({
"errors": [
graphql::Error::builder()
.message(format!(
r#"'content-type' header must be one of: {:?} or {:?}"#,
APPLICATION_JSON.essence_str(),
GRAPHQL_JSON_RESPONSE_HEADER_VALUE,
))
.extension_code("INVALID_CONTENT_TYPE_HEADER")
.build()
]
})
.to_string(),
))
.expect("cannot fail");

The apollo_router_operations_total metric, though, correctly record a 415 status

@BrynCooke
Copy link
Contributor

I think that this metric can largely be replaced once the work for custom instruments that @bnjjj is working on lands.

@BrynCooke BrynCooke added the potentially-breaking Requires an incompatible change label Mar 4, 2024
@abernix
Copy link
Member

abernix commented Jun 10, 2024

Will this be replaced by http.server.request.duration?

@bnjjj
Copy link
Contributor

bnjjj commented Jun 10, 2024

Yes this example of configuration should do the same job

telemetry:
  instrumentation:
    instruments:
      router:
        http.server.request.duration:
          attributes:
            error:
              error: reason
            operation_name:
              response_context: operation_name
            status:
              response_status: code

@bnjjj
Copy link
Contributor

bnjjj commented Jun 10, 2024

This PR will help to add operation_name at the router service without playing with the response_context

@BrynCooke
Copy link
Contributor

Closing and we will look to review all metrics for the next major release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
potentially-breaking Requires an incompatible change
Projects
None yet
Development

No branches or pull requests

4 participants