feat: add max task usage and num stateful tasks metrics #8549

lct45 · 2021-12-21T18:42:07Z

Description

We're adding a metric (max_task_storage_used_bytes) that takes the maximum task disk usage on a node and emits that. This allows users to see the maximum disk space a single task is taking up without having to filter through all the task metrics on their side.

We're also adding a metric num_stateful_tasks that records the number of stateful tasks on a node. These two metrics combined will allow users to better understand their task work distribution and understand how to tune their system for their needs.

Testing done

unit tests, tested on jconsole

Reviewer checklist

Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
Ensure relevant issues are linked (description should include text like "Fixes #")

e

lct45 · 2021-12-21T22:11:08Z

ksqldb-engine/src/main/java/io/confluent/ksql/internal/StorageUtilizationMetricsReporter.java

@@ -101,6 +108,14 @@ public static void configureShared(
        (((double) baseDir.getTotalSpace() - (double) baseDir.getFreeSpace()) 
            / (double) baseDir.getTotalSpace())
    );
+    metricRegistry.addMetric(


actually getting NPEs from these two, it seems like configureShared is called before configure to the metricRegistry and numberStatefulTasks aren't initialized before they're called in the gauges

updated the gauges to handle the if the items are null, moving them to configure means they won't get set at start-up and you'll have to check to see if it's already been registered so we don't get errors about double registering metrics

> SELECT profileId, > LATEST_BY_OFFSET(latitude) AS la, > LATEST_BY_OFFSET(longitude) AS lo > FROM riderlocations > GROUP BY profileId > EMIT CHANGES; Could not write the statement 'CREATE TABLE currentLocation AS SELECT profileId, LATEST_BY_OFFSET(latitude) AS la, LATEST_BY_OFFSET(longitude) AS lo FROM riderlocations GROUP BY profileId EMIT CHANGES;' into the command topic. Caused by: A metric named 'MetricName [name=max_task_storage_used_bytes, group=ksqldb_utilization, description=, tags={}]' already exists, can't register another one.```

Not sure I understand the above issue: what object the NPE is referring to? The metric names like numberStatefulTasks should be inside the registry, and here we are just adding a new metric under that name, and numberStatefulTasks are already static so it should have been initialized at the time this line is executed.

The NPE was when the gauge was being sampled, so when getMaxTaskUsage was called it tried to access the metrics registry before it had been initialized since it gets initialized in configure. This actually shouldn't happen now that I pass the metrics registry into getMaxTaskUsage. The other NPE was for numberStatefulTasks which was initialized in configure so when the gauge called numberStatefulTasks.get() it returned a NPE. This is now initialized to 0 so that NPE is also resolved

rodesai · 2021-12-21T22:37:05Z

ksqldb-engine/src/main/java/io/confluent/ksql/internal/StorageUtilizationMetricsReporter.java

@@ -49,8 +51,9 @@
  private static final String METRIC_GROUP = "ksqldb_utilization";

  private Map<String, Map<String, TaskStorageMetric>> metricsSeen;
-  private Metrics metricRegistry;
+  private static Metrics metricRegistry;


This should not be static - just leave it local to each member. We intentionally made it not-static to avoid re-using the instance across test runs.

Is there a way to still reference it from a static context?

I didn't change this because I'm not sure how to handle it, configureShared is called from a static context so it has to be static, meaning getMaxTaskUsage has to be static which calls metricRegistry.metrics() which intellij is also telling me needs to be static to be called from this context. I can create a helper function to get metricRegistry.metrics()` but it seems like it would have the same issue

configureShared is passed a MetricsRegistry instance that you can register the metrics in.

Ah makes sense, just updated

rodesai · 2021-12-21T22:38:45Z

ksqldb-engine/src/main/java/io/confluent/ksql/internal/StorageUtilizationMetricsReporter.java

@@ -65,6 +68,7 @@ public synchronized void configure(final Map<String, ?> map) {
        map.get(KsqlConfig.KSQL_INTERNAL_METRICS_CONFIG)
    );
    this.metricsSeen = new HashMap<>();
+    this.numberStatefulTasks = new AtomicInteger(0);


This is a static, so we shouldn't re-initialize for each instance. We should just initialize this statically (e.g. private static AtomicInteger numberStatefulTasks = new AtomicInteger(0);) or from configureShared

just changed it to be initialized statically, updated the gauge as well

rodesai · 2021-12-21T22:43:52Z

ksqldb-engine/src/main/java/io/confluent/ksql/internal/StorageUtilizationMetricsReporter.java

+    if (metricRegistry == null) {
+      return BigInteger.ZERO;
+    }
+    final Collection<KafkaMetric> taskMetrics = metricRegistry


This is kind of expensive. Once this is shipped can you just see what happens to the cpu of an idle instance? I don't think it will make much difference because it only runs every 30 seconds. But better to check anyway. Alternatively we could keep a static set of the tasks and a max that's updated on every new or removed task.

idle instance as in an instance with no queries or instance with queries but no data? I can check after shipping, it's definitely ... unideal, but storing the tasks / max had a longer LOE so went with this first.

Would the static max update as the underlying task metrics do? I wanted to make sure we were getting the max of all the most recent gauge values

idle instance as in an instance with no queries or instance with queries but no data? I can check after shipping, it's definitely ... unideal, but storing the tasks / max had a longer LOE so went with this first.

Yep. An idle cluster will still have loads of metrics, so seeing if the cpu usage for an idle cluster is higher than normal is a good test.

Would the static max update as the underlying task metrics do? I wanted to make sure we were getting the max of all the most recent gauge values

Good point - the space used is not known at the time the metric is added. If the current impl turns out too expensive we can just maintain a static mapping of task to sst file used metrics and then take the max every time the metric is sampled (rather than filtering over every metric like we're doing here).

rodesai · 2021-12-21T22:44:42Z

...b-engine/src/test/java/io/confluent/ksql/internal/StorageUtilizationMetricsReporterTest.java

@@ -253,6 +267,58 @@ public void shouldIgnoreNonSSTMetrics() {
    // Then:
    assertThrows(AssertionError.class, () -> verifyAndGetRegisteredMetric(TASK_STORAGE_METRIC, TASK_ONE_TAGS));
  }
+
+  @Test


can you add a case for max task when there are 0 tasks?

rodesai

LGTM with one nit inline

rodesai · 2022-01-04T17:30:28Z

ksqldb-engine/src/main/java/io/confluent/ksql/internal/StorageUtilizationMetricsReporter.java

+        .metrics()
+        .entrySet()
+        .stream()
+        .filter(e -> e.getKey().name().contains("task_storage_used_bytes"))


put the metric name in a constant

feat: add max task usage metric

bb3fdd5

lct45 requested a review from a team as a code owner December 21, 2021 18:42

lct45 requested a review from rodesai December 21, 2021 19:03

lct45 added 2 commits December 21, 2021 11:41

checkstyl

cfff094

e

add number stateful tasks

ceae440

lct45 changed the title ~~feat: add max task usage metric~~ feat: add max task usage and num stateful tasks metrics Dec 21, 2021

change from atomic int to int

f6d3059

lct45 commented Dec 21, 2021

View reviewed changes

handle NPEs

9ed5a55

rodesai reviewed Dec 21, 2021

View reviewed changes

lct45 added 2 commits December 21, 2021 15:12

review comments

8458505

review comments

8f13c11

rodesai approved these changes Jan 4, 2022

View reviewed changes

make name constant

f6a2a0f

lct45 merged commit ba27e3a into confluentinc:master Jan 4, 2022

agavra mentioned this pull request Jan 5, 2022

test: add more logging to flaky stream insert test #8558

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add max task usage and num stateful tasks metrics #8549

feat: add max task usage and num stateful tasks metrics #8549

lct45 commented Dec 21, 2021 •

edited

Loading

lct45 Dec 21, 2021

lct45 Dec 21, 2021 •

edited

Loading

guozhangwang Dec 22, 2021

lct45 Jan 3, 2022

rodesai Dec 21, 2021

lct45 Dec 21, 2021

lct45 Dec 21, 2021

rodesai Dec 22, 2021

lct45 Jan 3, 2022

rodesai Dec 21, 2021

lct45 Dec 21, 2021

rodesai Dec 21, 2021

lct45 Dec 21, 2021

rodesai Dec 22, 2021

rodesai Dec 22, 2021 •

edited

Loading

rodesai Dec 21, 2021

lct45 Dec 21, 2021

rodesai left a comment

rodesai Jan 4, 2022

feat: add max task usage and num stateful tasks metrics #8549

feat: add max task usage and num stateful tasks metrics #8549

Conversation

lct45 commented Dec 21, 2021 • edited Loading

Description

Testing done

Reviewer checklist

Choose a reason for hiding this comment

lct45 Dec 21, 2021 • edited Loading

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

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

rodesai Dec 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodesai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lct45 commented Dec 21, 2021 •

edited

Loading

lct45 Dec 21, 2021 •

edited

Loading

rodesai Dec 22, 2021 •

edited

Loading