[PATCH v3 1/3] sstables/storage: add funcs to track used space per storage type per shard

[Laszlo Ersek]

Cc: "Raphael S. Carvalho" <raph...@scylladb.com>
Ref: https://github.com/scylladb/scylladb/issues/18191
Signed-off-by: Laszlo Ersek <laszlo...@scylladb.com>
---

Notes:
v3:

- create metrics at the storage level, for tracking used space not only
by shard but also by storage type [Avi]

- split out to its own patch

- use make_current_bytes() rather than make_gauge()

sstables/storage.hh | 11 +++++++++
main.cc | 1 +
sstables/storage.cc | 25 ++++++++++++++++++--
3 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/sstables/storage.hh b/sstables/storage.hh
index efb4831b116a..c4ac54215fec 100644
--- a/sstables/storage.hh
+++ b/sstables/storage.hh
@@ -76,9 +76,15 @@ class opened_directory final {
}
};

+struct storage_stats {
+ uint64_t bytes_occupied = 0;
+};
+
class storage {
friend class test;

+ storage_stats& _stats;
+
// Internal, but can also be used by tests
virtual future<> change_dir_for_test(sstring nd) {
SCYLLA_ASSERT(false && "Changing directory not implemented");
@@ -91,11 +97,15 @@ class storage {
}

public:
+ explicit storage(storage_stats& stats) : _stats(stats) {}
virtual ~storage() {}

using absolute_path = bool_class<class absolute_path_tag>; // FIXME -- should go away eventually
using sync_dir = bool_class<struct sync_dir_tag>; // meaningful only to filesystem storage

+ void credit_bytes(uint64_t bytes) noexcept { _stats.bytes_occupied += bytes; }
+ void debit_bytes(uint64_t bytes) noexcept { _stats.bytes_occupied -= bytes; }
+
virtual future<> seal(const sstable& sst) = 0;
virtual future<> snapshot(const sstable& sst, sstring dir, absolute_path abs, std::optional<generation_type> gen = {}) const = 0;
virtual future<> change_state(const sstable& sst, sstable_state to, generation_type generation, delayed_commit_changes* delay) = 0;
@@ -119,5 +129,6 @@ std::unique_ptr<sstables::storage> make_storage(sstables_manager& manager, const
future<lw_shared_ptr<const data_dictionary::storage_options>> init_table_storage(const sstables_manager&, const schema&, const data_dictionary::storage_options& so);
future<> destroy_table_storage(const data_dictionary::storage_options& so);
future<> init_keyspace_storage(const sstables_manager&, const data_dictionary::storage_options& so, sstring ks_name);
+future<> init_storage_metrics();

} // namespace sstables
diff --git a/main.cc b/main.cc
index 79ef50a63ae9..f129c5f948ff 100644
--- a/main.cc
+++ b/main.cc
@@ -1590,6 +1590,7 @@ To start the scylla server proper, simply invoke as: scylla server (or just scyl
// #293 - do not stop anything
// engine().at_exit([&qp] { return qp.stop(); });
sstables::init_metrics().get();
+ sstables::init_storage_metrics().get();

db::sstables_format_listener sst_format_listener(gossiper.local(), feature_service, sst_format_selector);

diff --git a/sstables/storage.cc b/sstables/storage.cc
index 9896bc986ba5..df63b7686edb 100644
--- a/sstables/storage.cc
+++ b/sstables/storage.cc
@@ -69,8 +69,10 @@ class filesystem_storage final : public sstables::storage {
}

public:
+ static thread_local storage_stats filesystem_stats;
explicit filesystem_storage(sstring dir, sstable_state state)
- : _base_dir(dir)
+ : storage(filesystem_stats)
+ , _base_dir(dir)
, _dir(make_path(dir, state))
{}

@@ -515,8 +517,10 @@ class s3_storage : public sstables::storage {
sstring make_s3_object_name(const sstable& sst, component_type type) const;

public:
+ static thread_local storage_stats s3_stats;
s3_storage(shared_ptr<s3::client> client, sstring bucket, sstring dir)
- : _client(std::move(client))
+ : storage(s3_stats)
+ , _client(std::move(client))
, _bucket(std::move(bucket))
, _location(std::move(dir))
{
@@ -727,4 +731,21 @@ future<> destroy_table_storage(const data_dictionary::storage_options& so) {
}, so.value);
}

+thread_local storage_stats filesystem_storage::filesystem_stats;
+thread_local storage_stats s3_storage::s3_stats;
+static const seastar::metrics::label storage_label("storage_class");
+static thread_local seastar::metrics::metric_groups metrics;
+
+future<> init_storage_metrics() {
+ return seastar::smp::invoke_on_all([] {
+ namespace sm = seastar::metrics;
+ metrics.add_group("storage", {
+ sm::make_current_bytes("bytes_occupied", [] { return filesystem_storage::filesystem_stats.bytes_occupied; },
+ sm::description("Bytes occupied by sstables"), { storage_label("filesystem") }),
+ sm::make_current_bytes("bytes_occupied", [] { return s3_storage::s3_stats.bytes_occupied; },
+ sm::description("Bytes occupied by sstables"), { storage_label("s3") }),
+ });
+ });
+}
+
} // namespace sstables

[Benny Halevy]

We try to avoid globals like a plague.
I'm not sure where the requirements for this feature come from
(I see that Avi is mentioned in the Notes section, so Cc'ing him here.)
but usually we track this kind of stats per tablet, and I think it
also has value to get the distribution of capacity per storage type
per table, especially, when we'll have tiering, that is going to be configured
most probably per table.

[Laszlo Ersek]

I don't see much *practical* difference between a public data member and a get_stats() public member function that returns a reference. I understand the *promise* of the member function, I just don't see it paying off. Most of the time the internal representation cannot be changed while sustaining the previous external interface *efficiently*. In other words, as soon as a getter starts becoming useful, it inevitably starts turning into *cruft*.

That said, I can certainly wrap this into a getter.

> I'm not sure where the requirements for this feature come from
> (I see that Avi is mentioned in the Notes section, so Cc'ing him here.)

It was proposed by Raphael here (not an exhaustive list of references):

- https://github.com/scylladb/scylladb/issues/18191#issue-2226027135
- https://github.com/scylladb/scylladb/issues/18191#issuecomment-2037707711

The per-storage-type distinction was proposed by Avi:

- https://github.com/scylladb/scylladb/pull/20574#discussion_r1767380868

> but usually we track this kind of stats per tablet, and I think it
> also has value to get the distribution of capacity per storage type
> per table, especially, when we'll have tiering, that is going to be configured
> most probably per table.

Raphael, would that serve your needs?

Thanks!
Laszlo