You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This PR adds comprehensive telemetry support for hook execution, including detailed logging of hook calls, performance metrics, and PII sanitization to protect sensitive information. It introduces new telemetry events (HookCallEvent) and a sanitization utility for hook names.
Dive Deeper
This PR implements a comprehensive telemetry system for hook execution that follows the existing telemetry patterns in the codebase. The design consists of four main layers: 1. Event Generation Layer (hookEventHandler.ts)
The HookEventHandler class acts as the orchestrator, intercepting all hook executions
After hooks complete (success or failure), it calls logHookExecution() to emit telemetry
2. Logging Layer (loggers.ts)
logHookCall() receives HookCallEvent and emits to OpenTelemetry
This PR implements comprehensive telemetry support for hook execution, introducing HookCallEvent, logHookCall(), and recordHookCallMetrics() following existing telemetry patterns. The implementation includes PII sanitization for hook names and proper error handling. Overall, the code is well-structured and follows established conventions, though there are a few areas for improvement in type safety, error handling, and test coverage.
🔍 General Feedback
The implementation follows existing telemetry patterns consistently (similar to logToolCall, logApiRequest)
Good separation of concerns across event generation, logging, and metrics layers
Comprehensive test coverage for the new functionality
PII sanitization is well-implemented with the sanitizeHookName() utility
The use of WeakMap for tracking reported failures is a clever approach to prevent duplicate warnings
Good use of TypeScript types and interfaces throughout
🎯 Specific Feedback
🟡 High
File: packages/core/src/hooks/hookEventHandler.ts:93 - The requestContext parameter is typed as object which is too generic. Consider using a more specific type or interface to better document the expected shape and improve type safety.
File: packages/core/src/telemetry/sanitize.ts:27 - The sanitizeHookName() function doesn't handle all edge cases properly. For example, paths like ./ or ../ without a filename would return '.' or '..' instead of 'unknown-command'. Consider adding validation after extracting the basename.
File: packages/core/src/telemetry/types.ts:860 - The toOpenTelemetryAttributes() method conditionally includes sensitive data based on getTelemetryLogPromptsEnabled(), but the error message is always included even when logging is disabled. This could potentially leak sensitive information if the error message contains PII. Consider sanitizing error messages as well.
🟢 Medium
File: packages/core/src/hooks/hookEventHandler.ts:115-120 - The onHookStart and onHookEnd callbacks now have proper debug logging, but the parameters are renamed from _config to config and _index to index without actually using them in the function body. If these parameters are intentionally unused, consider prefixing with underscore to follow common conventions (_config, _index) or use them for more detailed logging.
File: packages/core/src/telemetry/types.ts:851 - The hook_output attribute is serialized with safeJsonStringify() but could be undefined. While this is handled, consider being explicit about the serialization behavior when the value is undefined for consistency with other telemetry events.
File: packages/core/src/telemetry/metrics.ts:568-577 - The recordHookCallMetrics() function always sanitizes hook names in metrics, which is correct. However, the comment says "Always sanitize hook names in metrics (metrics are aggregated and exposed)" - consider moving this sanitization logic to a shared utility or adding a reference to the sanitization module documentation for better discoverability.
File: packages/core/src/telemetry/loggers.ts:109 - The getCommonAttributes function is now exported, which is necessary for the hook telemetry implementation. Consider adding a brief JSDoc comment explaining its purpose and when it should be used.
🔵 Low
File: packages/core/src/hooks/hookEventHandler.ts:206-207 - The comment about suppressOutput being "already handled by not logging above when true" was removed, but there's no corresponding explanation of how suppressOutput is currently handled. Consider adding a comment clarifying the current handling mechanism.
File: packages/core/src/telemetry/sanitize.test.ts:22-24 - The test case "should return 'unknown-command' for null/undefined values" only tests empty strings, not actual null/undefined. The function signature takes a string, so null/undefined wouldn't compile. Consider removing this test case or clarifying what it's testing.
File: packages/core/src/telemetry/types.ts:802 - The constant EVENT_HOOK_CALL is defined but EVENT_HOOK_CALL_COUNT and EVENT_HOOK_CALL_LATENCY in metrics.ts use a different pattern (${SERVICE_NAME}.hook_call.count). Consider using the constant for consistency or document the naming convention.
File: packages/core/src/telemetry/types.ts:880 - The toLogBody() method creates a hookId as ${this.hook_event_name}.${this.hook_name}. For very long hook names or event names, this could create excessively long log bodies. Consider truncating or hashing very long identifiers.
✅ Highlights
Excellent use of WeakMap with the original request object as a key to suppress duplicate failure warnings - this is a sophisticated solution to a common problem
The sanitizeHookName() function handles both Unix and Windows paths, showing thoughtful cross-platform consideration
Comprehensive test coverage including edge cases for sanitization, different hook event names, and both success/failure scenarios
Good separation of sensitive data handling based on telemetry configuration settings
The implementation closely follows existing patterns in the codebase, making it easy to understand and maintain
Well-documented with clear JSDoc comments explaining the purpose and behavior of key functions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TLDR
This PR adds comprehensive telemetry support for hook execution, including detailed logging of hook calls, performance metrics, and PII sanitization to protect sensitive information. It introduces new telemetry events (
HookCallEvent) and a sanitization utility for hook names.Dive Deeper
This PR implements a comprehensive telemetry system for hook execution that follows the existing telemetry patterns in the codebase. The design consists of four main layers:
1. Event Generation Layer (
hookEventHandler.ts)HookEventHandlerclass acts as the orchestrator, intercepting all hook executionslogHookExecution()to emit telemetry2. Logging Layer (
loggers.ts)logHookCall()receives HookCallEvent and emits to OpenTelemetryReviewer Test Plan
Testing Matrix
Linked issues / bugs