feat(engine-v1): Add operation metrics #1714
Conversation
| let extensions = self.create_extensions(session_data.clone()); | ||
| let gql_span = GqlRequestSpan::new().into_span(); | ||
| let normalized_query = operation_normalizer::normalize(request.query(), request.operation_name()).ok(); |
There was a problem hiding this comment.
Not much we can do to avoid it I supppose but wonder what impact adding this is going to have on the wasm size...
There was a problem hiding this comment.
we do it already in the worker anyway already, so won't change anything
There was a problem hiding this comment.
We do it in the gateway currently, but this is going to run in the executor, no? Hence wondering
There was a problem hiding this comment.
hum good point, you're right
| let normalized_query = operation_normalizer::normalize(request.query(), request.operation_name()).ok(); | ||
| let (response, headers) = self.execute_with_auth(ctx, request, auth).await?; | ||
| if let Some((operation, normalized_query)) = response.graphql_operation.clone().zip(normalized_query) { | ||
| self.operation_metrics.record( | ||
| grafbase_tracing::metrics::GraphqlOperationMetricsAttributes { | ||
| ty: match operation.r#type { | ||
| common_types::OperationType::Query { .. } => "query", | ||
| common_types::OperationType::Mutation => "mutation", | ||
| common_types::OperationType::Subscription => "subscription", | ||
| }, | ||
| name: operation.name, | ||
| normalized_query_hash: blake3::hash(normalized_query.as_bytes()).into(), | ||
| normalized_query, | ||
| has_errors: !response.errors.is_empty(), | ||
| cache_status: headers | ||
| .get(X_GRAFBASE_CACHE) | ||
| .and_then(|v| v.to_str().ok().map(|s| s.to_string())), | ||
| }, | ||
| start.elapsed(), |
There was a problem hiding this comment.
I assume I'm missing something: why are we doing this here and in the engine?
There was a problem hiding this comment.
because gateway-core deals with the caching so need to be here, but the stream interface doesn't let me retrieve the necessary information. So need to do in the engine for stream
There was a problem hiding this comment.
The alternative would be imposing constraints on Executor::StreamingResponse to be able to retrieve headers and send all the metadata through it
There was a problem hiding this comment.
So need to do in the engine for stream
Ah, that'll be what I'm missing - didn't notice it was stream only
Finistere
force-pushed
the
gb-6698-add-engine-v1-metrics
branch
from
7d50ab2
to
e10c96f
Compare
Description
Please include a summary of the change and which issue is fixed.
Please also include relevant motivation and context.
Type of change