Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support ValueTuple<string, object?> as state of scope. #232

Merged
merged 3 commits into from
Mar 14, 2024

Conversation

jimbojim1997
Copy link
Contributor

Change for issue #186.

When calling ILogger.BeginScope<TState>(TState state) TState can now be a ValueTuple<string, object?>, Item1 is used as the property name and Item2 is used as the property value.

using (_logger.BeginScope(("TransactionId", (object?)12345)) {
    _logger.LogInformation("Transaction completed in {DurationMs}ms...", 30);
}
// Example JSON output:
// {
//	"@t":"2020-10-29T19:05:56.4176816Z",
//	"@m":"Completed in 30ms...",
//	"@i":"51812baa",
//	"DurationMs":30,
//	"SourceContext":"SomeNamespace.SomeService",
//	"TransactionId": 12345
// }

@jimbojim1997 jimbojim1997 marked this pull request as ready for review November 12, 2023 23:26
@nblumhardt
Copy link
Member

Thanks @jimbojim1997! This looks good, even if the need for the second tuple member to be object? is a bit awkward, it's an improvement on the dictionary approach and we might open it up to arbitrary tuple types later on 👍

8.0.0 is going out in the next ~24 hours so it's too late for this change to make that release, unfortunately. We'll be able to ship it shortly afterwards, though.

@@ -0,0 +1,100 @@
// Copyright (c) .NET Foundation. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

Is the .NET Foundation copyright header intended here, and in the other test file below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know what it needs to be so just copied it from another file.

I've had another look and there's a mix of files with this header, Copyright [year] Serilog Contributors, and no header. What should I change this and the other file to?

Copy link
Member

Choose a reason for hiding this comment

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

Standard header as per serilog core without a year in all source files
Optional in test files
(I've yet to do the PR to make it completely consistent in core)
serilog/serilog#1969 (comment)

@nblumhardt
Copy link
Member

I think the checks for OriginalFormatPropertyName can be eliminated on all of the paths except IEnumerable<...>, since only a specifically-typed state object will carry these. Given I dropped the ball on this PR over Christmas though, I'll merge now to avoid any busywork around conflicts etc. :-)

Sorry about the long delay @jimbojim1997, thanks for this!

@nblumhardt nblumhardt merged commit 1e1479e into serilog:dev Mar 14, 2024
1 check passed
@sungam3r sungam3r mentioned this pull request Mar 25, 2024
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.

3 participants