#5 Add Orchard.Logging module adding NLog to pipeline #299
Conversation
jersiovic
force-pushed
the
#5_pluginARealLogger
branch
from
53664fb to
31df86e
Compare
|
Yup, check out https://github.com/NLog/NLog.Extensions.Logging/blob/master/src/NLog.Extensions.Logging/AspNetExtensions.cs Sent from my iPhone
|
|
Created an issue to talk about it first: #300 |
jersiovic
force-pushed
the
#5_pluginARealLogger
branch
from
31df86e to
972abcf
Compare
|
In the mean time I have moved NLog configuration to the Startup.cs of the Orchard.Cms.Web project for this PR. Maybe it is useful while #300 is designed. |
|
Yes, this is better. Following up on the issue. |
|
Is there a way with NLog to get one log file per tenant? |
|
I don't know an specific way for doing that. In orchard one I passed a parameter to Log4Net with the tenant name. |
|
Please, can you resolve the conflicts? |
|
Sorry I din't read you. ASAP I will do it |
jersiovic
force-pushed
the
#5_pluginARealLogger
branch
from
972abcf to
f9d19ca
Compare
|
Rebase done, you can merge it |
|
I tested it after retrieving your xkproject branch. I had multiple Warnings after the setup : I created a tenant and tried to generate an error (ex: change the site theme to 'sdf' and reload the homepage). Do you have examples of actions creating a log? Can we have more details about the stack trace, file, line number, ...? |
|
We need the tenant to be rendered, in O1 it's complex to do, how is it with NLog? |
|
Sorry I only updated this branch but I didn't know you want I render tenant. |
|
@agriffard said:
Do you mean an Nlog cofig file? Sth like that http://www.peteonsoftware.com/index.php/2008/07/29/nlog/ ?
For getting more info related with exceptions, stack trace, file and line you can use these renderers: |
|
Are we logging the RequestId? |
|
No we are not logging RequestID. Sth like that would be ok? https://github.com/nlog/nlog/wiki/Trace-Activity-Id-Layout-Renderer |
|
Correction: what I mentioned doen't work for Net.Core. The easiest way to get that is to use a custom layout renderer based on a lambda that prints TraceIdentifier: |
sebastienros
force-pushed
the
master
branch
2 times, most recently
from
37f0cda to
081a307
Compare
This reverts commit b1701fd.
jersiovic
force-pushed
the
#5_pluginARealLogger
branch
from
aa020e1 to
29f5891
Compare
|
Done. |
|
|
||
| public async Task Invoke(HttpContext httpContext) | ||
| { | ||
| var shellSetting = _runningShellTable.Match(httpContext); |
There was a problem hiding this comment.
ShellSettings is already added as a Feature to HttpContext
There was a problem hiding this comment.
Well but I assume you need a property for the logging ...
There was a problem hiding this comment.
ah ok. Let me check if I can access directly to it
There was a problem hiding this comment.
I've removed this class, this middleware is don't needed anymore.
src/Orchard.Cms.Web/Startup.cs
Outdated
| @@ -48,7 +48,9 @@ public void Configure(IApplicationBuilder app, IHostingEnvironment env, ILoggerF | |||
| loggerFactory.AddDebug(); | |||
| } | |||
|
|
|||
| app.UseModules(); | |||
There was a problem hiding this comment.
I explained it on next comment.
This is the implementation of UseModules()
public static IApplicationBuilder UseModules(this IApplicationBuilder app)
{
// Ensure the shell tenants are loaded when a request comes in
// and replaces the current service provider for the tenant's one.
app.UseMiddleware<ModularTenantContainerMiddleware>();
app.UseMiddleware<ModularTenantRouterMiddleware>();
return app;
}
Problem is I need the new middleware registered just after ModularTenantContainerMiddleware registration. So, I replaced UseModules() per its implementation with the new middleware registered in the position I need.
src/Orchard.Cms.Web/Startup.cs
Outdated
| @@ -48,7 +48,9 @@ public void Configure(IApplicationBuilder app, IHostingEnvironment env, ILoggerF | |||
| loggerFactory.AddDebug(); | |||
| } | |||
|
|
|||
| app.UseModules(); | |||
| app.UseMiddleware<ModularTenantContainerMiddleware>(); | |||
| app.UseMiddleware<StoreTenantNameAtHttpContextMiddleware>(); | |||
There was a problem hiding this comment.
I need the tenant name stored at httpcontext just after the tenant pipeline is initialized to log properly the tenant name on the first request after starting the server. That's why I need to replace app.UseModules() per what it does.
I'm not happy with the solution of not using app.UseModules() .
@sebastienros previously I set it within ModularTenantContainerMiddleware but you told me I should avoid to have logging code specific for a log library within OrchardCore. However thinking it twice to store tenant name in httpcontext for sharing it with other middlewares is not specific code for an specific log library. Don't you prefer the previous solution than this one?
|
Done. I've replaced the new middleware per an NLog layout renderer that directly uses ShelSettings in Httpcontext.Features. Now it looks good |
|
After last commit removing unneeded middleware class IMO this PR is ready to merge |
|
Looks very nice and clean. We could work on the list and order of things to log, but can be done later when people really care, details. |
| @@ -41,6 +40,9 @@ public void Configure(IApplicationBuilder app, IHostingEnvironment env, ILoggerF | |||
|
|
|||
| app.UseStaticFiles(); | |||
| loggerFactory.AddConsole(Configuration); | |||
There was a problem hiding this comment.
I assume that here we have some configuration for the ConsoleLogger which is different from the NLog one? OR is it possible to used only NLog even for the console? And default the Console to Error, and keep Warning in the log file.
There was a problem hiding this comment.
You are right NLog can be used also for console. I will change that on another PR
|
Talked to @sebastienros - I think this should be in a separate project inside of the OrchardCore folder... in here: https://github.com/OrchardCMS/Orchard2/tree/master/src/OrchardCore called This is not a module, just a separate package. move, TenantLayoutRenderer, and the setup there.... then in the Startup.cs file, do .UseOrchardNLog. If you want, I can do it for you? and do a PR to your PR? Still awesome stuff so far, just think we can reuse better. |
|
Changed my mind :) - Lets take this in, and implement the changes as the next step. Good work @jersiovic .... |
|
@jetski5832 thank you. Yes I see it is better for reusing it to move that to another project. I will change also this in another PR |
Issue #5 related to this topic was closed but at the end I didn't find lines in Orchard 2 implementing the solution provided there. So, here is PR doing what there is described.