add StorageMonitor to measure storage and virtual storage usage by segment cache by clintropolis · Pull Request #18742 · apache/druid

clintropolis · 2025-11-14T00:39:27Z

Description

To use, add org.apache.druid.server.metrics.StorageMonitor to druid.monitoring.monitors.

changes:

adds StorageLocationStats and VirtualStorageLocationStats to abstract measuring segment cache/virtual segment cache usage
adds StorageStats to collect StorageLocationStats and VirtualStorageLocationStats into maps between location label and the stats of that location
adds new method SegmentCacheManager.getStorageStats to expose stats collection for segment cache manager implementations
adds new StorageMonitor to emit metrics for values in StorageStats
metrics for StorageLocationStats:
- storage/used/bytes
- storage/load/count
- storage/load/bytes
- storage/drop/count
- storage/drop/bytes
metrics for VirtualStorageLocationStats:
- storage/virtual/used/bytes
- storage/virtual/hit/count
- storage/virtual/load/count
- storage/virtual/load/bytes
- storage/virtual/evict/count
- storage/virtual/evict/bytes
- storage/virtual/reject/count

…gment cache changes: * adds `StorageLocationStats` and `VirtualStorageLocationStats` to abstract measuring segment cache/virtual segment cache usage * adds `StorageStats` to collect `StorageLocationStats` and `VirtualStorageLocationStats` into maps between location label and the stats of that location * adds new method `SegmentCacheManager.getStorageStats` to expose stats collection for segment cache manager implementations * adds new `StorageMonitor` to emit metrics for values in `StorageStats` * metrics for `StorageLocationStats`: - storage/static/count - storage/static/bytes - storage/drop/count - storage/drop/bytes * metrics for `VirtualStorageLocationStats`: - storage/virtual/hit/count - storage/virtual/load/count - storage/virtual/load/bytes - storage/virtual/evict/count - storage/virtual/evict/bytes - storage/virtual/reject/count

kfaraz

Thanks for adding the metrics, @clintropolis !
Left some suggestions/queries, mostly for my understanding.

kfaraz · 2025-11-14T08:55:40Z

+        builder.setMetric(VSF_HIT_COUNT, weakStats.getHitCount());
+        emitter.emit(builder);


Nit: Maybe combine these lines for brevity (unless you prefer the current syntax more)

Suggested change

builder.setMetric(VSF_HIT_COUNT, weakStats.getHitCount());

emitter.emit(builder);

emitter.emit(builder.setMetric(VSF_HIT_COUNT, weakStats.getHitCount()));

kfaraz · 2025-11-14T09:11:37Z

+
+    sleepForStorageMonitor();
+
+    emitter.waitForAnyEventWithMetricName(StorageMonitor.VSF_HIT_COUNT);


Nit: Do we need the new waitForAnyEventWithMetricName method?

Does the original syntax seem verbose?

Suggested change

emitter.waitForAnyEventWithMetricName(StorageMonitor.VSF_HIT_COUNT);

emitter.waitForEvent(event -> event.hasMetricName(StorageMonitor.VSF_HIT_COUNT));

Just wondering if we there is something we can do to make the syntax more dev-friendly for the general case too, since matching on just the metric name doesn't seem like a common use case.

yea, i guess this method isn't necessary, i guess i was thinking it would be common for looking for things from monitors like i'm doing here, but i guess it doesn't save all that much

Yeah, I think we can leave it out for now.

kfaraz · 2025-11-14T12:03:02Z

+   * returned by this method only includes the events which have happened since the previous call to this method.
+   */
+  @Nullable
+  StorageStats getStorageStats();


Maybe rename to getAndResetStorageStats() to align with behaviour?

kfaraz · 2025-11-14T12:04:51Z

+    return events.get(lastIndex).getValue().longValue();
+  }
+
+  private long getMetricTotal(LatchableEmitter emitter, String metricName)


I suppose this should be a utility method on StubServiceEmitter or MetricsVerifier itself. But we can do that later too.

kfaraz · 2025-11-14T12:08:41Z

+    LatchableEmitter emitter = historical.latchableEmitter();
+    // sleep to clear out the pipe to get zerod out storage monitor metrics and then flush (which clears out the
+    // internal events stores in test emitter)
+    sleepForStorageMonitor();


Can we avoid the sleep somehow? Maybe by waiting for some specific metric to be emitted?

i had some trouble getting waiting for events from the same monitor to work, looking into the code i think i see why, waitForEvent doesn't necessarily wait for the next event, it finds any processed matching event too, i believe i misunderstood what this method did, I think i need a version that skips any already emitted events and only waits for the next one

I think i need a version that skips any already emitted events and only waits for the next one

I see. That seems like a reasonable use case. I think you can add a method waitForNextEvent but we would need to call out that it would be susceptible to race conditions and may miss out on events and cause the test to wait forever.

this is more or less exactly what i have done, just haven't got around to pushing up a commit yet tidying some other stuff up

kfaraz · 2025-11-14T12:11:54Z

+  long getLoadCount();
+
+  long getLoadBytes();


Does this represent the load events that have occurred since the last emission period?
Or the number/bytes of segments currently loaded at a location?

