-
Notifications
You must be signed in to change notification settings - Fork 208
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
Opt-in to TraceId and SpanId enrichment for ASP.NET Core 5.0 logs #207
Comments
Hi! Thanks for the note. It looks like these just became opt-in in .NET 5, and baked into the logger factory itself, to reduce fixed logging overheads. Serilog's ASP.NET Core/hosting integration will need a similar opt-in enricher, enabled via ASP.NET Core impl PR: dotnet/aspnetcore#11211 |
Thanks for the very informative reply @nblumhardt. |
@naingyelin we'll need to track this, so this ticket is useful, thanks 👍 |
Hi!
and then used it when configuring Serilog
However I wonder why Serilog does not get the values from the logging scope automatically. Is it because it is not using |
That's correct. |
Is anyone starting this? Otherwise I'll dump @techgeek03's code in a NuGet package called Serilog.Enrichers.Span. |
I rustled up a new NuGet package for this purpose: https://github.com/RehanSaeed/Serilog.Enrichers.Span |
Just an FYI @techgeek03's code changes work with 3.1 as well. I added a null check when assigning activity in the Enrich method to catch cases where Activity.Current wasn't set. Thanks for this, saved me hours of fiddling! :-) |
Added that check in the 1.0.1 release. |
Is setting this required? In your nuget package you don't show that step in the readme. Additionally, I can't get mine to compile:
Error:
|
@RehanSaeed I was missing
|
If both are .NET 5 (this means they default to W3C format of spans) and you use HttpClient to make the call to the API, then the parent ID should be set for you via HTTP headers (see Deep Dive into Open Telemetry for .NET for more details). |
Thanks. Read your article and have a question. But first, my issue was solved because that sample repo (and my code used same pattern) passed a My question is, how is Razor Page calls...
Log for 'Calling DataLocker Web API'
A log event in the API
So |
When making a HTTP request, parent ID is passed via the |
I understand that flow and had read Jimmy's article too, but it doesn't answer (or I missed it) where the Is my logging not outputting a property values that would have this id? I must be missing something, because as is, if I saw a log with a |
@terryaney Had the same question. Digging a bit I found this issue over on dotnet runtime. TL;DR; A new Activity in HttpClient is created before sending the HTTP request causing the ParentId to not match the SpanId of the prior activity. See dotnet/runtime#31862. |
@davidfowl Will this continue to be a problem in .NET 6 and onwards? (I assume so, but just wanted this confirmed). @nblumhardt We use Serilog quite extensively in our products and wonder what is the best option for supporting trace contexts moving forward, so that we do not need to continue maintaining workarounds. Is the goal to actually support I ask because if we are to invest time into fixing this, we wish to ensure that we are headed in the right direction. For the meantime, we will continue using workarounds. |
Ufff I just spent hours debugging why Thank you @RehanSaeed ❤️ for https://github.com/RehanSaeed/Serilog.Enrichers.Span |
@terryaney I am afraid we've sunset the use-case for Serilog. Sorry I can't help further. |
I've been investigating a "long term" answer to this, which will work in ASP.NET and elsewhere; serilog/serilog#1955 is the first cut at it. Eyes, thoughts, comments most welcome over there :-) |
Looks like Span support in Serilog has been merged which is awesome and means I can retire https://github.com/RehanSaeed/Serilog.Enrichers.Span. |
Hi everyone! As @RehanSaeed mentioned above, we've made some progress on this via the Serilog core package. Before it will be useful though, support needs to be added to sinks - could anyone still watching this thread please drop a note in below with the names of the sinks most important to them in the context of this change? Thanks! |
Seq, Event Viewer, File. Well done. |
Thanks @terryaney 🙌 |
Great! What about |
I would like these changes to apply to console, File and AWS cloud watch
sinks.
Thank you
…On Wed, 4 Oct, 2023, 4:08 am Nicholas Blumhardt, ***@***.***> wrote:
Hi everyone! As @RehanSaeed <https://github.com/RehanSaeed> mentioned
above, we've made some progress on this via the Serilog core package.
Before it will be useful though, support needs to be added to sinks -
could anyone still watching this thread please drop a note in below with
the names of the sinks most important to them in the context of this
change? Thanks!
—
Reply to this email directly, view it on GitHub
<#207 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADSMUJXDT4SB2WL6M7GYRGTX5SHVHAVCNFSM4PTT7VR2U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCNZUGU4DGNBZGIZQ>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Keeping a short log of instructions for sinks I've looked at. ConsoleUpdating Serilog to 3.1.0-* is enough for
|
@lindeberg parent ids are properties of spans, while Serilog only produces log events at this point. Serilog events can be correlated with spans via the trace and span id properties, but to record spans themselves (start, end, trace id, span id, parent id, span name, attributes) you'll need to turn to another solution. Definitely still some interesting terrain to explore with Serilog in this space, though! |
@nblumhardt Thanks for the explanation, that makes sense. Yeah, interesting to explore! Seems like a possible paradigm shift towards more traces (and trace events) and less logs as the tracing tools gets more accessible. |
Elasticsearch is the important for sink for my organization atm so that's what I vote for. :D |
Thanks for the feedback; contributions will be needed for Elasticsearch and AWS CloudWatch as I don't have either of these ready at hand (probably the case for a lot of sinks outside the few core ones in @lindeberg I think you need to add a direct dependency on Serilog.Extensions.Logging 7.0.0-dev-10353; I've updated that package now to 7.0.1-* to make this more obvious, a build should get to NuGet soon. |
I created an issue for Serilog.Sinks.ElasticSearch: serilog-contrib/serilog-sinks-elasticsearch#573 |
@nblumhardt I still can't get it to work |
@lindeberg I'll be able to dig in deeper later on, but what I think is happening here is the request logging middleware running right at the end of the pipeline, where the implicit activity has already been completed. To check out this theory, you can try adding a regular |
@nblumhardt Also can't get it to work with call to Serilog static Logger, Serilog ILogger or Microsoft ILogger in neither Controller or Minimal API. |
@lindeberg debugging now. First issue, Serilog's |
Okay, second issue is that Going back to investigate now, but in my working copy I see: Using: .WriteTo.Console(new ExpressionTemplate(
"[{@t:HH:mm:ss} {@l:u3}{#if @tr is not null} ({@tr}:{@sp}){#end}] {@m}\n{@x}")) |
Found the second issue - Serilog.Sinks.Console uses its own output template formatting in order to support themes. |
Created an issue for Serilog.Sinks.Dynatrace iojancode/Serilog.Sinks.Dynatrace#13 |
Hello, Serilog community. Firstly, thank you for the great library!
Description
I am trying out Asp.Net on Net 5 Preview 7 so I have created 2 web API projects one targeting [netcoreapp3.1] and another targeting [net5]
The problem is when I am looking at logs, I no longer see TraceId and SpanId for the API targeting [net5] with the DotNet 5 preview 7 SDK.
I also tested using the vanilla Logger and there was no issue there. Do I have to configure something, did I just miss something or is the DotNet 5 Preview SDK not fully supported yet?
Thanks for any info.
Reproduction
Below is my bootstrapping code, it is identical for both APIs,
Expected behavior
Be able to see TraceId and SpanId in my logs, regardless of the AspDotnet runtime version
Relevant package, tooling and runtime versions
Additional context
The text was updated successfully, but these errors were encountered: