Eliminate unecessary allocations for suppressed logging levels - #5769
Open
solo (solonovamax) wants to merge 6 commits into
Open
Eliminate unecessary allocations for suppressed logging levels#5769solo (solonovamax) wants to merge 6 commits into
solo (solonovamax) wants to merge 6 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughLogging calls throughout Ktor now use lazy lambda-based trace and debug forms. Throwable-aware lazy overloads were added to the logging utilities, and client, server, shared, and test code was updated without changing functional control flow. ChangesLazy logging migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
ktor-client/ktor-client-plugins/ktor-client-content-negotiation/common/src/io/ktor/client/plugins/contentnegotiation/ContentNegotiation.kt (1)
341-344: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse the lazy trace overload for the final conversion log.
The new
isTraceEnabledguard still surrounds an eagerLOGGER.trace("...")call. Use the same lazy form as the surrounding migration:Proposed fix
- if (LOGGER.isTraceEnabled && result !is ByteReadChannel) { - LOGGER.trace("Response body was converted to ${result::class} for $requestUrl.") + if (result !is ByteReadChannel) { + LOGGER.trace { "Response body was converted to ${result::class} for $requestUrl." } }This keeps level gating in one place and aligns the remaining call with the lazy logging objective.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ktor-client/ktor-client-plugins/ktor-client-content-negotiation/common/src/io/ktor/client/plugins/contentnegotiation/ContentNegotiation.kt` around lines 341 - 344, Update the final conversion log in the response deserialization flow around suitableConverters.deserialize to use the logger’s lazy trace overload, removing the explicit LOGGER.isTraceEnabled guard while preserving the ByteReadChannel exclusion and existing message content.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCacheEntry.kt`:
- Around line 120-124: Update the request max-age parsing in HttpCacheEntry to
track whether a max-age directive was found separately from its numeric value.
Keep requestMaxAge equal to zero for absent or invalid values so existing
validation behavior is unchanged, but emit the LOGGER.trace message only when
the directive is actually absent and retain accurate wording for explicit
max-age=0.
In `@ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/UserAgent.kt`:
- Line 34: Update the trace message in the User-Agent header handling to
interpolate the configured value from pluginConfig.agent (the local agent
variable) instead of logging the literal text “agent”; preserve the existing
request URL context.
In
`@ktor-server/ktor-server-plugins/ktor-server-status-pages/common/src/io/ktor/server/plugins/statuspages/StatusPages.kt`:
- Around line 93-103: Update the three CallFailed trace statements in the
status-page handling flow around findHandlerByValue and handler execution to use
the throwable-aware LOGGER.trace(cause) overload, removing $cause from each
message while retaining the surrounding request URI, handler, and failure
context.
---
Nitpick comments:
In
`@ktor-client/ktor-client-plugins/ktor-client-content-negotiation/common/src/io/ktor/client/plugins/contentnegotiation/ContentNegotiation.kt`:
- Around line 341-344: Update the final conversion log in the response
deserialization flow around suitableConverters.deserialize to use the logger’s
lazy trace overload, removing the explicit LOGGER.isTraceEnabled guard while
preserving the ByteReadChannel exclusion and existing message content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9276f260-536a-4f74-9222-72617e498ec6
📒 Files selected for processing (42)
ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/DefaultRequest.ktktor-client/ktor-client-core/common/src/io/ktor/client/plugins/DefaultResponseValidation.ktktor-client/ktor-client-core/common/src/io/ktor/client/plugins/DefaultTransform.ktktor-client/ktor-client-core/common/src/io/ktor/client/plugins/HttpCallValidator.ktktor-client/ktor-client-core/common/src/io/ktor/client/plugins/HttpPlainText.ktktor-client/ktor-client-core/common/src/io/ktor/client/plugins/HttpRedirect.ktktor-client/ktor-client-core/common/src/io/ktor/client/plugins/HttpRequestLifecycle.ktktor-client/ktor-client-core/common/src/io/ktor/client/plugins/HttpRequestRetry.ktktor-client/ktor-client-core/common/src/io/ktor/client/plugins/UserAgent.ktktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCacheEntry.ktktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cookies/HttpCookies.ktktor-client/ktor-client-plugins/ktor-client-auth/common/src/io/ktor/client/plugins/auth/providers/BasicAuthProvider.ktktor-client/ktor-client-plugins/ktor-client-auth/common/src/io/ktor/client/plugins/auth/providers/BearerAuthProvider.ktktor-client/ktor-client-plugins/ktor-client-content-negotiation/common/src/io/ktor/client/plugins/contentnegotiation/ContentNegotiation.ktktor-server/ktor-server-core/common/src/io/ktor/server/application/ApplicationPlugin.ktktor-server/ktor-server-core/common/src/io/ktor/server/config/ConfigLoaders.ktktor-server/ktor-server-core/common/src/io/ktor/server/engine/DefaultEnginePipeline.ktktor-server/ktor-server-core/common/src/io/ktor/server/engine/DefaultTransform.ktktor-server/ktor-server-core/jvm/src/io/ktor/server/engine/EmbeddedServerJvm.ktktor-server/ktor-server-plugins/ktor-server-auth-jwt/jvm/src/io/ktor/server/auth/jwt/JWTAuth.ktktor-server/ktor-server-plugins/ktor-server-auth-jwt/jvm/src/io/ktor/server/auth/jwt/JWTUtils.ktktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/AuthenticationInterceptors.ktktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/OAuthProcedure.ktktor-server/ktor-server-plugins/ktor-server-call-id/common/src/io/ktor/server/plugins/callid/CallId.ktktor-server/ktor-server-plugins/ktor-server-call-logging/jvm/src/io/ktor/server/plugins/calllogging/CallLogging.ktktor-server/ktor-server-plugins/ktor-server-compression/jvm/src/io/ktor/server/plugins/compression/Compression.ktktor-server/ktor-server-plugins/ktor-server-content-negotiation/common/src/io/ktor/server/plugins/contentnegotiation/RequestConverter.ktktor-server/ktor-server-plugins/ktor-server-content-negotiation/common/src/io/ktor/server/plugins/contentnegotiation/ResponseConverter.ktktor-server/ktor-server-plugins/ktor-server-double-receive/common/src/io/ktor/server/plugins/doublereceive/DoubleReceive.ktktor-server/ktor-server-plugins/ktor-server-partial-content/common/src/io/ktor/server/plugins/partialcontent/PartialContent.ktktor-server/ktor-server-plugins/ktor-server-partial-content/common/src/io/ktor/server/plugins/partialcontent/PartialContentUtils.ktktor-server/ktor-server-plugins/ktor-server-rate-limit/common/src/io/ktor/server/plugins/ratelimit/RateLimitInterceptors.ktktor-server/ktor-server-plugins/ktor-server-sessions/common/src/io/ktor/server/sessions/SessionTrackerById.ktktor-server/ktor-server-plugins/ktor-server-sessions/common/src/io/ktor/server/sessions/SessionTrackerByValue.ktktor-server/ktor-server-plugins/ktor-server-sse/common/src/io/ktor/server/sse/SSEServerContent.ktktor-server/ktor-server-plugins/ktor-server-status-pages/common/src/io/ktor/server/plugins/statuspages/StatusPages.ktktor-server/ktor-server-plugins/ktor-server-websockets/common/src/io/ktor/server/websocket/Routing.ktktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.ktktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/CompressionTestSuite.ktktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/ContentTestSuite.ktktor-shared/ktor-websockets/common/src/io/ktor/websocket/PingPong.ktktor-utils/common/src/io/ktor/util/logging/Logger.kt
Contributor
Author
|
It seems that |
Bruce Hamilton (bjhham)
left a comment
Bruce Hamilton (bjhham)
left a comment
Contributor
There was a problem hiding this comment.
Thanks, I've been meaning to get around to these. Can you change the base to release/3.x so this can go into the next patch? You can use the ./switch-base-branch.sh for this.
Contributor
Author
sure, can do. I'll fix the test failure first though. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ktor-server/ktor-server-plugins/ktor-server-call-logging/jvm/test/io/ktor/server/plugins/calllogging/CallLoggingTest.kt (1)
514-514: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unconditional debug print from the test.
This writes the collected messages to stdout on every run, including successful runs, creating noisy CI output. Keep this diagnostic local to failure investigation instead of committing it.
Proposed fix
- println("messages:\n" + messages.joinToString(separator = ",\n", prefix = "[\n", postfix = "\n]") {" \"$it\""})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ktor-server/ktor-server-plugins/ktor-server-call-logging/jvm/test/io/ktor/server/plugins/calllogging/CallLoggingTest.kt` at line 514, Remove the unconditional println diagnostic that prints messages in CallLoggingTest; retain the test assertions and behavior unchanged, without adding replacement logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@ktor-server/ktor-server-plugins/ktor-server-call-logging/jvm/test/io/ktor/server/plugins/calllogging/CallLoggingTest.kt`:
- Line 514: Remove the unconditional println diagnostic that prints messages in
CallLoggingTest; retain the test assertions and behavior unchanged, without
adding replacement logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d53a23e-3797-497a-9885-19392ba4cb80
📒 Files selected for processing (1)
ktor-server/ktor-server-plugins/ktor-server-call-logging/jvm/test/io/ktor/server/plugins/calllogging/CallLoggingTest.kt
solo (solonovamax)
force-pushed
the
perf/logging-string-interpolation
branch
from
July 26, 2026 19:34
ded0908 to
293c31b
Compare
Uses lazy logging functions in order to avoid unecessary allocations for logging levels that have been disabled (trace/debug) Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
The warning is incorrect. Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
solo (solonovamax)
force-pushed
the
perf/logging-string-interpolation
branch
from
July 27, 2026 00:53
e385e22 to
76d9ff1
Compare
Contributor
Author
|
It seems the failures are due to flaky tests. |
Bruce Hamilton (bjhham)
approved these changes
Jul 27, 2026
Contributor
Author
|
Because cd11882 (Aleksei Tirman (@Stexxe)) in |
…-string-interpolation # Conflicts: # ktor-server/ktor-server-plugins/ktor-server-compression/jvm/src/io/ktor/server/plugins/compression/Compression.kt
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Uses lazy logging functions in order to avoid unecessary allocations for logging levels that have been disabled (trace/debug)
There are no behavioural changes, other than that the allocations will be decreased.
The only other noticeable change is that in a couple of places a
Throwablewas used in the string interpolation. I changed this so that it passed the throwable as a parameter to the logging function, that way it will print the stack trace.Also this fixes KTOR-9676 because the warning is wrong (and annoying), and since I'm already changing logging stuff, might as well do it here too.
Subsystem
A bunch:
Motivation
I kept seeing log messages with string interpolation and it annoyed me.
Solution
Use the lazy logging functions for trace/debug to not perform the string interpolation.