-
Notifications
You must be signed in to change notification settings - Fork 796
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
Enable LogContext for .Net Core. #648
Changes from 11 commits
8edfa16
92c0ffa
e5a4d1b
6517350
ab37bfd
42dfa30
4e9c2a3
09afb78
f036e16
2fd3810
b3352df
c65dc5b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,9 +12,16 @@ | |
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
|
||
#if LOGCONTEXT | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry if this is a duplicate, I may have already asked - but are there any platforms on which There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not entirely clear on what 5.1 is. I thought from standard platform that it was 4.5, but if I try to compile with either There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, that's a shame. What do you think (+ @merbla @khellang) about using There are two benefits:
Worth a shot? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This a site @khellang pointed me to. Not always 100% however a great start.
These would indicate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @merbla thanks for the refs. The idea is not to use either of those, but something like: [ThreadStatic]
static ImmutableStack<ILogEventEnricher> Data; This should work on all platforms and does the job except for carrying state across async calls. |
||
using System; | ||
#if REMOTING | ||
using System.Runtime.Remoting.Messaging; | ||
#endif | ||
#if ASYNCLOCAL | ||
using System.Collections.Generic; | ||
using System.Threading; | ||
#endif | ||
using Serilog.Core; | ||
using Serilog.Core.Enrichers; | ||
using Serilog.Events; | ||
|
@@ -42,19 +49,24 @@ namespace Serilog.Context | |
/// </code> | ||
/// </example> | ||
/// <remarks>The scope of the context is the current logical thread, using | ||
/// <see cref="CallContext.LogicalGetData"/> (and so is | ||
/// preserved across async/await calls).</remarks> | ||
#if ASYNCLOCAL | ||
/// <seealso cref="AsyncLocal{T}"/> | ||
#else | ||
/// <seealso cref="CallContext.LogicalGetData"/> | ||
#endif | ||
/// (and so is preserved across async/await calls).</remarks> | ||
public static class LogContext | ||
{ | ||
#if ASYNCLOCAL | ||
static readonly AsyncLocal<ImmutableStack<ILogEventEnricher>> Data = new AsyncLocal<ImmutableStack<ILogEventEnricher>>(); | ||
#else | ||
#if DOTNET5_1 | ||
[ThreadStatic] | ||
static ImmutableStack<ILogEventEnricher> Data; | ||
#else | ||
static readonly string DataSlotName = typeof(LogContext).FullName; | ||
|
||
/// <summary> | ||
/// When calling into appdomains without Serilog loaded, e.g. via remoting or during unit testing, | ||
/// it may be necesary to set this value to true so that serialization exceptions are avoided. When possible, | ||
/// using the <see cref="Suspend"/> method in a using block around the call has a lower overhead and | ||
/// should be preferred. | ||
/// </summary> | ||
public static bool PermitCrossAppDomainCalls { get; set; } | ||
#endif | ||
#endif | ||
|
||
/// <summary> | ||
/// Push a property onto the context, returning an <see cref="IDisposable"/> | ||
|
@@ -132,33 +144,87 @@ static ImmutableStack<ILogEventEnricher> GetOrCreateEnricherStack() | |
return enrichers; | ||
} | ||
|
||
static ImmutableStack<ILogEventEnricher> Enrichers | ||
#if ASYNCLOCAL | ||
static ImmutableStack<ILogEventEnricher> Enrichers | ||
{ | ||
get | ||
{ | ||
return Data.Value; | ||
} | ||
set | ||
{ | ||
Data.Value = GetContext(value); | ||
} | ||
} | ||
#else | ||
|
||
#if DOTNET5_1 | ||
static ImmutableStack<ILogEventEnricher> Enrichers | ||
{ | ||
get | ||
{ | ||
return Data; | ||
} | ||
set | ||
{ | ||
Data = GetContext(value); | ||
} | ||
} | ||
|
||
#else | ||
static ImmutableStack<ILogEventEnricher> Enrichers | ||
{ | ||
get | ||
{ | ||
var data = CallContext.LogicalGetData(DataSlotName); | ||
|
||
ImmutableStack<ILogEventEnricher> context; | ||
#if REMOTING | ||
if (PermitCrossAppDomainCalls) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mean the property itself, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, sorry about the vague comment 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved |
||
{ | ||
context = ((Wrapper) data)?.Value; | ||
context = ((Wrapper)data)?.Value; | ||
} | ||
else | ||
{ | ||
context = (ImmutableStack<ILogEventEnricher>)data; | ||
} | ||
|
||
#else | ||
context = data; | ||
#endif | ||
return context; | ||
} | ||
set | ||
{ | ||
|
||
var context = !PermitCrossAppDomainCalls ? (object)value : new Wrapper { Value = value }; | ||
|
||
var context = GetContext(value); | ||
CallContext.LogicalSetData(DataSlotName, context); | ||
} | ||
} | ||
#endif | ||
#endif | ||
|
||
#if REMOTING | ||
/// <summary> | ||
/// When calling into appdomains without Serilog loaded, e.g. via remoting or during unit testing, | ||
/// it may be necesary to set this value to true so that serialization exceptions are avoided. When possible, | ||
/// using the <see cref="Suspend"/> method in a using block around the call has a lower overhead and | ||
/// should be preferred. | ||
/// </summary> | ||
public static bool PermitCrossAppDomainCalls { get; set; } | ||
|
||
static object GetContext(ImmutableStack<ILogEventEnricher> value) | ||
{ | ||
var context = !PermitCrossAppDomainCalls ? (object) value : new Wrapper | ||
{ | ||
Value = value | ||
}; | ||
return context; | ||
} | ||
#else | ||
static ImmutableStack<ILogEventEnricher> GetContext(ImmutableStack<ILogEventEnricher> value) | ||
{ | ||
return value; | ||
} | ||
#endif | ||
|
||
internal static void Enrich(LogEvent logEvent, ILogEventPropertyFactory propertyFactory) | ||
{ | ||
|
@@ -187,10 +253,13 @@ public void Dispose() | |
} | ||
} | ||
|
||
#if REMOTING | ||
sealed class Wrapper : MarshalByRefObject | ||
{ | ||
public ImmutableStack<ILogEventEnricher> Value { get; set; } | ||
} | ||
#endif | ||
} | ||
} | ||
|
||
#endif |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,22 +7,10 @@ | |
"licenseUrl": "http://www.apache.org/licenses/LICENSE-2.0", | ||
"iconUrl": "http://serilog.net/images/serilog-nuget.png", | ||
"frameworks": { | ||
"net40": { | ||
"compilationOptions": { | ||
"keyFile": "../../assets/Serilog.snk", | ||
"define": [ "APPSETTINGS", "LOGCONTEXT", "PROCESS", "FILE_IO", "PERIODIC_BATCHING" ] | ||
}, | ||
"frameworkAssemblies": { | ||
"System.Configuration": "" | ||
}, | ||
"dependencies": { | ||
"Microsoft.Bcl.Async": "1.0.168" | ||
} | ||
}, | ||
"net45": { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this change is inevitable - 👍 |
||
"compilationOptions": { | ||
"keyFile": "../../assets/Serilog.snk", | ||
"define": [ "APPSETTINGS", "LOGCONTEXT", "PROCESS", "FILE_IO", "PERIODIC_BATCHING" ] | ||
"define": [ "APPSETTINGS", "LOGCONTEXT", "PROCESS", "FILE_IO", "PERIODIC_BATCHING", "REMOTING" ] | ||
}, | ||
"frameworkAssemblies": { | ||
"System.Configuration": "" | ||
|
@@ -31,7 +19,7 @@ | |
"dotnet5.1": { | ||
"compilationOptions": { | ||
"keyFile": "../../assets/Serilog.snk", | ||
"define": [ "NO_APPDOMAIN" ] | ||
"define": [ "NO_APPDOMAIN", "LOGCONTEXT" ] | ||
}, | ||
"dependencies": { | ||
"Microsoft.CSharp": "4.0.1-beta-23516", | ||
|
@@ -42,6 +30,7 @@ | |
"System.Linq": "4.0.1-beta-23516", | ||
"System.Reflection.Extensions": "4.0.1-beta-23516", | ||
"System.Runtime.Extensions": "4.0.11-beta-23516", | ||
"System.Runtime.Serialization.Primitives": "4.1.0-beta-23516", | ||
"System.Text.RegularExpressions": "4.0.11-beta-23516", | ||
"System.Threading": "4.0.11-beta-23516", | ||
"System.Threading.Thread": "4.0.0-beta-23516" | ||
|
@@ -50,7 +39,8 @@ | |
"dotnet5.4": { | ||
"compilationOptions": { | ||
"keyFile": "../../assets/Serilog.snk", | ||
"define": [ "PROCESS", "FILE_IO", "PERIODIC_BATCHING", "NO_TIMER", "NO_APPDOMAIN" ] | ||
"define": [ "ASYNCLOCAL", "LOGCONTEXT", "PROCESS", "FILE_IO", "PERIODIC_BATCHING", "NO_TIMER", "NO_APPDOMAIN", "USERNAMEFROMENV" ] | ||
|
||
}, | ||
"dependencies": { | ||
"Microsoft.CSharp": "4.0.1-beta-23516", | ||
|
@@ -64,6 +54,7 @@ | |
"System.Linq": "4.0.1-beta-23516", | ||
"System.Reflection.Extensions": "4.0.1-beta-23516", | ||
"System.Runtime.Extensions": "4.0.11-beta-23516", | ||
"System.Runtime.Serialization.Primitives": "4.1.0-beta-23516", | ||
"System.Text.RegularExpressions": "4.0.11-beta-23516", | ||
"System.Threading": "4.0.11-beta-23516", | ||
"System.Threading.Thread": "4.0.0-beta-23516" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we now have dotnet5.1 TFM support, there's no need for
#if LOGCONTEXT
anymore - that's all the target platforms covered (I hope :-)) - they should all supportLogContext
now in one form or another.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha ha. You're right. I'll see how far I can get on my Mac tonight, otherwise it will be tomorrow morning. I've been meaning to see how the cross-platform bits are working.