CORE-7338 license: add `enterprise_license_expiry_sec` metric #23367

pgellert · 2024-09-18T16:31:16Z

This introduces a new metric for monitoring the expiry time of the enterprise license.

cluster_features_enterprise_license_expiry_sec

Description: “Number of seconds remaining until the Enterprise License expires”
Endpoints: both public and internal metrics
Labels: none

Fixes https://redpandadata.atlassian.net/browse/CORE-7338

Backports Required

Release Notes

Features

A new metric (cluster_features_enterprise_license_expiry_sec) is added for easier monitoring of the enterprise license's expiry time.

oleiman

seems straightforward. couple of structural suggestions, at your discretion

src/v/security/license.cc

oleiman · 2024-09-18T17:52:55Z

src/v/features/feature_table.cc

+    if (ss::this_shard_id() != 0) {
+        return;
+    }
+
+    if (!config::shard_local_cfg().disable_metrics()) {
+        setup_metrics(_metrics);
+    }
+
+    if (!config::shard_local_cfg().disable_public_metrics()) {
+        setup_metrics(_public_metrics);
+    }


suggestion: I would lean toward sticking this logic in setup_metrics and calling it directly from feature_table::start or something.

not sure it's 100% needed in this case, but hitting the metrics stuff in the constructor always makes me a bit nervous.

Yeah, that's a fair point. There's no feature_table::start method at the moment, and similarly there are no start methods on any controller-log-backed _table.cc's. I think that's because they are ss::sharded<> and they are expected to be constructed and started at the same time with ss::sharded::start(). So what I've done now is I've added a setup_metrics method that gets called inside the constructor of feature_table right after the probe is created to make it explicit that we start the probe immediately.

expected to be constructed and started at the same time with ss::sharded::start()

Not necessarily, you'll see this pattern throughout application.cc

https://github.com/redpanda-data/redpanda/blob/dev/src/v/redpanda/application.cc#L2475-L2478

But yeah, it doesn't seem like this probe should need an async lifetime since the lifetime of the sharded<feature_table> tracks with the whole program...the test failures[1] are real and maybe a result of feature_table being con/destructed manually across various tests and fixtures.

[1] build - these do go away if I set _probe = nullptr in feature_table::feature_table(...)

Those are not _table's though 😉 but yeah fair point, there are certainly examples of decoupled sharded service construction and start. I'm wondering what seastar-people would think of that.

Thanks for the pointer on the build, the cause turned out to be multiple feature_table's being constructed at the same time.

pgellert · 2024-09-19T12:59:40Z

Force-push:

First: rebase to dev
Second: Fixed the linking error in the CI build (missing dependency on security), addressed code review feedback

src/v/features/BUILD

vbotbuildovich · 2024-09-19T18:00:16Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/54738#01920b30-447c-414f-a20b-3d774ec8f978

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/54843#019210b8-8f03-47b2-b7d8-57a720868301

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/54843#019210b8-8f05-4210-a2a8-92316692f089

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/54843#019210ba-d69c-482f-8d6f-43239679bcd7

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/54971#0192204f-23d4-4490-94a2-39776385d208

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/55015#019223a0-4ead-440f-9fa1-6c344a245886

oleiman · 2024-09-19T19:58:08Z

Threw @BenPope on the review because he's helped me with metrics footguns before...not sure exactly what the problem is here, but my knee-jerk guess is lifetime issues.

kbatuigas · 2024-09-19T21:53:56Z

src/v/features/feature_table.cc

+          "enterprise_license_expiry_sec",
+          [this]() { return calculate_expiry_metric(_parent.get_license()); },
+          sm::description(
+            "Number of seconds remaining until the Enterprise License expires"))


Suggested change

"Number of seconds remaining until the Enterprise License expires"))

"Number of seconds remaining until the Enterprise license expires"))

Thanks, I've updated this now

kbatuigas

Just a suggestion (we won't need to capitalize "license" in the docs), lgtm otherwise

BenPope · 2024-09-20T10:15:55Z

src/v/security/license.cc

    const auto now = std::chrono::duration_cast<std::chrono::seconds>(
-      std::chrono::system_clock::now().time_since_epoch());
+      clock::now().time_since_epoch());
    return now > expiry;


I'd be tempted to reimplement this:

Suggested change

const auto now = std::chrono::duration_cast<std::chrono::seconds>(

std::chrono::system_clock::now().time_since_epoch());

clock::now().time_since_epoch());

return now > expiry;

return clock::now() > expiration();

BenPope · 2024-09-20T10:42:07Z

src/v/features/feature_table.h

@@ -655,6 +683,7 @@ class feature_table {

    ss::gate _gate;
    ss::abort_source _as;
+    std::unique_ptr<probe> _probe;


Holding the probes by pointer is useful when the owning class:

is movable; the metrics often hold a reference to a data member

has an asynchronous lifetime; to deregister the metrics on ss::future<> stop()

Even if neither of these were true, holding it by pointer is still useful as a compiler firewall; if ~feature_table() noexcept() = default; is defined in the implementation file, then probe can be forward declared, and metrics can be an implementation dep.

In order to keep your tests, you'll have to move feature_table::probe::calculate_expiry_metric to a more visible place, perhapsfeature_table::calculate_expiry_metric.

Please destroy the probe in stop().

The probe is constructed in feature_table constructor and holds a reference to it, so it's worth either making feature_table explicitly non-movable, or give it an async lifetime and construct the probe in ss::future<> start().

Suggested change

std::unique_ptr<probe> _probe;

class probe;

std::unique_ptr<probe> _probe;

Thanks for the detailed explanation, very useful. I've changed this now to have the probe be forward declared and its implementation hidden inside the implementation file.

BenPope · 2024-09-20T11:01:01Z

src/v/features/feature_table.cc

@@ -697,6 +706,49 @@ void feature_table::assert_compatible_version(bool override) {
    }
 }

+feature_table::probe::probe(const feature_table& parent)


feature_table can be passed through setup_metrics.

I've left it to be passed in the constructor because then I can hold it as a reference instead of a pointer inside the probe which means that I don't have to deal with a nullptr. tldr: it's simpler to pass it in the constructor and setup_metrics gets called right after the constructor so there's not much of a difference here

I thinking to capture it directly into the lambda:

[&ft]() { return calculate_expiry_metric(ft.get_license()); },

But it's a minor point.

BenPope · 2024-09-20T11:11:29Z

src/v/features/feature_table.cc

+    if (!license) {
+        return -1;
+    }


discussion:

If there is no license, it may be possible to not set up this metric (or even the probe), but then setting up the probe or metric must be attached to set_license / revoke_license / feature_table_snapshot::apply.

For the latter, it may be worth:

diff --git a/src/v/features/feature_table_snapshot.cc b/src/v/features/feature_table_snapshot.cc index 6e1cfb406e..b5477a88c5 100644 --- a/src/v/features/feature_table_snapshot.cc +++ b/src/v/features/feature_table_snapshot.cc @@ -42,7 +42,11 @@ void feature_table_snapshot::apply(feature_table& ft) const { applied_offset); ft.set_active_version(version); - ft._license = license; + if (license) { + ft.set_license(*license); + } else { + ft.revoke_license(); + }

But I'm also OK with returning -1.

Yeah, that's a good point to discuss but I've kept this as is for now to keep things simpler. I kinda like having the -1 explicitly present to signal that the license it unset (as opposed to just the metric missing because of network issues, Prometheus scraper being down, etc.). But I don't feel strongly either way which makes me prefer simplicity.

may be possible to not set up this metric (or even the probe)

I kinda like that. But yeah, doesn't seem too critical

pgellert · 2024-09-20T16:58:37Z

Force-push:

code review feedback (mostly hiding the probe in the implementation file plus some minor things)
added a bazel build for features/test while I was there
fixed the failing test -- some storage fixture tests create multiple feature_table instances at the same time (specifically disk_log_builder creates one under the hood) which causes metric double-registration issues, so to solve this I followed the existing pattern and disabled metrics

BenPope · 2024-09-23T10:53:13Z

src/v/features/feature_table.cc

@@ -232,9 +284,15 @@ feature_table::feature_table() {
            }
        }
    }
+


declaring the destructor made this true, but I wonder if it's worth being explicit:

static_assert( !std::is_move_constructible_v<feature_table> && !std::is_move_assignable_v<feature_table> && !std::is_copy_constructible_v<feature_table> && !std::is_copy_assignable_v<feature_table>, "The probe captures a reference to this");

Yeah I like it, I've adopted this now

BenPope · 2024-09-23T10:56:26Z

tests/rptest/tests/rpk_cluster_test.py

+
+        internal_val = get_metrics_value(MetricsEndpoint.METRICS)
+        public_val = get_metrics_value(MetricsEndpoint.PUBLIC_METRICS)
+        assert internal_val == public_val, f"Mismatch: {internal_val} != {public_val}"


I guess it's possible to get unlucky and have the scrapes cross over a 1 second boundary.

That should be fine though because it's called from a wait_until with retry_on_exc=True so in that case we would just retry.

oleiman

lgtm

src/v/features/feature_table.h

oleiman · 2024-09-23T14:35:14Z

src/v/features/feature_table.cc

+
+    auto rem = license->expiration() - now;
+    auto rem_capped = std::max(rem.zero(), rem);
+    return std::chrono::duration_cast<std::chrono::seconds>(rem_capped).count();


Nit: rem_capped / 1s

pgellert · 2024-09-23T18:01:06Z

Force-push: adopt remaining minor improvement suggestions

oleiman

lgtm!

Expose the expiration as a time_point.

These two fixture tests create multiple `feature_table`s at the same time. This commit disables emitting metrics on these tests to prevent double metric registration errors in the follow up commit of this PR.

Adds a metric to allow easier monitoring of when the enterprise license is going to expiry. The dependency on the `security` module existed even previously in the bazel build but it was missing from the cmake build.

Adds a bazel build for the features module. The include for base/vlog.h was unused, so it was removed instead of adding a dependency.

Adds an integration test for the license expiry metric.

pgellert · 2024-09-24T09:30:53Z

Force-push:

BenPope · 2024-09-24T09:56:49Z

src/v/features/feature_table.cc

