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

Add a layout renderer for correlation ID #328

Closed
Giorgi opened this issue Oct 31, 2018 · 21 comments · Fixed by #330
Closed

Add a layout renderer for correlation ID #328

Giorgi opened this issue Oct 31, 2018 · 21 comments · Fixed by #330
Labels

Comments

@Giorgi
Copy link
Contributor

Giorgi commented Oct 31, 2018

Type (choose one):

In case of a FEATURE REQUEST:

Should I send a PR ?

@snakefoot
Copy link
Contributor

There is already something called ${aspnet-traceidentifier} coming from HttpContext.TraceIdentifier:

https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.http.httpcontext.traceidentifier?view=aspnetcore-2.1

@Giorgi
Copy link
Contributor Author

Giorgi commented Oct 31, 2018

${aspnet-traceidentifier} is only in NLog.Web.AspNetCore not in NLog.Web

@Giorgi
Copy link
Contributor Author

Giorgi commented Oct 31, 2018

What if I implement ${aspnet-traceidentifier} so that it uses HttpRequestMessageExtensions.GetCorrelationId on full Framework?

@304NotModified
Copy link
Member

is this for full .net or .net core?

@snakefoot
Copy link
Contributor

Must admit my skills in ASP.NET MVC + Core are weak. So I have a hard time knowing how to lookup the different ids:

  • The unique requestid from the client
  • The unique traceid on the server when handling the request (connects all logging for one request)
  • The unique correlationid that connects the requestid and traceid together (connects all logging across multiple requests)

See also here aspnet/Hosting#1394

@Giorgi
Copy link
Contributor Author

Giorgi commented Oct 31, 2018

@304NotModified This is for full .net

@304NotModified
Copy link
Member

OK,

if we don't need more dependencies, then NLog package is OK for this.

If not, then we need a NLog.WebApi or something like that. I have to admit I don't have much time to setup a new package (and unfortunately it always takes more time then it should be)

@Giorgi
Copy link
Contributor Author

Giorgi commented Oct 31, 2018

@304NotModified so do you agree with implementing ${aspnet-traceidentifier} in full .Net using HttpRequestMessageExtensions.GetCorrelationId or do you prefer creating a new layout renderer?

@Giorgi
Copy link
Contributor Author

Giorgi commented Oct 31, 2018

@304NotModified Looks like we will need dependency on Microsoft.AspNet.WebApi.Core nuget package

@304NotModified
Copy link
Member

OK, then the layout renderer could be ${webapi-traceidentifier} ;)

@Giorgi
Copy link
Contributor Author

Giorgi commented Nov 1, 2018

Yes, but we will need a new project in the solution and new nuget package so that we don't introduce new dependency for NLog.web users

@snakefoot
Copy link
Contributor

snakefoot commented Nov 1, 2018

The specification of ´GetCorrelationId` :

Retrieves the Guid which has been assigned as the correlation ID associated with the given request. The value will be created and set the first time this method is called.

So it sounds like the traceidentifier. The source-code is this:

https://github.com/aspnet/AspNetWebStack/blob/master/src/System.Web.Http/HttpRequestMessageExtensions.cs#L759

Where it attempts 3 lookups:

  1. Lookup of HttpPropertyKeys.RequestCorrelationKey from HttpRequestMessage.Properties
  2. Fallback lookup of Trace.CorrelationManager.ActivityId (Unless equals Guid.Empty)
  3. Fallback lookup of Guid.NewGuid();

If fallback is required then the fallback Guid is stored using HttpPropertyKeys.RequestCorrelationKey.

Because there is no HttpRequestMessage (and building one from HttpContextBase will have a huge overhead and will not be the same, and will have empty Properties-dictionary). Then I suggest the following:

https://github.com/NLog/NLog/wiki/Trace-Activity-Id-Layout-Renderer

@Giorgi
Copy link
Contributor Author

Giorgi commented Nov 2, 2018

Yes, I currently use ${activityid} as a workaround to log correlation id

@Giorgi
Copy link
Contributor Author

Giorgi commented Nov 2, 2018

@304NotModified Are you ok with adding a new nuget package for this layout renderer or do you think using ${activityid} as a workaround is enough?

@snakefoot
Copy link
Contributor

@Giorgi I'm curious if GetCorrelationId works when used from HttpRequestBase. I have my doubts.

@Giorgi
Copy link
Contributor Author

Giorgi commented Nov 2, 2018

GetCorrelationId accepts an HttpRequestMessage object. How can it work for HttpRequestBase? I checked and in Asp.Net web api you can access HttpRequestMessage instance by calling HTTPContext.Current.Items["MS_HttpRequestMessage"] but we will need to copy code from GetCorrelationId to avoid dependency on Microsoft.AspNet.WebApi.Core nuget package

@snakefoot
Copy link
Contributor

snakefoot commented Nov 2, 2018

@Giorgi That lookup will only work if some other code has already injected HttpRequestMessage into the HttpRequestBase.Items-dictionary. Again not a ASP.NET wizard so maybe HttpRequestBase is always created together with an HttpRequestMessage-object (which are connected). I know Microsoft libraries loves to execute unnecessary code to help Intel sell more CPUs, but sometimes there are surprises.

@Giorgi
Copy link
Contributor Author

Giorgi commented Nov 3, 2018

@snakefoot So what do you suggest?

@snakefoot
Copy link
Contributor

snakefoot commented Nov 3, 2018 via email

@snakefoot
Copy link
Contributor

snakefoot commented Dec 2, 2018

@Giorgi Found this random code from the JsNLog-library:

        /// <summary>
        /// Creates an ID that is unique hopefully.
        /// 
        /// This method initially tries to use the request id that IIS already uses internally. This allows us to correlate across even more log files.
        /// If this fails, for example if this is not part of a web request, than it uses a random GUID.
        /// 
        /// See
        /// http://blog.tatham.oddie.com.au/2012/02/07/code-request-correlation-in-asp-net/
        /// </summary>
        /// <returns></returns>
        public override string IISRequestId()
        {
            var provider = (IServiceProvider)HttpContext.Current;
            if (provider != null)
            {
                var workerRequest = (HttpWorkerRequest)provider.GetService(typeof(HttpWorkerRequest));
                if (workerRequest != null)
                {
                    Guid requestIdGuid = workerRequest.RequestTraceIdentifier;
                    if (requestIdGuid != Guid.Empty)
                    {
                        return requestIdGuid.ToString();
                    }
                }
            }

            return null;
        }

Maybe this can be used to implement ${aspnet-traceidentifier} for System.Web ? (Requires IIS ETW (Event Tracing for Windows) feature enabled)

@snakefoot
Copy link
Contributor

@Giorgi Created #330 that might provide what you are looking for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants