-
Notifications
You must be signed in to change notification settings - Fork 513
Nethermind UI (initial) #8109
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
Nethermind UI (initial) #8109
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to move from Nethermind.Runner to a plugin
@@ -63,6 +63,7 @@ public sealed class BlockchainProcessor : IBlockchainProcessor, IBlockProcessing | |||
private readonly Stopwatch _stopwatch = new(); | |||
|
|||
public event EventHandler<IBlockchainProcessor.InvalidBlockEventArgs>? InvalidBlock; | |||
public event EventHandler<BlockStatistics>? NewProcessingStatistics; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just pass through add/remove for simplicity?
@@ -41,3 +61,114 @@ public static void EnableConsoleColorOutput() | |||
[DllImport("kernel32.dll")] | |||
private static extern bool SetConsoleMode(IntPtr hConsoleHandle, uint dwMode); | |||
} | |||
|
|||
|
|||
public sealed class LineInterceptingTextWriter : TextWriter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need that? Doesn't NLog have something that would make it simpler for us?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really; that I could find :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart of my comments I'm fine with rest from C#.
Someone needs to review TypeScript, @rubo ?
@@ -76,6 +76,7 @@ public async Task Start(CancellationToken cancellationToken) | |||
s.AddSingleton(_jsonRpcUrlCollection); | |||
s.AddSingleton(_webSocketsManager); | |||
s.AddSingleton(_rpcAuthentication); | |||
s.AddSingleton(_api); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this, we want to remove/hide api not inject it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to inject them individually
await ctx.Response.WriteAsync($"event: nodeData\n", cancellationToken: ct); | ||
await ctx.Response.WriteAsync($"data: ", cancellationToken: ct); | ||
await ctx.Response.Body.WriteAsync(GetNodeData(), cancellationToken: ct); | ||
await ctx.Response.WriteAsync($"\n\n", cancellationToken: ct); | ||
|
||
await ctx.Response.WriteAsync($"event: txNodes\n", cancellationToken: ct); | ||
await ctx.Response.WriteAsync($"data: ", cancellationToken: ct); | ||
await ctx.Response.Body.WriteAsync(TxPoolFlow.NodeJson, cancellationToken: ct); | ||
await ctx.Response.WriteAsync($"\n\n", cancellationToken: ct); | ||
|
||
await ctx.Response.WriteAsync($"event: log\n", cancellationToken: ct); | ||
await ctx.Response.WriteAsync($"data: ", cancellationToken: ct); | ||
await ctx.Response.Body.WriteAsync(JsonSerializer.SerializeToUtf8Bytes(ConsoleHelpers.GetRecentMessages(), JsonSerializerOptions.Web), cancellationToken: ct); | ||
await ctx.Response.WriteAsync($"\n\n", cancellationToken: ct); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better to do that in 1 WriteAsync?
await ctx.Response.WriteAsync($"event: {entry.Type}\n", cancellationToken: ct); | ||
await ctx.Response.WriteAsync($"data: ", cancellationToken: ct); | ||
await ctx.Response.Body.WriteAsync(entry.Data, cancellationToken: ct); | ||
await ctx.Response.WriteAsync($"\n\n", cancellationToken: ct); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
Metrics.TransactionsSourcedPrivateOrderFlow += notInMempoool; | ||
Metrics.TransactionsSourcedMemPool += transactionsInBlock - notInMempoool; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With DarkPoolRatio, do we need those 2 metrics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although the UI concept is cool, I have some concerns about the approach. I'd prefer this to be a separate component instead of being incorporated into the Nethermind Runner
. Please see the comments.
<Target Name="JsBuild" BeforeTargets="PreBuildEvent"> | ||
<Exec Command="yarn build:dev" Condition="'$(Configuration)'=='Debug'" /> | ||
<Exec Command="yarn build:release" Condition="'$(Configuration)'=='Release'" /> | ||
</Target> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is highly undesirable to make yarn, hence node.js, a required tool to build Nethermind for a non-essential and optional component as the UI. The build failed for me just because I didn't have yarn.
A few suggestions:
- Do not use node.js/yarn at all
- Provide already minified versions (do we really need it, given its local usage only?)
- Having the UI as a separate NuGet package, like the health checks. I think this is the best approach and similar to the plugin approach suggested by @LukaszRozmej. I can help with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use node.js/yarn at all
Removed yarn; still uses node because need that to compile typescript, even with the dotnet build process
Having the UI as a separate NuGet package, like the health checks.
Would be annoying af to develop as the UI is directly dependent on the data the Runner is providing and will be evolving over time. Health checks sit on an agreed upon standardised api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node is only used in docker to build; its not included in the final container
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that maintaining a NuGet package is somewhat annoying, but bringing the Node.js as a new build dependency for some optional components is also annoying. I believe not everyone in the team uses or even has Node.js & stuff, let alone third parties that build from source. Moreover, with a NuGet package, you'll have more liberties regarding the organization and approaches, as there will be much less nitpicking.
Of course, this is just my opinion, but I think I'm not alone in that. I'd like more opinions here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have all these fonts in the repo instead of downloading them from HTML?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Web site has 0 requirement for an internet connection (just to your node) and doesn't send beacon requests to google
Maybe you want to put it on a big TV, but don't want to give that TV full access to the Internet just to download some fonts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, but on the other hand, binary stuff is not git-friendly. If it was a separate package, it would be much less of a concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to put this in the logo
directory with the other images and rename it to images
. So, all the graphics are in the same place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a special image and different dimensions; logos is 3rd party chain logos and all the same dimensions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still, images can go into their separate directory, and there you can organize them however you wish. As mentioned above, if it was a separate package, I wouldn't care.
18b3229
to
8f5ffe2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments on the TS side of things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably need to include some LICENCE file for these fonts.
@@ -0,0 +1,272 @@ | |||
/* latin-ext */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably want to put this file through some code formatter.
"ansi-to-html": "0.7.2", | ||
"d3": "7.9.0", | ||
"d3-sankey": "0.12.3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are listing these dependencies but we are also vendoring them (lib/*.js
). Why is that?
// Prepare day, month, year, hours, minutes, seconds | ||
const day = String(date.getDate()).padStart(2, '0'); | ||
const month = months[date.getMonth()]; | ||
const year = date.getFullYear(); | ||
const hours = String(date.getHours()).padStart(2, '0'); | ||
const minutes = String(date.getMinutes()).padStart(2, '0'); | ||
const seconds = String(date.getSeconds()).padStart(2, '0'); | ||
|
||
// Return the formatted date string | ||
// Example: "09 Feb 2025 13:45:00" | ||
return `${day} ${month} ${year} ${hours}:${minutes}:${seconds}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Prepare day, month, year, hours, minutes, seconds | |
const day = String(date.getDate()).padStart(2, '0'); | |
const month = months[date.getMonth()]; | |
const year = date.getFullYear(); | |
const hours = String(date.getHours()).padStart(2, '0'); | |
const minutes = String(date.getMinutes()).padStart(2, '0'); | |
const seconds = String(date.getSeconds()).padStart(2, '0'); | |
// Return the formatted date string | |
// Example: "09 Feb 2025 13:45:00" | |
return `${day} ${month} ${year} ${hours}:${minutes}:${seconds}`; | |
return date.toLocaleString(navigator.language, { | |
dateStyle: 'long', | |
timeStyle: 'medium', | |
hour12: false | |
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want consistency regarding language we can force the locale to be en-UK
instead of navigator-language
. The date then looks like 10 March 2025 at 16:25:57
in all devices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
en-uk
is invalid though. en
is enough here ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it invalid semantically or do you get an error when trying to use it? en
gets me the MM/DD/YYYY
format which is common in the US while en-UK
gets me DD/MM/YYYY
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean the correct locale for the UK is en-gb,
although the browser somehow recognizes the other one. As we use a neutral locale for Nethermind everywhere else, en
is the one to go with.
} | ||
updateText(version, data.version); | ||
updateText(network, networkName); | ||
(document.getElementById("network-logo") as HTMLImageElement).src = `logos/${getNetworkLogo(data.network)}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as with previous getElementById
updateTreemap<TransactionReceipt>( | ||
document.getElementById("block"), // or d3.select("#treemap") | ||
200, | ||
parseInt(data.head.gasLimit, 16), | ||
mergedData, | ||
// keyFn | ||
d => d.hash, | ||
// orderFn | ||
d => d.order, | ||
// sizeFn | ||
d => parseInt(d.gasUsed, 16), | ||
// colorFn | ||
d => parseInt(d.effectiveGasPrice, 16) * parseInt(d.gasUsed, 16) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe refactor updateTreeMap
to accept an object for named parameters instead of relying on comments?
updateTreemap<TransactionReceipt>( | |
document.getElementById("block"), // or d3.select("#treemap") | |
200, | |
parseInt(data.head.gasLimit, 16), | |
mergedData, | |
// keyFn | |
d => d.hash, | |
// orderFn | |
d => d.order, | |
// sizeFn | |
d => parseInt(d.gasUsed, 16), | |
// colorFn | |
d => parseInt(d.effectiveGasPrice, 16) * parseInt(d.gasUsed, 16) | |
); | |
updateTreemap<TransactionReceipt>( | |
{ | |
element: document.getElementById("block"), // or d3.select("#treemap"), | |
height: 200, | |
totalSize: parseInt(data.head.gasLimit, 16), | |
data: mergedData, | |
keyFn: d => d.hash, | |
orderFn: d => d.order, | |
sizeFn: d => parseInt(d.gasUsed, 16), | |
colorFn: d => parseInt(d.effectiveGasPrice, 16) * parseInt(d.gasUsed, 16) | |
}); |
frag.appendChild(newEntry); | ||
} | ||
logs = []; | ||
nodeLog.appendChild(frag); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we drain this nodeLog
element at some point? Otherwise if we keep appending then we'll run OOM.
"Mainnet": "ethereum-logo.svg", | ||
"1": "ethereum-logo.svg", | ||
"480": "world-logo.svg", | ||
"8453": "base-logo.svg" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that we have logos for Base and Worldchain. No Optimism?
2bf7b0c
to
06aa86b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces the initial iteration of a Nethermind UI to allow node monitoring without external tools, while adding TxPool metrics reporting and updating various parts of the codebase to support these features.
- Introduces new TxPool monitoring classes (TxPoolFlow, Link, Node).
- Enhances JSON RPC startup by mapping data feeds and enabling static file serving when health checks are enabled.
- Adds console message interception for logging output and new processing statistics events.
Reviewed Changes
Copilot reviewed 57 out of 60 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/Nethermind/Nethermind.Runner/Monitoring/TxPool/TxPoolFlow.cs | New TxPool metrics and state linking logic. |
src/Nethermind/Nethermind.Runner/Monitoring/TxPool/Node.cs | Defines the Node class used in TxPoolFlow. |
src/Nethermind/Nethermind.Runner/Monitoring/TxPool/Link.cs | Introduces immutable Link class for TxPool state tracking. |
src/Nethermind/Nethermind.Runner/Monitoring/NethermindNodeData.cs | Maps node information for monitoring. |
src/Nethermind/Nethermind.Runner/Monitoring/DataFeedExtensions.cs | Adds data feed mapping to endpoint routes. |
src/Nethermind/Nethermind.Runner/JsonRpc/Startup.cs | Updates JSON RPC startup to support new endpoints and static files. |
src/Nethermind/Nethermind.Runner/Ethereum/JsonRpcRunner.cs | Registers additional services for TxPool support. |
src/Nethermind/Nethermind.Runner/ConsoleHelpers.cs | Enhances console output interception and event emission. |
src/Nethermind/Nethermind.Runner.Test/StandardTests.cs | Temporarily comments out a metrics test. |
src/Nethermind/Nethermind.Consensus/Processing/* | Adds new processing statistics events and related handlers. |
src/Nethermind/Nethermind.Blockchain/* | Introduces ForkChoiceUpdated events and corresponding structural changes. |
Files not reviewed (3)
- Dockerfile: Language not supported
- Dockerfile.chiseled: Language not supported
- src/Nethermind/Nethermind.Runner/Dockerfile: Language not supported
Comments suppressed due to low confidence (1)
src/Nethermind/Nethermind.Blockchain/ReadOnlyBlockTree.cs:156
- The empty add/remove accessors for the OnForkChoiceUpdated event may lead subscribers to receive no notifications; confirm that this no-op implementation is intentional or provide a proper backing event if notifications are expected.
event EventHandler<IBlockTree.ForkChoice> IBlockTree.OnForkChoiceUpdated { add { } remove { } }
{ | ||
lock (_recentMessages) | ||
{ | ||
if (_recentMessages.Count > 100) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition may allow the queue to exceed 100 messages; using '>=' instead of '>' can ensure that the queue does not grow beyond the intended limit.
if (_recentMessages.Count > 100) | |
if (_recentMessages.Count >= 100) |
Copilot uses AI. Check for mistakes.
@@ -168,7 +180,7 @@ public void Configure(IApplicationBuilder app, IWebHostEnvironment env, IJsonRpc | |||
} | |||
} | |||
|
|||
if (method == "GET") | |||
if (method == "GET" && !(ctx.Request.Headers.Accept[0].Contains("text/html", StringComparison.Ordinal))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accessing the first element of the Accept header without checking if the header is non-empty may lead to an index out-of-range exception; consider verifying that the header has at least one entry before indexing.
if (method == "GET" && !(ctx.Request.Headers.Accept[0].Contains("text/html", StringComparison.Ordinal))) | |
if (method == "GET" && ctx.Request.Headers.Accept.Count > 0 && !(ctx.Request.Headers.Accept[0].Contains("text/html", StringComparison.Ordinal))) |
Copilot uses AI. Check for mistakes.
Was having issue with github after rebase, so reopened at this PR #8503 |
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
Documentation
Requires documentation update