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

Merged
merged 12 commits into from Feb 16, 2016

Conversation

Projects
None yet
5 participants
@colin-young
Contributor

colin-young commented Jan 26, 2016

For issue #588

Removed .Net 4.0 (hoping for #635 )
Changed from .Net 4.5.1 to 4.5.2, but left 4.5 as-is
Added ASYNCLOCAL and REMOTING feature pivots
Removed remoting support from .Net Core
Replaced CallContext with AsyncLocal when ASYNCLOCAL feature is available
I have not tested in a remoting situation, and have not reviewed the unit tests to see how they are covering that. All unit tests do pass (except maybe DoesNotPreventCrossDomainCalls which I haven't run yet).

Looking for some early feedback.

Thanks.

@khellang

This comment has been minimized.

Show comment
Hide comment
@khellang

khellang Jan 26, 2016

Member

Just skimming over this, it feels a bit #ifdef heavy for my taste. Are there any cases where it might be worth #ifdefing a whole class instead?

Member

khellang commented Jan 26, 2016

Just skimming over this, it feels a bit #ifdef heavy for my taste. Are there any cases where it might be worth #ifdefing a whole class instead?

@colin-young

This comment has been minimized.

Show comment
Hide comment
@colin-young

colin-young Jan 26, 2016

Contributor

That was my feeling with the #ifdefs, but I wasn't sure what direction you might want to go with serilog. There's also the issue with remoting not being supported in Core, but needing AsyncLocal. And .Net 4.5 doesn't support AsyncLocal. The other option would be to use AsyncLocal only for Core, and then REMOTING would be equivalent to ASYNCLOCAL, but not a strictly accurate description of the capability/feature.

My personal preference between those 2 options would be the extra #ifdef statements.

Contributor

colin-young commented Jan 26, 2016

That was my feeling with the #ifdefs, but I wasn't sure what direction you might want to go with serilog. There's also the issue with remoting not being supported in Core, but needing AsyncLocal. And .Net 4.5 doesn't support AsyncLocal. The other option would be to use AsyncLocal only for Core, and then REMOTING would be equivalent to ASYNCLOCAL, but not a strictly accurate description of the capability/feature.

My personal preference between those 2 options would be the extra #ifdef statements.

@colin-young

This comment has been minimized.

Show comment
Hide comment
@colin-young

colin-young Jan 27, 2016

Contributor

I rearranged the #ifdefs a little and got a slight reduction. IMHO it makes it a little more readable, but there are still more than I'd consider ideal.

Contributor

colin-young commented Jan 27, 2016

I rearranged the #ifdefs a little and got a slight reduction. IMHO it makes it a little more readable, but there are still more than I'd consider ideal.

var data = CallContext.LogicalGetData(DataSlotName);
ImmutableStack<ILogEventEnricher> context;
#if REMOTING
if (PermitCrossAppDomainCalls)

This comment has been minimized.

@nblumhardt

nblumhardt Jan 27, 2016

Member

PermitCrossAppDomainCalls should be #if REMOTING since AppDomain doesn't appear on the modern targets.

@nblumhardt

nblumhardt Jan 27, 2016

Member

PermitCrossAppDomainCalls should be #if REMOTING since AppDomain doesn't appear on the modern targets.

This comment has been minimized.

@colin-young

colin-young Jan 28, 2016

Contributor

You mean the property itself, right?

@colin-young

colin-young Jan 28, 2016

Contributor

You mean the property itself, right?

This comment has been minimized.

@nblumhardt

nblumhardt Jan 28, 2016

Member

Yes, sorry about the vague comment 👍

@nblumhardt

nblumhardt Jan 28, 2016

Member

Yes, sorry about the vague comment 👍

This comment has been minimized.

@colin-young

colin-young Jan 29, 2016

Contributor

Moved PermitCrossAppDomainCalls property to inside the REMOTING block at the bottom of the file.

@colin-young

colin-young Jan 29, 2016

Contributor

Moved PermitCrossAppDomainCalls property to inside the REMOTING block at the bottom of the file.

@@ -7,22 +7,10 @@
"licenseUrl": "http://www.apache.org/licenses/LICENSE-2.0",
"iconUrl": "http://serilog.net/images/serilog-nuget.png",
"frameworks": {
"net40": {

This comment has been minimized.

@nblumhardt

nblumhardt Jan 27, 2016

Member

I think this change is inevitable - 👍

@nblumhardt

nblumhardt Jan 27, 2016

Member

I think this change is inevitable - 👍

@colin-young

This comment has been minimized.

Show comment
Hide comment
@colin-young

colin-young Feb 8, 2016

Contributor

Any comments on the updated PR incorporating all (I hope) previous comments?

Contributor

colin-young commented Feb 8, 2016

Any comments on the updated PR incorporating all (I hope) previous comments?

@nblumhardt

This comment has been minimized.

Show comment
Hide comment
@nblumhardt

nblumhardt Feb 9, 2016

Member

Thanks Colin - looking good. Did you have a chance to consider:

[ThreadStatic]
static ImmutableStack<ILogEventEnricher> Data;

for .NET 5.1 support? It'd be great to cover all platforms with this and call it "done done" :-)

(I also think extracting some "policy" classes would be a win here, given the amount of conditional compilation required, but probably better to move this forward and do some refactoring down the line.)

(Let me know if you're out of time, great effort on everything so far!)

Member

nblumhardt commented Feb 9, 2016

Thanks Colin - looking good. Did you have a chance to consider:

[ThreadStatic]
static ImmutableStack<ILogEventEnricher> Data;

for .NET 5.1 support? It'd be great to cover all platforms with this and call it "done done" :-)

(I also think extracting some "policy" classes would be a win here, given the amount of conditional compilation required, but probably better to move this forward and do some refactoring down the line.)

(Let me know if you're out of time, great effort on everything so far!)

@colin-young

This comment has been minimized.

Show comment
Hide comment
@colin-young

colin-young Feb 9, 2016

Contributor

I did not. I'll take a look at that tomorrow. Fortunately I'm in the middle of wrestling with some versioning issues with my libraries (a.b.c-beta-9 is apparently newer than a.b.c-beta-10 and the build tools absolutely refuse to forget it ever existed even if I've removed it from my NuGet server and cleared the cache), so I'll be running a bunch of builds tomorrow. Nobody will notice a few extra changes... plus it's kind of related to what I'm supposed to be doing anyway.

I have to admit though, I have no clue what .NET 5.1 is.

Contributor

colin-young commented Feb 9, 2016

I did not. I'll take a look at that tomorrow. Fortunately I'm in the middle of wrestling with some versioning issues with my libraries (a.b.c-beta-9 is apparently newer than a.b.c-beta-10 and the build tools absolutely refuse to forget it ever existed even if I've removed it from my NuGet server and cleared the cache), so I'll be running a bunch of builds tomorrow. Nobody will notice a few extra changes... plus it's kind of related to what I'm supposed to be doing anyway.

I have to admit though, I have no clue what .NET 5.1 is.

@IanYates

This comment has been minimized.

Show comment
Hide comment
@IanYates

IanYates Feb 9, 2016

Contributor

@nblumhardt might have meant 4.5.1 perhaps?

Contributor

IanYates commented Feb 9, 2016

@nblumhardt might have meant 4.5.1 perhaps?

@colin-young

This comment has been minimized.

Show comment
Hide comment
@colin-young

colin-young Feb 9, 2016

Contributor

No, there is a framework target of dotnet5.1.

On Mon, Feb 8, 2016, 20:07 Ian Yates notifications@github.com wrote:

@nblumhardt https://github.com/nblumhardt might have meant 4.5.1
perhaps?


Reply to this email directly or view it on GitHub
#648 (comment).

Contributor

colin-young commented Feb 9, 2016

No, there is a framework target of dotnet5.1.

On Mon, Feb 8, 2016, 20:07 Ian Yates notifications@github.com wrote:

@nblumhardt https://github.com/nblumhardt might have meant 4.5.1
perhaps?


Reply to this email directly or view it on GitHub
#648 (comment).

@IanYates

This comment has been minimized.

Show comment
Hide comment
@IanYates
Contributor

IanYates commented Feb 9, 2016

@khellang

This comment has been minimized.

Show comment
Hide comment
@khellang

khellang Feb 9, 2016

Member

Yep. dotnet5.1 is the equivalent of netstandard1.0 in this document. dotnet5.x was the "working TFM" before it became netstandard1.x. It's basically the lowest API set possible that we can support, which means we pretty much target anything. The reason net451 is still there is because some clients don't yet support the new TFMs. By the time we ship 2.0, we'll probably move to netstandard1.0 only.

Member

khellang commented Feb 9, 2016

Yep. dotnet5.1 is the equivalent of netstandard1.0 in this document. dotnet5.x was the "working TFM" before it became netstandard1.x. It's basically the lowest API set possible that we can support, which means we pretty much target anything. The reason net451 is still there is because some clients don't yet support the new TFMs. By the time we ship 2.0, we'll probably move to netstandard1.0 only.

@khellang

This comment has been minimized.

Show comment
Hide comment
@khellang

khellang Feb 9, 2016

Member

BTW, we should look at moving to a newer netstandard than 1.0 as well. I'm not sure how much sense it makes to support those older frameworks. It's very limiting in terms of API surface.

Member

khellang commented Feb 9, 2016

BTW, we should look at moving to a newer netstandard than 1.0 as well. I'm not sure how much sense it makes to support those older frameworks. It's very limiting in terms of API surface.

@colin-young

This comment has been minimized.

Show comment
Hide comment
@colin-young

colin-young Feb 9, 2016

Contributor

@khellang the bit that doesn't make sense to me is that my understanding from that page is 1.0 is 4.5, which is supposed to have AsyncLocal, but that's not what the compiler tells me. It's entirely possible that I'm misinterpreting that page.

Contributor

colin-young commented Feb 9, 2016

@khellang the bit that doesn't make sense to me is that my understanding from that page is 1.0 is 4.5, which is supposed to have AsyncLocal, but that's not what the compiler tells me. It's entirely possible that I'm misinterpreting that page.

@khellang

This comment has been minimized.

Show comment
Hide comment
@khellang

khellang Feb 9, 2016

Member

1.0 is not 4.5, 1.1 is 4.5. And even though .NET 4.5 might have it, other platforms might not. netstandard is basically the minimum API set available on all the supported platforms. As you can see, it also supports Windows Phone 8.0, which might or might not have that API.

Member

khellang commented Feb 9, 2016

1.0 is not 4.5, 1.1 is 4.5. And even though .NET 4.5 might have it, other platforms might not. netstandard is basically the minimum API set available on all the supported platforms. As you can see, it also supports Windows Phone 8.0, which might or might not have that API.

@colin-young

This comment has been minimized.

Show comment
Hide comment
@colin-young

colin-young Feb 9, 2016

Contributor

Whoops. I keep getting dotnet5.1 mixed up with netstandard1.1 when it's really netstandard1.0.

So it looks like we can't just use dotnet5.{1|2} because of Windows Phone 8.1 (and maybe Windows 8.1) not supporting AsyncLocal or CallContext (things are starting to make a little more, well, sense is a bit strong, but at least there is a sort of twisted logic to it now). I'm not entirely sure if the separate net452 framework specifier would take precedence over the dotnet5.n specifier if running on the full .Net 4.5 framework. Plus it's just plain confusing to have overlapping specifications IMHO.

This is the most useful thing I've found to translate. I get that naming is hard, but it would be nice if all the names were collected in one place to make it easier to translate.

Contributor

colin-young commented Feb 9, 2016

Whoops. I keep getting dotnet5.1 mixed up with netstandard1.1 when it's really netstandard1.0.

So it looks like we can't just use dotnet5.{1|2} because of Windows Phone 8.1 (and maybe Windows 8.1) not supporting AsyncLocal or CallContext (things are starting to make a little more, well, sense is a bit strong, but at least there is a sort of twisted logic to it now). I'm not entirely sure if the separate net452 framework specifier would take precedence over the dotnet5.n specifier if running on the full .Net 4.5 framework. Plus it's just plain confusing to have overlapping specifications IMHO.

This is the most useful thing I've found to translate. I get that naming is hard, but it would be nice if all the names were collected in one place to make it easier to translate.

@khellang

This comment has been minimized.

Show comment
Hide comment
@khellang

khellang Feb 9, 2016

When changing to dotnet5.3, you need to change this to DOTNET5_3 as well 😄

khellang commented on src/Serilog/Context/LogContext.cs in 2fd3810 Feb 9, 2016

When changing to dotnet5.3, you need to change this to DOTNET5_3 as well 😄

@colin-young

This comment has been minimized.

Show comment
Hide comment
@colin-young

colin-young Feb 9, 2016

Contributor

Yeah, that was an premature commit. I just fixed it so they are all at 5.1.

The #ifdefs have become a bit uglier.

Contributor

colin-young commented Feb 9, 2016

Yeah, that was an premature commit. I just fixed it so they are all at 5.1.

The #ifdefs have become a bit uglier.

@colin-young

This comment has been minimized.

Show comment
Hide comment
@colin-young

colin-young Feb 10, 2016

Contributor

I'll check out the full build and tests tomorrow. A bit limited on OS X since I could only check dotnet5.4 framework.

UPDATE: ran full build and tests on Windows. Looks like I didn't mess it up.

Contributor

colin-young commented Feb 10, 2016

I'll check out the full build and tests tomorrow. A bit limited on OS X since I could only check dotnet5.4 framework.

UPDATE: ran full build and tests on Windows. Looks like I didn't mess it up.

@nblumhardt

This comment has been minimized.

Show comment
Hide comment
@nblumhardt

nblumhardt Feb 16, 2016

Member

Legendary; thanks @colin-young !

Member

nblumhardt commented Feb 16, 2016

Legendary; thanks @colin-young !

nblumhardt added a commit that referenced this pull request Feb 16, 2016

@nblumhardt nblumhardt merged commit 505f6c8 into serilog:dev Feb 16, 2016

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@colin-young colin-young deleted the colin-young:logcontext-588 branch Feb 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment