-
Notifications
You must be signed in to change notification settings - Fork 463
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
Migrate to High Performance Logging for Enhanced Observability and Performance #1335
base: master
Are you sure you want to change the base?
Migrate to High Performance Logging for Enhanced Observability and Performance #1335
Conversation
92bcb48
to
7288490
Compare
Minor not but 3 of the reference links go to the same page and one returns a 404. Hopefully this PR was generated with an automated tool. It's a large amount of work! Overall this approach feels disconcerting to me. The amount of code in the delegates seems to be quadrupling what's required for a single log message. Maybe it is making logging more efficient but isn't the solution to do that in the logging frameworks/libraries and allow application code can stay concise, Additionally, IMHO instrumenation and analytics should be separate from logging. They serve different purposes and combiing them creates a maintenance burden, paricularly for simple debug log messages. |
-50% GitHub Copilot 😄 This PR is all about making logging efficient when not logging. There's a lot of heavy CPU and memory work in If someone has a good test bench for SIPSorcery, I would appreciate to test this and see the difference.
I don't understand what you mean. |
You list observability, structured & semantic logging, consistent schema and telemetry as benefits. Those make sense for a small number of the "high value" log events. The majority of log messages don't qualify. Incurring the additional overhead for 10% (guess) of the log messages that would be useful for telemetery etc. will introduce a significant burden for the 90% that don't. In other code bases I've worked on we've used a different framework for telemetery, analytics and not tried to double up the logging framework. |
OpenTelemetry covers logging, tracing and metrics. In OpenTelemery (https://opentelemetry.io/), structured & semantic logging is a first class citizen. But, in the end, it's just a common protocol that all players are using. For logging, it's not a lot more that using Serilog with a JSON sink. Apart the well-known communication protocol being supported by major players like Microsoft Azure Application Insights, AWS, DataDog, Honneycomb, a lots more, incluiding Serilog.Sinks.OpenTelemetry. Metrics and tracing (Activity) are also first citizens in .NET. But that will be a subject to another PR. 😄 Do you have any sample web (or "webifyable") project that I can setup with Aspire (https://learn.microsoft.com/en-us/dotnet/aspire/whats-new/dotnet-aspire-9.1) for you to see? |
Ok I've mixed myself up (partly due to the PR description listing a lot of things). The structured and semantic logging is fine and makes sense. Your previous logging PR made a lot of changes for that, The telemetery is not really a factor here since that will be part of the logging framework (or app insights etc). So it boils down exclusively to using logging delegates for performance. And same argument as originally I don't think the extra maintenance and less concise code justifies it.
I'm a Kubernetes man myself, it has equivalent tools to Aspire, but if it helps there's an ASP.NET WebRTC demo. |
I'm sad you fill that way. Kubernetes is a run-time hosting/deployment infrastructure. Aspire is a development environment. I'll make an Aspire demo. |
We're clutching at straws here but my point is Aspire is an SDK with the sole purpose of deploying apps to Azure which is where the k8s comparison comes from. |
That's not what Aspire is. |
7288490
to
c0e2d30
Compare
It is my understanding that the only difference between doing something like... |
And the allocation of the There's also pre-processing of the message template. There aren't "some performance" improvements in this PR. There are huge performance improvements in this PR because, besides the documented performance improvements of using high performance logging, all calculations needed just for logging are now guarded by a log level check.
|
cbd5085
to
78c0808
Compare
Added Aspire demo project.
You can play with this with the current implementation or with the NuGet of a version at your choice and check logs, .NET metrics, etc. |
78c0808
to
827cbc8
Compare
Migrate to High Performance Logging for Enhanced Observability and Performance
Overview
This PR proposes replacing traditional logging methods (
LogError
,LogWarning
, etc.) with High Performance Logging patterns to improve our application's observability, performance, and maintainability.Key Benefits
1. 🚀 Performance Improvements
2. 📊 Better Observability
3. 🔍 Enhanced Maintainability
4. 💡 Developer Experience
Technical Resources
For more information about High Performance Logging patterns and best practices:
Implementation Example
Performance Impact
Next Steps
Let me know if you need any clarification or have suggestions for improvement!