-
Notifications
You must be signed in to change notification settings - Fork 100
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
Performance Monitor Plugin #188
Conversation
@meevee98 Gread work! But there are some issues need to be fixed.
always return Timeout.
|
Thank you for your tests @cloud8little
I couldn't reproduce, can you give me more details?
For this to work, you need to add
Confirmation and payload time only work after start consensus, that's why it always returns a timeout. I've changed the printed message to make this condition clearer. |
|
Thank you for your help @bettybao1209. I think the readme is very good.
I investigated the problem and I think this is not wrong at all. That's why the result of For this, the problem was that some blocks had a much longer time in seconds, increasing the average. I found that these three blocks had a time greater than 4 hours, with the longest between blocks 100 and 101, with a gap of almost 2 days: Probably that happened because the blockchain was paused and continued later. I'm still working on the problem with Performance Counter. |
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.
Looks good, I see that new performance indicators were added.
@meevee98 it's this solved? #188 (comment) |
[ConsoleCommand("block avgtime", Category = "Block Commands", Description = "Show the average time in seconds the latest blocks are active.")] | ||
private void OnBlockAverageTimeCommand(uint blockCount = 1000) | ||
{ | ||
uint desiredCount = blockCount; |
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.
Reuse blockCount
throw new RpcException(-100, "Minimum 1 block"); | ||
} | ||
|
||
if (desiredCount > 10000) |
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.
How much time spend with this limit?
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.
[ConsoleCommand("block timestamp", Category = "Block Commands", Description = "Show the block timestamp for each of the n latest blocks.")] | ||
private void OnBlockTimestampCommand(uint blockCount) | ||
{ | ||
uint desiredCount = blockCount; |
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.
Reuse blockCount
uint maxNBlocksPerDay = 24 * 60 * 60 / (Blockchain.MillisecondsPerBlock / 1000); | ||
if (desiredCount > maxNBlocksPerDay) | ||
{ | ||
throw new RpcException(-100, maxNBlocksPerDay.ToString("Maximum 0 blocks")); |
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.
0?
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 was the format syntax. I changed it to make clearer what the message should be.
using (var snapshot = Blockchain.Singleton.GetSnapshot()) | ||
{ | ||
var block = GetBlock(height.ToString()); | ||
var blocksTime = snapshot.GetBlocksTimestamp(desiredCount, block); |
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.
Did you test the time it spend with the maximum values?
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.
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 think that it should be lower than 1000 in rpc, in console it's ok
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.
Sure, I have no problem decreasing it.
/// <param name="height"> | ||
/// The block height to be passed in the ping message | ||
/// </param> | ||
private void SendBlockchainPingMessage(uint height) |
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 should send the ping always with our hight, why it's an argument?
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.
This method is used in the block sync
command. In the case that the local node is not updated, the ping message sends the max height of the remote nodes.
It was needed before the implementation of the ping
command, but now it can just send the ping with the current height. I will change it.
|
||
try | ||
{ | ||
client.GetBlockCount(); |
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.
MaxTimeout? what's happend if the server never answer and it doesn't close the connection?
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.
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.
By default it's 100 seconds https://docs.microsoft.com/en-us/dotnet/api/system.net.http.httpclient.timeout?view=netcore-3.1 I think that it should be 30 or less
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 your point.
I'm going to set the timeout in the method then.
@neo-project/ngd-shanghai could someone test it? |
@cloud8little will help test this. |
|
@meevee98 could you help answer the #14 please? #188 (comment) |
Hi @Liaojinghui,
Is this still relevant? |
it doesnt support other OS, like linux. And never fixed anything, from what i can see. |
close as inactive |
This is a plugin created to monitor some aspects of Neo.
The metrics that the plugin measures are the ones discussed in neo#920