-
Notifications
You must be signed in to change notification settings - Fork 19
feat(router): allow to customize access log message by plugin system #1379
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| --- | ||
| hive-router-internal: minor | ||
| hive-router: minor | ||
| hive-router-plan-executor: patch | ||
| --- | ||
|
|
||
| # Expose the summary message to the plugin system | ||
|
|
||
| Plugins can now override the request summary log line's message via `hive_router::set_summary_message(message)`, callable from any hook. | ||
|
|
||
| Fixes https://github.com/graphql-hive/router/issues/1378 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -143,13 +143,26 @@ pub fn set_summary_attribute(key: impl Into<String>, value: impl Into<sonic_rs:: | |
| summary::record(|s| s.set_custom(key, value)); | ||
| } | ||
|
|
||
| /// Returns the current request log summary for the current request, if one exists. | ||
| pub fn get_current_summary() -> Option<Arc<summary::RequestSummary>> { | ||
| summary::current_summary() | ||
| } | ||
|
|
||
| /// Lets plugins attach a custom correlation to every log line of the current request (not | ||
| /// just the summary), e.g. a tenant or project id extracted from the URL. Setting the same | ||
| /// key again overwrites the previous value. A no-op outside a request. | ||
| /// just the summary), e.g. a tenant or project id extracted from the URL. | ||
| /// Setting the same key again overwrites the previous value. A no-op outside a request. | ||
| pub fn set_log_correlation(key: impl Into<String>, value: impl std::fmt::Display) { | ||
| request_id::set_correlation(key, value); | ||
| } | ||
|
|
||
| /// Lets plugins override the request summary log line's message. | ||
| /// This can be called only once per request, and only during the request's lifetime. | ||
| /// Calling it more than once, for the same request is a no-op. | ||
| /// Calling it outside of a request is a no-op. | ||
| pub fn set_summary_message(message: impl Into<std::borrow::Cow<'static, str>>) { | ||
| summary::record(|s| s.set_message(message)); | ||
| } | ||
|
|
||
| #[inline] | ||
| fn obtain_header_value<'a>( | ||
| header_map: &'a ntex::http::HeaderMap, | ||
|
|
@@ -178,7 +191,7 @@ async fn graphql_endpoint_handler( | |
| .capture_request(&request); | ||
|
|
||
| let started_at = std::time::Instant::now(); | ||
| let (response_mode, mut response, summary_guard) = async { | ||
| let (mut response, summary_guard) = async { | ||
|
Member
Author
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. we just no longer needs |
||
| let summary_guard = summary::SummaryOnDrop::new(started_at); | ||
| debug!( | ||
| target: targets::HTTP_SERVER, | ||
|
|
@@ -191,7 +204,7 @@ async fn graphql_endpoint_handler( | |
| "http request started", | ||
| ); | ||
|
|
||
| let (response_mode, inner_res) = graphql_endpoint_dispatch( | ||
| let inner_res = graphql_endpoint_dispatch( | ||
| &mut request, | ||
| body_stream, | ||
| schema_state, | ||
|
|
@@ -221,16 +234,11 @@ async fn graphql_endpoint_handler( | |
| .store(payload_bytes, std::sync::atomic::Ordering::Relaxed); | ||
| }); | ||
|
|
||
| (response_mode, inner_res, summary_guard) | ||
| (inner_res, summary_guard) | ||
| } | ||
| .await; | ||
|
|
||
| // for streamed responses the summary must be emitted when the stream ends, not now | ||
| // we do that by attaching the summary guard to the response body, so it will be emitted | ||
| // when the stream terminates (or the client disconnects) | ||
| if response_mode.can_stream() { | ||
| response = summary_guard.attach_to_response(response); | ||
| } | ||
| response = summary_guard.attach_to_response(response); | ||
|
Member
Author
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. We need to attach all kind of requests, not just streams. This also makes the |
||
|
|
||
| let graphql_operation = read_graphql_operation_metric_identity(&request); | ||
| let graphql_operation_name = graphql_operation | ||
|
|
@@ -259,7 +267,7 @@ async fn graphql_endpoint_dispatch( | |
| schema_state: web::types::State<Arc<SchemaState>>, | ||
| app_state: web::types::State<Arc<RouterSharedState>>, | ||
| parent_ctx: opentelemetry::Context, | ||
| ) -> (ResponseMode, web::HttpResponse) { | ||
| ) -> web::HttpResponse { | ||
| let root_http_request_span = HttpServerRequestSpan::from_request( | ||
| request, | ||
| &app_state | ||
|
|
@@ -343,7 +351,7 @@ async fn graphql_endpoint_dispatch( | |
|
|
||
| root_http_request_span.record_response(&response); | ||
|
|
||
| (response_mode, response) | ||
| response | ||
| } | ||
| .instrument(root_http_request_span.clone()) | ||
| .await | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| use std::borrow::Cow; | ||
| use std::collections::{BTreeMap, HashSet}; | ||
| use std::error::Error; | ||
| use std::future::Future; | ||
|
|
@@ -37,6 +38,7 @@ pub struct RequestSummary { | |
| pub duration_ms: AtomicU64, | ||
| pub supergraph_identifier: AtomicU64, | ||
| pub custom: Mutex<BTreeMap<String, sonic_rs::Value>>, | ||
| pub message: OnceLock<Cow<'static, str>>, | ||
| } | ||
|
|
||
| impl RequestSummary { | ||
|
|
@@ -94,6 +96,11 @@ impl RequestSummary { | |
| } | ||
| } | ||
|
|
||
| /// Overrides the summary log line's message. First call wins; later calls are no-ops. | ||
|
Member
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 had to read all of the code to understand why are other call no-ops - maybe help future viewers by explaining why right here in the comment? |
||
| pub fn set_message(&self, message: impl Into<Cow<'static, str>>) { | ||
| let _ = self.message.set(message.into()); | ||
| } | ||
|
|
||
| pub fn record_subgraph(&self, name: &str) { | ||
| self.subgraph_requests.fetch_add(1, Relaxed); | ||
| if let Ok(mut subgraphs) = self.involved_subgraphs.lock() { | ||
|
|
@@ -116,6 +123,7 @@ impl RequestSummary { | |
|
|
||
| info!( | ||
| target: targets::SUMMARY, | ||
| message = self.message.get().map(Cow::as_ref), | ||
| client_name = self.client_name.get().map(String::as_str), | ||
| client_version = self.client_version.get().map(String::as_str), | ||
| operation_name = self.operation_name.get().map(String::as_str), | ||
|
|
@@ -160,6 +168,10 @@ pub fn record(f: impl FnOnce(&RequestSummary)) { | |
| let _ = REQUEST_SUMMARY.try_with(|summary| f(summary)); | ||
| } | ||
|
|
||
| pub fn current_summary() -> Option<Arc<RequestSummary>> { | ||
| REQUEST_SUMMARY.try_with(|summary| summary.clone()).ok() | ||
| } | ||
|
|
||
| pub fn emit() { | ||
| if !is_enabled() { | ||
| return; | ||
|
|
@@ -242,10 +254,15 @@ impl Drop for SummaryOnDrop { | |
| }; | ||
| summary.set_duration(self.started_at.elapsed()); | ||
|
|
||
| match self.request_ids.take() { | ||
| Some(ids) => REQUEST_IDENTIFIERS.sync_scope(ids, || summary.emit()), | ||
| None => summary.emit(), | ||
| } | ||
| // Re-enter both task-locals before emitting: by now (especially for responses whose | ||
| // body outlives the original request future) they may no longer be ambiently scoped, | ||
| // but the formatters look up `custom`/`correlations` independently via their own | ||
| // `try_with` at format time, so both must be active for that lookup to succeed. | ||
| let request_ids = self.request_ids.take(); | ||
|
Member
Author
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. This fix the sync issue and ensures |
||
| REQUEST_SUMMARY.sync_scope(summary, || match request_ids { | ||
| Some(ids) => REQUEST_IDENTIFIERS.sync_scope(ids, emit), | ||
| None => emit(), | ||
| }); | ||
| } | ||
| } | ||
|
|
||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
the actual front-facing "api" for plugin system
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.
what if you have multiple plugins setting the summary message? the "why" does only the first call win will be questioned - like with #1379 (comment) - maybe explain why here?
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.
first call wins, otherwise we have to make the mechanism more complex with locks. I assumed users will implement their own plugin in order to override, so race conditions / conflicts are not expected.