-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Use implicit logging scope for Activity #22376
Conversation
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.
There are conflicts in the test that need resolving.
694e14d
to
f809e23
Compare
540220e
to
9c40ed7
Compare
3fdc3f6
to
eab10a2
Compare
Still waiting on #22387 before the new API from runtime is available |
I don’t think I should revert anything. Just make a new change. |
I'm going to squash before merging, so it won't look like a reverted commit when merged. I just found it easier to work this way. |
just wondering do we have any tests covering this functionality in asp.net? I just wanted to ensure we are not introducing any regression there. |
d1d6edb
to
4cc8190
Compare
We did have some tests that I've removed in the PR. It isn't functionally the same behavior. In place of the 1 scope object that had both Activity and RequestId/RequestPath. Ideally I wouldn't like to test runtime functionality in this repo. |
@Pilchie Does this need tactics approval? Also, who needs to merge since I don't have access to this branch? |
At least did you validate the behavior manually for asp.net? |
Why would it need tactics approval? |
We already branched for preview6 |
@tarekgh Manually verified with console logger. Is there any other logger provider you suggest I verify against given the fact that it's now two different scope objects? New log message
Old log message
|
App insights. I wonder if this should instead go into the host builder’s create default builder |
I just realized I also need to make the change in here (since we don't use aspnetcore/src/DefaultBuilder/src/WebHost.cs Line 209 in 5f7e1a0
|
@cijothomas Would this change impact AppInsights? Activity information is no longer present on the |
Don't think so. ApplicationInsights does not directly rely on the activity information from logger. (To correlate with parent request, Activity.Current is used) |
The AppInsights logger should be using the scope provider so I should get this for free. It would be good to test it anyways |
src/DefaultBuilder/src/WebHost.cs
Outdated
@@ -215,6 +221,15 @@ internal static void ConfigureWebDefaults(IWebHostBuilder builder) | |||
StaticWebAssetsLoader.UseStaticWebAssets(ctx.HostingEnvironment, ctx.Configuration); | |||
} | |||
}); | |||
builder.ConfigureLogging(loggingBuilder => |
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.
I don't think this should be here. This should go into the generic host.
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.
This gets called by the generic host. I verified that when doing #22528.
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.
I thought we agreed to only to enable it for webapps?
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.
I think we should turn it on by default for all generic hosted apps. That means workers get it for free as well.
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.
So remove the ConfigureWebDefaults
changes (make changes to generic host in the runtime repo) and keep the WebHostBuilder
changes and get this merged?
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.
I think we should turn it on by default for all generic hosted apps. That means workers get it for free as well.
Can we clarify the scope of this? I mean how common the apps are using default host against using none-defaulted hosts? I am asking because originally we decided to have the behavior as opt-in to be intentional paying the extra cost when only opting-in .
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.
Worker service and ASP.NET Core core applications.
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.
Created a new PR in runtime for this dotnet/runtime#37892
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.
do we still need to have this PR?
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.
In theory this PR prevents a regression for existing WebHost customers. Keep in mind we plan to obsolete WebHost in 5.0, but not remove it yet.
This reverts commit f7a2d3c.
Addresses #21020