The latter seems like a useful metric too. We already have the segment/used and segment/usedPercent metrics emitted by the HistoricalMetricsMonitor, but they are not keyed on the location as a dimension.

right now these metrics are all just for the emission period. I was aiming for these metrics to show like the activity during the period to give a good idea of segment cache activity rather than overall usage, the static load metrics showing churn in rebalancing, while the vsf metrics could be used as an indicator that there is a lot of churn from queries targeting different segments competing for disk space.

That said, it does feel useful to have some metrics like you are suggesting as well, especially later once we can have mixed disk usage from virtual loads via load rules... will think on it a bit.

kfaraz · 2025-11-14T12:18:45Z

+
+package org.apache.druid.segment.loading;
+
+public interface VirtualStorageLocationStats


Should this extend StorageLocationStats?

they don't quite line up, there is not an explicit 'drop' in virtual storage, instead they are always evicted when something else needs the space, so i had them separate so the names are clearer to their function. I also didn't see a lot of usefulness in them being the same type, since they don't emit the same metrics, the only thing they both have in common is a location dimension and loads. I guess later when load rules would allow mixed usage of disk it might be nice to have one object per location, but it would also need to distinguish static loads from virtual loads, so need to think about it a bit.

went ahead and added used bytes, which unlike the others is just the level at the time of emission

kfaraz · 2025-11-14T12:19:56Z

+    this.stats = stats;
+    this.virtualStats = virtualStats;


Can we ever have a case where both maps are non-empty i.e. some static storage locations and some virtual on the same service?

not right now, we could in the future once we support defining virtual loads via load rules instead of the 'all or nothing' virtual storage mode we have right now

Makes sense, thanks for the clarification!

kfaraz · 2025-11-19T14:12:51Z

+  public Number getLatestMetricEventValue(String metricName)
+  {
+    final Deque<ServiceMetricEvent> metricEventQueue = metricEvents.get(metricName);
+    return metricEventQueue == null ? 0 : metricEventQueue.getLast().getValue();


Nit: should we return null if there has been no event?

divy-imply · 2025-11-25T19:29:29Z

👋 Can we add a plain text mapping to metric name to meaning.
naive example:
storage/used/bytes : data stored in hot tier/cache

by reading the PR description, i understand these metrics are per segment usage. how does it translate to tier labeled as hot?

clintropolis · 2025-12-02T18:52:27Z

👋 Can we add a plain text mapping to metric name to meaning.
naive example:
storage/used/bytes : data stored in hot tier/cache

by reading the PR description, i understand these metrics are per segment usage. how does it translate to tier labeled as hot?

Well, there isn't a really clear way to do this since tiers are just labels, so there isn't really a concept of 'hot', that is something operators decide depending on configurations and such. Also, in the future I plan to support specifying virtual loads via load rules instead of a system level configuration, so at that point the same node could be emitting both regular and virtual metrics, so trying to frame these metrics in terms of tiers might be confusing.

clintropolis added Area - Metrics/Event Emitting jacoco:skip labels Nov 14, 2025

clintropolis added 2 commits November 13, 2025 17:43

fix space

3919bcb

more style

ab13d9f

kfaraz reviewed Nov 14, 2025

View reviewed changes

clintropolis added 2 commits November 18, 2025 03:23

adjustments

9d29a44

Merge remote-tracking branch 'upstream/master' into storage-monitor

3b6ee50

kfaraz approved these changes Nov 19, 2025

View reviewed changes

clintropolis added 2 commits November 25, 2025 11:08

null latest value if not present

04ed591

Merge remote-tracking branch 'upstream/master' into storage-monitor

af2396a

clintropolis merged commit 5626213 into apache:master Dec 2, 2025
57 checks passed

clintropolis deleted the storage-monitor branch December 2, 2025 23:24

kgyrtkirk added this to the 36.0.0 milestone Jan 19, 2026

clintropolis mentioned this pull request Feb 24, 2026

always use StorageMonitor, also make StorageMonitor work for MSQ tasks #19048

Merged

		builder.setMetric(VSF_HIT_COUNT, weakStats.getHitCount());
		emitter.emit(builder);

	builder.setMetric(VSF_HIT_COUNT, weakStats.getHitCount());
	emitter.emit(builder);
	emitter.emit(builder.setMetric(VSF_HIT_COUNT, weakStats.getHitCount()));


		sleepForStorageMonitor();

		emitter.waitForAnyEventWithMetricName(StorageMonitor.VSF_HIT_COUNT);

	emitter.waitForAnyEventWithMetricName(StorageMonitor.VSF_HIT_COUNT);
	emitter.waitForEvent(event -> event.hasMetricName(StorageMonitor.VSF_HIT_COUNT));


		package org.apache.druid.segment.loading;

		public interface VirtualStorageLocationStats

Uh oh!

Conversation

clintropolis commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

kfaraz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

divy-imply commented Nov 25, 2025

Uh oh!

clintropolis commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

clintropolis commented Nov 14, 2025 •

edited

Loading

clintropolis commented Dec 2, 2025 •

edited

Loading