feat(router): allow to customize access log message by plugin system - #1379
feat(router): allow to customize access log message by plugin system#1379dotansimha wants to merge 3 commits into
Conversation
c9b774c to
8427a5d
Compare
There was a problem hiding this comment.
Pull request overview
Adds a plugin-facing API to override the request summary (access log) message and adjusts summary emission timing so end-of-request hooks can enrich the summary before it is logged.
Changes:
- Introduces
set_summary_message(andget_current_summary) to allow plugins to customize the summary message based on request outcome data. - Updates request summary logging to carry a customizable
messagefield and re-scopes task-locals when emitting from a drop guard. - Extends E2E and plugin example tests to validate customizable summary messages.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| plugin_examples/custom_logger_correlation/src/test.rs | Adds assertions that the plugin customized the request summary log message. |
| plugin_examples/custom_logger_correlation/src/plugin.rs | Uses an on_end callback to compute and set a custom summary message. |
| plugin_examples/custom_logger_correlation/router.config.yaml | Changes example logging config (currently conflicts with JSON-capturing test expectations). |
| plugin_examples/Cargo.lock | Bumps hive-router-related dependency versions for plugin examples. |
| lib/internal/src/telemetry/logging/summary.rs | Adds a customizable message field to RequestSummary and re-enters task-locals before emitting. |
| e2e/src/telemetry/logging.rs | Updates JSON snapshots to include message and adds a test for plugin-customized messages. |
| bin/router/src/lib.rs | Exposes get_current_summary, adds set_summary_message, and defers summary emission by attaching guard to response body. |
| .changeset/expose-summary-message-to-plugin-system.md | Adds release notes for the new plugin capability (currently mismatched with implemented default behavior). |
Suppressed comments (1)
bin/router/src/lib.rs:244
attach_to_responsewraps every response body so the summary can be emitted later, but this also adds per-body polling overhead even when the summary target is filtered off (in which caseSummaryOnDrophas no captured summary and will never emit). Guard the wrapping behindsummary::is_enabled()to preserve the existing cheap no-op behavior when summary logging is disabled.
response = summary_guard.attach_to_response(response);
let graphql_operation = read_graphql_operation_metric_identity(&request);
let graphql_operation_name = graphql_operation
.as_ref()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d168927 to
e7edda7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (5)
.changeset/expose-summary-message-to-plugin-system.md:9
- The changeset claims the default summary has "no message", but this PR's e2e snapshots now expect
"message": "request summary"for the default case. The release note should be updated to match the actual default behavior, and (optionally) mentionget_current_summary()since the plugin example relies on it.
Plugins can now override the request summary log line's message via `hive_router::set_summary_message(message)`, callable from any hook.
plugin_examples/custom_logger_correlation/src/plugin.rs:5
EndHookPayloadis imported but never referenced in this example plugin. If the repo is built with-D warnings, this will fail compilation due to an unused import.
use hive_router::plugins::plugin_trait::{EndHookPayload, RouterPlugin, StartHookPayload};
lib/internal/src/telemetry/logging/summary.rs:126
messageis recorded as anOption<&str>, but nothing in this code sets a default message. That means non-customized summaries will either omitmessageor serialize it as a null/"None"-like value, which contradicts the updated e2e snapshots expecting"message": "request summary". Consider emitting a stable default when no plugin override is set.
message = self.message.get().map(Cow::as_ref),
lib/internal/src/telemetry/logging/summary.rs:173
current_summary()returns a summary even when summary logging is disabled (because requests are still scoped withdisabled_summary()). That can mislead plugins into reading default/empty fields (e.g., status_code=0) even though the summary is effectively inactive. Align this withrecord()/emit()semantics by returningNonewhen!is_enabled().
pub fn current_summary() -> Option<Arc<RequestSummary>> {
REQUEST_SUMMARY.try_with(|summary| summary.clone()).ok()
}
bin/router/src/lib.rs:148
- This new public API is missing a doc comment, while adjacent plugin-facing helpers are documented. Adding a short doc comment would clarify that it is request-scoped and primarily intended for end hooks (and what happens when summary logging is disabled).
pub fn get_current_summary() -> Option<Arc<summary::RequestSummary>> {
summary::current_summary()
}
e7edda7 to
bdb6bfd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (4)
lib/internal/src/telemetry/logging/summary.rs:173
current_summary()can return the shareddisabled_summary()instance when the summary target is filtered off. Exposing that shared instance (and especially viahive_router::get_current_summary) is risky: plugins could accidentally mutate it and leak state across requests, and even reads will be misleading because it is never populated when disabled. Consider returningNonewhen!is_enabled()instead.
pub fn current_summary() -> Option<Arc<RequestSummary>> {
REQUEST_SUMMARY.try_with(|summary| summary.clone()).ok()
}
lib/internal/src/telemetry/logging/summary.rs:127
RequestSummary::emitrecords amessagefield only when a plugin has set one, which means the default summary log line won’t include amessagekey at all. The e2e snapshots now expectmessage: "request summary", so this should have an explicit default to keep JSON output stable when no plugin overrides it.
This issue also appears on line 171 of the same file.
info!(
target: targets::SUMMARY,
message = self.message.get().map(Cow::as_ref),
client_name = self.client_name.get().map(String::as_str),
bin/router/src/lib.rs:148
get_current_summary()exposessummary::RequestSummaryin the public API. That type has manypubinternals (atomics/mutexes/OnceLocks), so this effectively freezes internal telemetry structure for semver and allows plugins to mutate fields that probably shouldn’t be part of the stable plugin contract. A safer API would expose a read-only snapshot (or targeted getters) for the small set of fields plugins need (status code, duration, operation name) rather than returning the internal struct.
pub fn get_current_summary() -> Option<Arc<summary::RequestSummary>> {
summary::current_summary()
}
plugin_examples/custom_logger_correlation/src/plugin.rs:5
EndHookPayloadandStartHookPayloadare imported but unused in this module; if the workspace/CI is configured with-D warnings, this will fail builds. Consider removing unused imports (or explicitly allowing unused imports in examples if that’s intentional).
use hive_router::plugins::plugin_trait::{EndHookPayload, RouterPlugin, StartHookPayload};
| } | ||
| } | ||
|
|
||
| /// Overrides the summary log line's message. First call wins; later calls are no-ops. |
There was a problem hiding this comment.
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?
| if response_mode.can_stream() { | ||
| response = summary_guard.attach_to_response(response); | ||
| } | ||
| response = summary_guard.attach_to_response(response); |
There was a problem hiding this comment.
We need to attach all kind of requests, not just streams. This also makes the duration_ms more accurate, because it emit when the request is really full dropped.
|
|
||
| let started_at = std::time::Instant::now(); | ||
| let (response_mode, mut response, summary_guard) = async { | ||
| let (mut response, summary_guard) = async { |
There was a problem hiding this comment.
we just no longer needs respose_mode here at all
|
|
||
| /// Lets plugins override the request summary log line's message. The first call wins; | ||
| /// later calls for the same request are no-ops. | ||
| pub fn set_summary_message(message: impl Into<std::borrow::Cow<'static, str>>) { |
There was a problem hiding this comment.
the actual front-facing "api" for plugin system
There was a problem hiding this comment.
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.
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.
| // 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(); |
There was a problem hiding this comment.
This fix the sync issue and ensures on_end of on_http_request has access to both the summary and log correlations
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
.changeset/expose-summary-message-to-plugin-system.md:9
- The changeset documents
set_summary_message, but this PR also addsget_current_summary()(used by the custom logger example) which is part of the new plugin-facing surface area. Please either documentget_current_summary()here as well, or keep it non-public if it’s not meant for plugin authors.
Plugins can now override the request summary log line's message via `hive_router::set_summary_message(message)`, callable from any hook.
bin/router/src/lib.rs:148
get_current_summary()exposeshive_router_internal::...::RequestSummarydirectly in the publichive_routerAPI. This leaks an internal type (and its mutable fields) to plugins and makes the API harder to use/keep stable (downstream crates can’t name the type without adding a directhive_router_internaldependency). Consider returning an opaque, read-only snapshot type defined inhive_router(or dedicated getters for the fields needed to build a message). If exposing the full struct is intentional, it should be explicitly documented as stable/unstable and re-exported underhive_routerto avoid coupling consumers to an internal crate path.
pub fn get_current_summary() -> Option<Arc<summary::RequestSummary>> {
summary::current_summary()
}
|
🐋 This PR was built and pushed to the following Docker images: Image Names: Platforms: Image Tags: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
plugin_examples/custom_logger_correlation/src/plugin.rs:5
- Unused
EndHookPayload/StartHookPayloadimports here (the example doesn’t reference them), which can trigger warnings and makes the snippet noisier than necessary.
use hive_router::plugins::plugin_trait::{EndHookPayload, RouterPlugin, StartHookPayload};
lib/internal/src/telemetry/logging/summary.rs:173
current_summary()returns aRequestSummaryeven when the summary target is filtered off (becauseWithRequestSummarystill scopes a disabled summary). That makesget_current_summary()misleading and can cause plugins to read default/stale values and do unnecessary work even thoughrecord()/emit()are no-ops when disabled. Consider returningNonewhen!is_enabled()to align semantics with the rest of the API.
pub fn current_summary() -> Option<Arc<RequestSummary>> {
REQUEST_SUMMARY.try_with(|summary| summary.clone()).ok()
}
Closes #1378