Skip to content

[Telemetry 1/4] Add telemetry hooks to Neo core#4372

Open
Jim8y wants to merge 2 commits into
telemetryfrom
telemetry-1
Open

[Telemetry 1/4] Add telemetry hooks to Neo core#4372
Jim8y wants to merge 2 commits into
telemetryfrom
telemetry-1

Conversation

@Jim8y

@Jim8y Jim8y commented Dec 12, 2025

Copy link
Copy Markdown
Contributor

Summary

This PR adds static event hooks to the Neo core P2P layer to enable telemetry collection without modifying the core behavior.

Changes

  • Add MessageSent event to RemoteNode for tracking outgoing messages
  • Add ConnectionChanged event for monitoring peer connection lifecycle
  • Wrap event invocations in try-catch to prevent telemetry failures from crashing the node

Files Changed

  • src/Neo/Network/P2P/RemoteNode.cs

Test Plan

  • Build passes
  • Existing unit tests pass
  • Event handlers with exceptions don't crash the node

Part of Telemetry Feature

This is PR 1 of 4 for adding comprehensive telemetry support to Neo nodes:

  1. [This PR] Core hooks - Add event hooks to Neo core
  2. Plugin + Collectors - Main telemetry plugin and metric collectors
  3. Health endpoints - Kubernetes-compatible health check endpoints
  4. Documentation - README and monitoring configurations

Related to #4376

@Jim8y

Jim8y commented Dec 12, 2025

Copy link
Copy Markdown
Contributor Author

As is suggested and requested from @shargon and @vncoelho , telemetry is split into 4 prs. These prs will not be merged into master directly, but will go to telemetry first.

@shargon shargon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's ready to review?

Comment thread src/Neo/Network/P2P/RemoteNode.cs Outdated
Comment thread src/Neo/Neo.csproj Outdated
@Jim8y Jim8y marked this pull request as ready for review December 24, 2025 01:42

@shargon shargon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

localNode.RemoteNodes.TryAdd(Self, this);
var listenPort = config.Tcp?.Port;
_connectionDirection = listenPort.HasValue && local.Port == listenPort.Value ? ConnectionDirection.Inbound : ConnectionDirection.Outbound;
NotifyConnectionChanged(true, ConnectionChangeReason.Connected);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Send in StartProtocol message?

Comment thread src/Neo/Plugins/Plugin.cs
/// But it also depends on <see cref="UnhandledExceptionPolicy"/>.
/// </summary>
internal bool IsStopped { get; set; }
protected internal bool IsStopped { get; set; }

@erikzhang erikzhang Dec 30, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The getter can be public, but the setter should be internal or private.

_ack = false;
// Here it is possible that we dont have the Version message yet,
// so we need to send the message uncompressed
SafeInvoke(() => MessageSent?.Invoke(_system, message));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why SafeInvoke? I think exceptions should be handled correctly.

@erikzhang erikzhang left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some conversations are still unresolved. Please do not merge it yet.

@Wi1l-B0t

Wi1l-B0t commented Feb 6, 2026

Copy link
Copy Markdown
Contributor

No Progress? @Jim8y

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Downstream Needed for downstream repos Waiting for Review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants