Skip to content

add log #3881

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

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from
Draft

add log #3881

wants to merge 12 commits into from

Conversation

Jim8y
Copy link
Contributor

@Jim8y Jim8y commented Apr 4, 2025

Description

This PR integrates Serilog as a structured logging system for the Neo blockchain. The implementation provides enhanced logging capabilities with improved readability, flexibility, and diagnostic capabilities over the current logging system.

Key features implemented:

  • Structured logging using Serilog, enabling more detailed and queryable log output
  • Configurable logging levels (Verbose, Debug, Information, Warning, Error, Fatal)
  • Multiple sink support (Console, File, rolling files with retention policies)
  • JSON formatting option for machine-readable logs
  • Performance optimizations via log level filtering at source
  • Log enrichment with contextual properties (node ID, peer information, network details)

This change enhances observability of the node's behavior during normal operation and significantly improves diagnostic capabilities during troubleshooting sessions.

Type of change

  • New feature (non-breaking change which adds functionality)
  • Optimization (the change is only an optimization)
  • Style (the change is only a code style for better maintenance or standard purpose)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • Unit tests covering Serilog configuration and appender functionality
  • Integration tests verifying logs are properly emitted during various node operations
  • Performance testing to ensure logging doesn't impact node performance
  • Manual testing with different configuration settings
  • Stress testing with high-volume log generation

Test Configuration:

  • Tested with default configuration (console and file sinks)
  • Tested with JSON formatting for machine parsing
  • Tested with various log levels to verify filtering
  • Tested log rotation and retention policies
  • Tested in both development and production environments
  • Verified compatibility with log aggregation tools (e.g., ELK stack)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Copy link
Contributor

@Wi1l-B0t Wi1l-B0t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great PR.
Many changes, so need some time to review.

@Jim8y Jim8y marked this pull request as draft April 5, 2025 14:30
@Jim8y
Copy link
Contributor Author

Jim8y commented Apr 8, 2025

image image

/// </summary>
/// <param name="logger">The Serilog logger instance</param>
/// <returns>A collection of sink objects</returns>
public static IEnumerable<object> GetSinks(this ILogger logger)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks weird.
Why get some prive members?

@@ -56,14 +60,23 @@ internal class Close { public bool Abort; }
/// <param name="local">The address of the local node.</param>
protected Connection(object connection, IPEndPoint remote, IPEndPoint local)
{
// Initialize logger in constructor since we need the dynamic values
_log = Log.ForContext(GetType()).ForContext("Remote", remote).ForContext("Local", local);
_log?.Debug("Connection actor created");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why so many _log?, and why not _log directly?
If the log is not needed, just set it to a noop log?

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

Successfully merging this pull request may close these issues.

3 participants