+          {
+            sm::make_gauge(
+              "enterprise_license_expiry_sec",
+              [&ft = _parent]() {


My idea was so that you didn't need the _parent member.

Right, now I get what you meant there, that makes more sense. I will leave this as is now since the behaviour is pretty much the same.

vbotbuildovich · 2024-09-24T15:24:56Z

/backport v24.2.x

vbotbuildovich · 2024-09-24T15:24:57Z

/backport v24.1.x

vbotbuildovich · 2024-09-24T15:24:58Z

/backport v23.3.x

vbotbuildovich · 2024-09-24T15:26:06Z

Failed to create a backport PR to v24.1.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-23367-v24.1.x-169 remotes/upstream/v24.1.x
git cherry-pick -x 78de1c702ef22c1becb876913674b9c5fa7de31b 1c8f66b467b0b5e84de6e6c424fbe65e825f5e98 edb5f48493aaec25ebcb10f35eb26b3ad5ae1a10 c44d9d8f53222dc4c50063cafc8ff020dc4e586d baece2ccd6c9986a85c1af108dbb5b8387e85f1f

Workflow run logs.

vbotbuildovich · 2024-09-24T15:26:10Z

Failed to create a backport PR to v24.2.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-23367-v24.2.x-227 remotes/upstream/v24.2.x
git cherry-pick -x 78de1c702ef22c1becb876913674b9c5fa7de31b 1c8f66b467b0b5e84de6e6c424fbe65e825f5e98 edb5f48493aaec25ebcb10f35eb26b3ad5ae1a10 c44d9d8f53222dc4c50063cafc8ff020dc4e586d baece2ccd6c9986a85c1af108dbb5b8387e85f1f

Workflow run logs.

vbotbuildovich · 2024-09-24T15:26:12Z

Failed to create a backport PR to v23.3.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-23367-v23.3.x-530 remotes/upstream/v23.3.x
git cherry-pick -x 78de1c702ef22c1becb876913674b9c5fa7de31b 1c8f66b467b0b5e84de6e6c424fbe65e825f5e98 edb5f48493aaec25ebcb10f35eb26b3ad5ae1a10 c44d9d8f53222dc4c50063cafc8ff020dc4e586d baece2ccd6c9986a85c1af108dbb5b8387e85f1f

Workflow run logs.

pgellert requested review from a team September 18, 2024 16:31

pgellert self-assigned this Sep 18, 2024

pgellert requested review from andijcr and oleiman and removed request for a team September 18, 2024 16:31

github-actions bot added the area/redpanda label Sep 18, 2024

oleiman reviewed Sep 18, 2024

View reviewed changes

pgellert force-pushed the elf/expiry-metric branch 2 times, most recently from c29dc0c to 1d76d0a Compare September 19, 2024 12:58

github-actions bot added the area/build label Sep 19, 2024

pgellert requested a review from oleiman September 19, 2024 13:00

pgellert commented Sep 19, 2024

View reviewed changes

src/v/features/BUILD Outdated Show resolved Hide resolved

oleiman requested a review from BenPope September 19, 2024 19:52

kbatuigas reviewed Sep 19, 2024

View reviewed changes

BenPope reviewed Sep 20, 2024

View reviewed changes

pgellert force-pushed the elf/expiry-metric branch from 1d76d0a to f2fc540 Compare September 20, 2024 16:55

pgellert requested review from BenPope and kbatuigas September 20, 2024 16:58

BenPope reviewed Sep 23, 2024

View reviewed changes

BenPope previously approved these changes Sep 23, 2024

View reviewed changes

oleiman previously approved these changes Sep 23, 2024

View reviewed changes

pgellert dismissed stale reviews from oleiman and BenPope via f630e93 September 23, 2024 17:59

pgellert force-pushed the elf/expiry-metric branch from f2fc540 to f630e93 Compare September 23, 2024 17:59

pgellert requested review from BenPope and oleiman September 23, 2024 18:01

oleiman previously approved these changes Sep 23, 2024

View reviewed changes

security: add license::expiration

78de1c7

Expose the expiration as a time_point.

pgellert force-pushed the elf/expiry-metric branch from f630e93 to 89ccd73 Compare September 24, 2024 09:28

pgellert added 4 commits September 24, 2024 10:29

storage: disable metrics in disk_log_builder tests

1c8f66b

These two fixture tests create multiple `feature_table`s at the same time. This commit disables emitting metrics on these tests to prevent double metric registration errors in the follow up commit of this PR.

features: new metric enterprise_license_expiry_sec

edb5f48

Adds a metric to allow easier monitoring of when the enterprise license is going to expiry. The dependency on the `security` module existed even previously in the bazel build but it was missing from the cmake build.

f/test: add bazel build

c44d9d8

Adds a bazel build for the features module. The include for base/vlog.h was unused, so it was removed instead of adding a dependency.

dt: test enterprise_license_expiry_sec metric

baece2c

Adds an integration test for the license expiry metric.

pgellert dismissed oleiman’s stale review via baece2c September 24, 2024 09:29

pgellert force-pushed the elf/expiry-metric branch from 89ccd73 to baece2c Compare September 24, 2024 09:29

pgellert requested a review from oleiman September 24, 2024 09:31

BenPope approved these changes Sep 24, 2024

View reviewed changes

pgellert merged commit 3ff5d86 into redpanda-data:dev Sep 24, 2024
18 checks passed

vbotbuildovich mentioned this pull request Sep 24, 2024

[v24.1.x] CORE-7338 license: add enterprise_license_expiry_sec metric #23454

Closed

This was referenced Sep 24, 2024

[v24.2.x] CORE-7338 license: add enterprise_license_expiry_sec metric #23455

Closed

[v23.3.x] CORE-7338 license: add enterprise_license_expiry_sec metric #23456

Closed

This was referenced Sep 24, 2024

[v24.2.x] CORE-7338 license: add metric #23466

Merged

[v24.1.x] CORE-7338 license: add metric #23467

Merged

[v23.3.x] CORE-7338 license: add metric #23468

Merged

	"Number of seconds remaining until the Enterprise License expires"))
	"Number of seconds remaining until the Enterprise license expires"))

	std::unique_ptr<probe> _probe;
	class probe;
	std::unique_ptr<probe> _probe;

@@ @@ -232,9 +284,15 @@ feature_table::feature_table() { @@
                           }
                       }
                   }

CORE-7338 license: add enterprise_license_expiry_sec metric #23367

CORE-7338 license: add enterprise_license_expiry_sec metric #23367

Conversation

pgellert commented Sep 18, 2024

cluster_features_enterprise_license_expiry_sec

Backports Required

Release Notes

Features

oleiman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgellert commented Sep 19, 2024

vbotbuildovich commented Sep 19, 2024 • edited Loading

oleiman commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbatuigas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenPope Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgellert commented Sep 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleiman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgellert commented Sep 23, 2024

oleiman left a comment

Choose a reason for hiding this comment

pgellert commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbotbuildovich commented Sep 24, 2024

vbotbuildovich commented Sep 24, 2024

vbotbuildovich commented Sep 24, 2024

vbotbuildovich commented Sep 24, 2024

vbotbuildovich commented Sep 24, 2024

vbotbuildovich commented Sep 24, 2024

CORE-7338 license: add `enterprise_license_expiry_sec` metric #23367

CORE-7338 license: add `enterprise_license_expiry_sec` metric #23367

vbotbuildovich commented Sep 19, 2024 •

edited

Loading

BenPope Sep 20, 2024 •

edited

Loading