Skip to content
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.

Sign up for GitHub

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

Election age statistics #4537

Merged
merged 3 commits into from
Apr 9, 2024
Merged

Election age statistics #4537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ab69325
Add election age information to statistics
RickiNano Apr 4, 2024
8bcd913
Updated unit test
RickiNano Apr 4, 2024
2151c27
Fixed unit test
RickiNano Apr 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions nano/node/election.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,10 @@ class election final : public std::enable_shared_from_this<election>
nano::vote_info get_last_vote (nano::account const & account);
void set_last_vote (nano::account const & account, nano::vote_info vote_info);
nano::election_status get_status () const;
std::chrono::steady_clock::time_point get_election_start () const
clemahieu marked this conversation as resolved.
Show resolved Hide resolved
{
return election_start;
}

private: // Dependencies
nano::node & node;
Expand Down
23 changes: 20 additions & 3 deletions nano/node/json_handler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2009,10 +2009,21 @@ void nano::json_handler::election_statistics ()
unsigned hinted_count = 0;
unsigned optimistic_count = 0;
unsigned total_count = 0;
std::chrono::steady_clock::duration total_age{};
auto now = std::chrono::steady_clock::now ();
std::chrono::steady_clock::time_point oldest_election_start = now;

for (auto const & election : active_elections)
{
total_count++;
auto election_start = election->get_election_start ();
auto age = now - election_start;
total_age += age;
if (election_start < oldest_election_start)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can oldest_election_start = std::min(oldest_election_start, election->election_start) be used instead of branching?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think I can do that if using the getter function. That would probably require election_start to be public which I think is a bad idea. It shouldn't be modified outside the election class

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be compatible with the getter:

oldest_election_start = std::min(oldest_election_start, election->get_election_start())

{
oldest_election_start = election_start;
}

switch (election->behavior ())
{
case election_behavior::normal:
Expand All @@ -2028,14 +2039,20 @@ void nano::json_handler::election_statistics ()
}

auto utilization_percentage = (static_cast<double> (total_count * 100) / node.config.active_elections_size);
std::stringstream stream;
stream << std::fixed << std::setprecision (2) << utilization_percentage;
auto max_election_age = std::chrono::duration_cast<std::chrono::seconds> (now - oldest_election_start).count ();
double average_election_age = total_count ? std::chrono::duration<double> (total_age).count () / total_count : 0;

std::stringstream stream_utilization, stream_average_age;
stream_utilization << std::fixed << std::setprecision (2) << utilization_percentage;
stream_average_age << std::fixed << std::setprecision (2) << average_election_age;

response_l.put ("normal", normal_count);
response_l.put ("hinted", hinted_count);
response_l.put ("optimistic", optimistic_count);
response_l.put ("total", total_count);
response_l.put ("aec_utilization_percentage", stream.str ());
response_l.put ("aec_utilization_percentage", stream_utilization.str ());
response_l.put ("max_election_age", max_election_age);
response_l.put ("average_election_age", stream_average_age.str ());

response_errors ();
}
Expand Down
42 changes: 33 additions & 9 deletions nano/rpc_test/rpc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6870,14 +6870,38 @@ TEST (rpc, confirmation_info)
TEST (rpc, election_statistics)
{
nano::test::system system;
auto node1 = add_ipc_enabled_node (system);
nano::node_config node_config;
node_config.ipc_config.transport_tcp.enabled = true;
node_config.ipc_config.transport_tcp.port = system.get_available_port ();
nano::node_flags node_flags;
node_flags.disable_request_loop = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to explain why you are disabling the request loop.

auto node1 (system.add_node (node_config, node_flags));
auto const rpc_ctx = add_rpc (system, node1);
boost::property_tree::ptree request1;
request1.put ("action", "election_statistics");
auto response1 (wait_response (system, rpc_ctx, request1));
ASSERT_EQ ("0", response1.get<std::string> ("normal"));
ASSERT_EQ ("0", response1.get<std::string> ("hinted"));
ASSERT_EQ ("0", response1.get<std::string> ("optimistic"));
ASSERT_EQ ("0", response1.get<std::string> ("total"));
ASSERT_EQ ("0.00", response1.get<std::string> ("aec_utilization_percentage"));

nano::block_builder builder;
auto send1 = builder
.send ()
.previous (nano::dev::genesis->hash ())
.destination (nano::public_key ())
.balance (nano::dev::constants.genesis_amount - 100)
.sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub)
.work (*system.work.generate (nano::dev::genesis->hash ()))
.build ();
node1->process_active (send1);
ASSERT_TIMELY (5s, !node1->active.empty ());
Copy link
Contributor

Choose a reason for hiding this comment

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

This line looks strange to me. Maybe there is a race condition between process_active() and start_elections(). The usual way to use start_elections() is:

	ASSERT_TRUE (nano::test::process (*node1, {send1}));
	ASSERT_TRUE (nano::test::start_elections (system, *node1, {send1}));

ASSERT_TRUE (nano::test::start_elections (system, *node1, { send1 }));
ASSERT_EQ (1, node1->active.size ());

boost::property_tree::ptree request;
request.put ("action", "election_statistics");
{
auto response (wait_response (system, rpc_ctx, request));
ASSERT_EQ ("1", response.get<std::string> ("normal"));
ASSERT_EQ ("0", response.get<std::string> ("hinted"));
ASSERT_EQ ("0", response.get<std::string> ("optimistic"));
ASSERT_EQ ("1", response.get<std::string> ("total"));
ASSERT_NE ("0.00", response.get<std::string> ("aec_utilization_percentage"));
ASSERT_NO_THROW (response.get<std::string> ("max_election_age"));
Copy link
Contributor

Choose a reason for hiding this comment

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

These 2 checks could check that the values are within an expected range.

ASSERT_NO_THROW (response.get<std::string> ("average_election_age"));
}
}
Loading