Honor IncludeCategory in the JSON log format - #2474
Conversation
There was a problem hiding this comment.
Pull request overview
Updates Amazon.Lambda.Logging.AspNetCore to honor LambdaLoggerOptions.IncludeCategory when Lambda JSON logging is enabled, so category information isn’t silently dropped when switching from text to JSON logs.
Changes:
- Prepend a
{Category}placeholder (and corresponding argument) to the JSON logging message template whenIncludeCategoryis enabled. - Add tests asserting JSON parameter logging includes/excludes category depending on
IncludeCategory. - Add an autover changelog entry describing the fix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Libraries/src/Amazon.Lambda.Logging.AspNetCore/LambdaILogger.cs | Adds category injection in the JSON logging branch when IncludeCategory is enabled. |
| Libraries/test/Amazon.Lambda.Logging.AspNetCore.Tests/LoggingTests.cs | Adds tests validating category presence/absence impacts parameter count in JSON mode. |
| .autover/changes/541b1775-ecf8-4e75-99c8-675924a5c34e.json | Records a patch changelog note for the logging fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (_options.IncludeCategory) | ||
| { | ||
| // Unlike the text format, the JSON format otherwise drops the | ||
| // category entirely, so IncludeCategory has no effect. Prepend a | ||
| // "{Category}" placeholder (which the JSON formatter turns into a | ||
| // queryable property) and supply the category value. | ||
| messageTemplate = "[{Category}] " + messageTemplate; | ||
| parameters.Insert(0, _categoryName); | ||
| } |
There was a problem hiding this comment.
Fixed in 536f8b4. Prepending the category as literal text instead of a {Category} placeholder means it can no longer get mixed into the formatter's positional vs named argument detection, so the caller's own placeholders and their values stay untouched either way. Added a test with a positional template to cover it.
|
Reporter of #2469 here, answering the question in your description: from a consumer standpoint the dedicated Unrelated nit: the branch normalizes |
|
That settles the field question, thanks. A real Line endings are fixed in bd5e0f2. The first commit had normalized the whole file to CRLF where upstream is mixed, so 63 of those lines were pure churn. The diff is 11 added and 0 deleted now, and |
|
@normj could you take a look when you get a chance? It has been open since July 11 and the workflows are still at action_required, so CI has never actually run on it. One thing needs your call rather than mine: madmox asked above for a dedicated top-level |
|
im fine with the change as-is for now. top level category can be a follow up pr if we want. norm is out of office until next week for reviewing |
|
I will ask someone else on the team to review in meantime since norm is out |
bd5e0f2 to
776b763
Compare
|
Rebased onto dev. That restore failure on your run was NU1903 against PowerShellHost, not this change, my branch just predated the System.Security.Cryptography.Xml bump (8.0.3/10.0.7 on the old base against 8.0.4/10.0.10 on dev). Same three files as before, byte for byte. |
In text format the ILogger category is written on every line when LambdaLoggerOptions.IncludeCategory is true (the default), but the JSON branch of LambdaILogger.Log never emitted the category, so migrating a function from text to JSON logging silently lost it with no way to opt back in (aws#2469). When IncludeCategory is set, prepend a {Category} placeholder to the message template and supply the category value, so it surfaces as a queryable property in the JSON record instead of being dropped. Text format is unchanged, and JSON output is unchanged when IncludeCategory is false. Fixes aws#2469
Copilot's review on this PR caught a real issue: prepending a {Category}
placeholder to honor IncludeCategory in JSON mode makes the formatter's
positional-argument detection treat the whole template as named, which
can shift how a caller's own {0}/{1} placeholders line up with their
values. Prepend the category as literal text instead, same as the text
format already does.
Updated the two category tests and added one covering a positional
template to confirm it stays untouched. All 17 tests pass locally on
net8.0 and net10.0.
The first commit normalized the whole file to CRLF where upstream has mixed endings, so the diff read 74 changed lines when only 11 lines actually change. Content is unchanged: git diff -w against the previous commit is empty.
776b763 to
73c7034
Compare
|
updated to master branch as base branch since #2542 was merged. rerunning the integ tests https://github.com/aws/aws-lambda-dotnet/actions/runs/32997374660 |
Fixes #2469
Problem
With
Amazon.Lambda.Logging.AspNetCore, theILoggercategory is written on every line in text format whenLambdaLoggerOptions.IncludeCategoryis true (which is the default). The JSON branch ofLambdaILogger.Log, however, only forwards the message template and its arguments and never emits the category, soIncludeCategoryis silently ignored in JSON mode. Migrating a function from text to JSON logging drops the category entirely, with no way to opt back in.Change
When
IncludeCategoryis set, the JSON branch now prepends a{Category}placeholder to the message template and supplies the category as the matching argument. Because the JSON formatter turns named template parameters into top-level properties, the category surfaces as a queryable property (and in the rendered message), matching the parity the text format already provides.IncludeCategory = false: unchanged (no extra property, same parameter count).IncludeCategory = true: the category is included.This follows the reporter's suggested "at minimum honor IncludeCategory" approach and needs no runtime-side change. If the maintainers would rather expose it as a dedicated top-level
categoryfield emitted byJsonLogMessageFormatter(the issue's preferred option), I'm happy to take that direction instead.Testing
Amazon.Lambda.Logging.AspNetCore.Testson net10.0:Passed! Failed: 0, Passed: 16. AddedTestJSONParameterLoggingIncludesCategoryWhenEnabled(category present, parameter count goes 3 to 4) andTestJSONParameterLoggingOmitsCategoryWhenDisabled(no category, count stays 3); the existingTestJSONParameterLoggingstill passes since it usesIncludeCategory = false.