-
Notifications
You must be signed in to change notification settings - Fork 271
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
Migrate monotonic counter metrics to u64_counter!
#6350
base: dev
Are you sure you want to change the base?
Changes from all commits
304919a
30fc35e
148d9c4
be452c9
7f744e9
eb3a8f7
a93408d
f3bc20b
9130dbc
551170f
b5497c6
69e71b1
4858d24
fce826c
3c43714
fd39d9b
0d6e222
01ebdef
aca6024
cdb7940
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -556,8 +556,10 @@ impl Plugin for AuthorizationPlugin { | |
Ok(ControlFlow::Continue(request)) | ||
} else { | ||
// This is a metric and will not appear in the logs | ||
tracing::info!( | ||
monotonic_counter.apollo_require_authentication_failure_count = 1u64, | ||
u64_counter!( | ||
"apollo_require_authentication_failure_count", | ||
"Number of unauthenticated requests (deprecated)", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I marked this as deprecated because the naming doesn't seem to follow convention, but I don't know if we have a proper alternative for this? We have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that's the right thing to do. cc @BrynCooke you probably have more context |
||
1 | ||
); | ||
tracing::error!("rejecting unauthenticated request"); | ||
let response = supergraph::Response::error_builder() | ||
|
@@ -588,11 +590,13 @@ impl Plugin for AuthorizationPlugin { | |
let needs_requires_scopes = request.context.contains_key(REQUIRED_SCOPES_KEY); | ||
|
||
if needs_authenticated || needs_requires_scopes { | ||
tracing::info!( | ||
monotonic_counter.apollo.router.operations.authorization = 1u64, | ||
u64_counter!( | ||
"apollo.router.operations.authorization", | ||
"Number of subgraph requests requiring authorization", | ||
1, | ||
authorization.filtered = filtered, | ||
authorization.needs_authenticated = needs_authenticated, | ||
authorization.needs_requires_scopes = needs_requires_scopes, | ||
authorization.needs_requires_scopes = needs_requires_scopes | ||
); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,7 +49,11 @@ where | |
match Pin::new(&mut this.sleep).poll(cx) { | ||
Poll::Pending => Poll::Pending, | ||
Poll::Ready(_) => { | ||
tracing::info!(monotonic_counter.apollo_router_timeout = 1u64,); | ||
u64_counter!( | ||
"apollo_router_timeout", | ||
"Number of timed out client requests", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can mark it as deprecated There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The alternative is to use a custom instrument, right? So that I can document it in the deprecated metrics section There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. telemetry:
instrumentation:
instruments:
router:
http.server.request.duration:
# Adding subgraph name, response status code from the router and the operation name
attributes:
http.response.status_code: true # If status code == 504 then it's a timeout at the router http request level
graphql.operation.name:
operation_name: string
subgraph:
# Adding subgraph name, response status code from the subgraph and original operation name from the supergraph
http.client.request.duration:
attributes:
subgraph.name: true
http.response.status_code: # If status code == 504 then it's a timeout at the subgraph http request level
subgraph_response_status: code |
||
1 | ||
); | ||
Poll::Ready(Err(Elapsed::new().into())) | ||
} | ||
} | ||
|
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'm wondering if we should not add
authentication.jwt.failed = false
in the future (for 2.0) to be consistent with what sigv4 is doing for example