Skip to content

Fix JSON log parameter ordering - #2506

Open
Pybsama wants to merge 1 commit into
aws:masterfrom
Pybsama:codex/fix-ilogger-json-state-order
Open

Fix JSON log parameter ordering#2506
Pybsama wants to merge 1 commit into
aws:masterfrom
Pybsama:codex/fix-ilogger-json-state-order

Conversation

@Pybsama

@Pybsama Pybsama commented Jul 30, 2026

Copy link
Copy Markdown

Issue #, if available:

Fixes #2468

Description of changes:

When the JSON logging path receives a custom ILogger state whose enumeration
order differs from {OriginalFormat}, order the callback arguments by message
template placeholder name instead of by state position.

The implementation:

  • preserves missing placeholder slots and handles duplicate placeholder names;
  • supports destructuring hints, alignment, format specifiers, escaped braces,
    and positional templates;
  • keeps the existing fast path when state is already aligned, and uses the same
    1,024-entry soft cache cap as RuntimeSupport; and
  • adds focused regressions for the ASP.NET Core hosting-log shape and the edge
    cases above, plus the required AutoVer patch change file.

Validation:

  • dotnet test Libraries/test/Amazon.Lambda.Logging.AspNetCore.Tests/Amazon.Lambda.Logging.AspNetCore.Tests.csproj --framework net10.0 --configuration Debug (20 passed)
  • dotnet test Libraries/test/Amazon.Lambda.Logging.AspNetCore.Tests/Amazon.Lambda.Logging.AspNetCore.Tests.csproj --framework net10.0 --configuration Release (20 passed)
  • dotnet build Libraries/test/Amazon.Lambda.Logging.AspNetCore.Tests/Amazon.Lambda.Logging.AspNetCore.Tests.csproj --configuration Release --framework net8.0 (0 warnings, 0 errors)
  • repeated the same checks after applying the commit cleanly to current master (fdb84c71)
  • AutoVer change-file validation and git -c core.whitespace=cr-at-eol diff --check

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@madmox

madmox commented Aug 21, 2026

Copy link
Copy Markdown

Reporter of #2468 here. The approach looks right, two practical notes:

  • It's still marked as draft, so it won't reach the maintainers' review queue.
  • It targets master. CONTRIBUTING.md does say master, but every recently merged PR here (release PRs included) targets dev; worth confirming before this stalls.

Also worth knowing: #2474 inserts a block immediately before the LambdaLogger.Log(...) call this PR modifies. The two are behaviorally independent, but whichever lands second will need a rebase.

@Pybsama

Pybsama commented Aug 28, 2026

Copy link
Copy Markdown
Author

Thanks for both notes. I checked them against the current repository state: #2545 retired the dev branch, so master is now the correct single-trunk target. I also applied this commit cleanly to current master (fdb84c71) and reran the focused suite: 20/20 tests in both Debug and Release on net10.0, plus a net8.0 Release build with 0 warnings and 0 errors.\n\n#2474 is still open. I confirmed that the changes are behaviorally independent but touch the same source/test regions, so whichever lands second will need a conflict resolution. If #2474 lands first, I will rebase this PR and add the combined category-prefix + reordered-parameters regression. Marking this ready for maintainer review now.

@Pybsama
Pybsama marked this pull request as ready for review August 28, 2026 18:33
@Pybsama
Pybsama requested review from a team as code owners August 28, 2026 18:33
@Pybsama
Pybsama requested review from GarrettBeatty and normj August 28, 2026 18:33
@GarrettBeatty
GarrettBeatty requested a balanced review from Copilot August 28, 2026 23:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes JSON structured logging by aligning custom ILogger state values with template placeholders.

Changes:

  • Adds name-based parameter ordering and template parsing.
  • Adds regression and edge-case tests.
  • Adds an AutoVer patch entry.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
LambdaILogger.cs Implements parameter reordering and parsing.
LoggingTests.cs Tests ordering and template edge cases.
.autover/changes/58593c38-0c20-4fee-993c-15ea9f1f49fb.json Records the patch change.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +147 to +150
if (UsesPositionalArguments(templateProperties))
{
return GetPropertyValues(properties);
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JSON log format pairs ILogger message template placeholders with state values by position, garbling ASP.NET Core hosting logs

3 participants