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.

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

Stats overhaul #4583

Merged
merged 26 commits into from
Apr 30, 2024
Merged

Conversation

pwojcikdev
Copy link
Contributor

@pwojcikdev pwojcikdev commented Apr 24, 2024

Stats overhaul

The goal of this PR is to address some of the issues that have been increasingly cropping up with the current stats system. I believe that having a more flexible and performant system will allow us to experiment and iterate faster.

The immediate problem that is fixed is the fact that we ran out of space for declaring new stat enums:

In file included from /nano-node/nano/lib/stats.hpp:5:
/nano-node/nano/lib/stats_enums.hpp:407:2: error: enumerator value 256 is not representable in the underlying type 'uint8_t' (aka 'unsigned char')

However, this PR contains a bit more than that. The main changes are:

  • Do not require stat::detail enum to have uint8 underlying type. This allows for practically unlimited number of custom stats.
  • Offload heavy IO operations (saving stats to disk) to a dedicated thread. Previously this was done inside the call to stat update functions, which introduced latency spikes.
  • Rework the way histograms are handled. Previously histograms needed to be registered with the stats system beforehand, which could be cumbersome and as a result not used. Now we store samples in a ring buffer and leave histogram creation to the user (e.g. nano-prom-exporter).
    Note: I'm still not sure whether this approach is really better, this needs a bit more experimentation.
  • Use read-write locks with atomic integers for counter values. This is a bit experimental, but on my system it seems to give some performance benefits. I'll try to include microbenchmarking suite as part of a separate PR (ongoing work is here https://github.com/pwojcikdev/nano-node/tree/stats-benchmarking )
  • Remove special semantics of stat::detail::all enum. Previously we tracked a special all entry that was used to aggregate all stats of a given type. It was used only by unit tests and required special filters to remove from grafana dashboards, where it unnecessarily cluttered the view. Now stat aggregation is handled by a dedicated overload of stats::count () function.

Heatmaps

I added experimental support for histogram generation to my fork of the nano-prom-exporter (https://github.com/pwojcikdev/nano-prom-exporter/commits/histograms/). This allows for generating heatmaps like this one:

Screenshot 2024-03-06 at 10 20 51

@pwojcikdev pwojcikdev force-pushed the stats-improvements-2 branch from 98be662 to c6f88b7 Compare April 24, 2024 14:51
@pwojcikdev pwojcikdev force-pushed the stats-improvements-2 branch from c6f88b7 to 663b98b Compare April 24, 2024 19:05
dsiganos
dsiganos previously approved these changes Apr 26, 2024
@dsiganos
Copy link
Contributor

LGTM, I did a high level review only because a detailed review would take too long.

@gr0vity-dev
Copy link
Contributor

When doing a quick throughput test, the performance seems similar.
One thing I noticed is that the "all" tab from messages remains empty.
(I didn't update the grafana dashboard yet)

Screenshot 2024-04-29 at 08 21 47

# Conflicts:
#	nano/lib/thread_roles.hpp
@pwojcikdev pwojcikdev merged commit 04de36c into nanocurrency:develop Apr 30, 2024
25 of 26 checks passed
@qwahzi qwahzi added this to the V27 milestone Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Merged / V27.0
Development

Successfully merging this pull request may close these issues.

5 participants