feat: Add logging support to Management API client#1242
feat: Add logging support to Management API client#1242fern-api[bot] wants to merge 4 commits intomasterfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1242 +/- ##
==========================================
- Coverage 78.19% 77.74% -0.45%
==========================================
Files 354 359 +5
Lines 17742 17892 +150
Branches 9413 9456 +43
==========================================
+ Hits 13874 13911 +37
- Misses 3868 3981 +113
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
developerkunal
changed the title
- Export logging namespace from src/management/index.ts - Add comprehensive logging documentation to README.md - Include examples for debug logging, custom loggers, and ConsoleLogger - Document log levels and security features (automatic redaction)
developerkunal
dismissed stale reviews from tanya732 and kushalshit27
via
1024238
|
Claude finished @gyaneshgouraw-okta's task —— View job Overall Assessment❌ This PR introduces a useful logging feature but has critical issues that must be fixed before merging, particularly a logic bug that inverts the log level filtering behavior. Critical Issues:
Security Concerns:
Performance Issues:
Code Quality:
Positive Aspects:
The logging feature is valuable, but the inverted log level logic is a blocker that will cause incorrect behavior in production. Job Run • Branch: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * @returns True if the level should be logged | ||
| */ | ||
| public shouldLog(level: LogLevel): boolean { | ||
| return !this.silent && this.level >= logLevelMap[level]; |
There was a problem hiding this comment.
The logic in shouldLog is inverted. Currently, it checks this.level >= logLevelMap[level], which means a higher level number (e.g., Error=4) will output logs for lower levels (e.g., Debug=1). This is backwards.
For example:
- If
this.level = Error (4)and we calldebug()(level=1), the check becomes4 >= 1→ true, so debug logs appear - If
this.level = Debug (1)and we callerror()(level=4), the check becomes1 >= 4→ false, so error logs are suppressed
The comparison should be <= instead of >=.
|
|
||
| if (!hasSensitive) { | ||
| return url; // Early exit - no sensitive params | ||
| } |
There was a problem hiding this comment.
This fast-path check includes lower.includes("auth") which will incorrectly flag many legitimate query parameters as sensitive. For example:
?author=johnwould trigger redaction?authenticate_user=falsewould trigger redaction?authorization_type=implicitwould trigger redaction
Consider removing lower.includes("auth") or making it more specific (e.g., lower.includes("auth_token") or lower.includes("auth-token")).
| const decodedKey = decodeURIComponent(key); | ||
| shouldRedact = SENSITIVE_QUERY_PARAMS.has(decodedKey.toLowerCase()); | ||
| } catch {} | ||
| } |
There was a problem hiding this comment.
The URL decoding attempt catches all errors silently without any logging or handling. This could mask legitimate issues. Additionally, URL-encoded keys should be checked before decoding to avoid unnecessary decoding attempts.
Consider checking if URL encoding exists before attempting to decode, and at minimum log caught errors when debug logging is enabled.
| ); | ||
|
|
||
| if (atIndex < firstDelimiter) { | ||
| url = `${url.slice(0, afterProtocol)}[REDACTED]@${url.slice(atIndex + 1)}`; |
There was a problem hiding this comment.
The URL redaction logic only redacts credentials in the authority section (user:pass@host) but doesn't handle edge cases:
- Empty credentials (e.g.,
http://:password@hostorhttp://user:@host) - Multiple
@symbols (e.g.,http://user:pass@word@host)
The current logic would fail to properly redact case 2, as it would only redact up to the first @.
| url: redactUrl(url), | ||
| headers: redactHeaders(headers), | ||
| queryParameters: redactQueryParameters(args.queryParameters), | ||
| hasBody: requestBody != null, |
There was a problem hiding this comment.
Logging the request body content is missing. For debugging purposes, it would be valuable to log whether a body exists and potentially its size or a redacted version of its content. This would help diagnose issues with malformed requests.
| if (config instanceof Logger) { | ||
| return config; | ||
| } | ||
| config = config ?? {}; |
There was a problem hiding this comment.
The null coalescing assignment config = config ?? {} on line 192 is redundant since line 186 already checks if config == null and returns early. This line will never execute with a null config.
| } | ||
| } | ||
|
|
||
| const SENSITIVE_HEADERS = new Set([ |
There was a problem hiding this comment.
Missing sensitive headers in the redaction list:
x-auth-tokenandx-api-keyare included, but common variants are missing:bearer(standalone Bearer tokens)x-access-tokenx-session-tokenx-refresh-tokenx-client-secretauthentication(some APIs use this instead of Authorization)
| @@ -106,13 +256,29 @@ export async function fetcherImpl<R = unknown>(args: Fetcher.Args): Promise<APIR | |||
| ); | |||
|
|
|||
| if (response.status >= 200 && response.status < 400) { | |||
There was a problem hiding this comment.
Response headers should be redacted before logging for security. Response headers may contain sensitive information like:
Set-Cookieheaders with session tokensX-RateLimit-*headers that could reveal API usage patterns- Custom authentication headers
Consider redacting response headers similar to how request headers are redacted.
| } | ||
| return { | ||
| ok: true, | ||
| body: (await getResponseBody(response, args.responseType)) as R, |
There was a problem hiding this comment.
The response body is never logged or included in debug output. While this is good for security (avoiding logging sensitive data), it makes debugging difficult. Consider:
- Adding a configuration option to enable/disable body logging
- Logging body size/type information
- Providing a sanitized preview for non-sensitive content types
| ### Logging | ||
|
|
||
| The SDK provides built-in logging support to help with debugging and monitoring API requests. Logging is disabled by default to avoid noise in production environments. | ||
|
|
There was a problem hiding this comment.
The documentation states logging is "disabled by default" but technically it's "silent by default" (silent: true). The logging infrastructure is always instantiated, it just doesn't output anything. Consider clarifying this distinction in the docs, as users might think there's zero performance overhead when disabled.
| }); | ||
| const fetchFn = args.fetchFn ?? (await getFetchFn()); | ||
| const headers = await getHeaders(args); | ||
| const logger = createLogger(args.logging); |
There was a problem hiding this comment.
The logger is created on every request, even when logging is disabled. This creates unnecessary object allocation overhead. Consider:
- Creating the logger once during client initialization and reusing it
- Adding an early-exit check if
args.loggingis undefined andsilentis true - Caching the logger instance per configuration
4 participants
Changes
This PR adds comprehensive logging support to the Management API client through Fern regeneration. The changes include:
New Logging Framework:
Loggerclass with level-based logging (debug, info, warn, error)ConsoleLoggerimplementation for console outputLogConfiginterface for logger configurationHTTP Request Logging:
API Integration:
loggingparameter toBaseClientOptionsinterfaceConfiguration:
silent: true) to avoid breaking existing implementationsUsage Examples:
References
This is a Fern-generated update that adds logging infrastructure to support debugging and monitoring of Management API calls.
Testing
Testing approach:
loggingconfiguration:Checklist