dsmiley commented on code in PR #3671:
URL: https://github.com/apache/solr/pull/3671#discussion_r2376876197
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -556,6 +557,25 @@ public void setName(String v) {
assert this.name != null;
assert coreDescriptor.getCloudDescriptor() == null : "Cores are not
renamed in SolrCloud";
this.name = Objects.requireNonNull(v);
+ initCoreAttributes();
+ }
+
+ public Attributes getCoreAttributes() {
+ return coreAttributes;
+ }
+
+ public void initCoreAttributes() {
Review Comment:
protected or private but not public, right?
##########
solr/core/src/java/org/apache/solr/search/CaffeineCache.java:
##########
@@ -458,60 +464,86 @@ public String getDescription() {
return description;
}
- // for unit tests only
- @VisibleForTesting
- MetricsMap getMetricsMap() {
- return cacheMap;
- }
-
@Override
public SolrMetricsContext getSolrMetricsContext() {
return solrMetricsContext;
}
@Override
public String toString() {
- return name() + (cacheMap != null ? cacheMap.getValue().toString() : "");
Review Comment:
I recall there was a useful feature in the admin UI to observe the contents
of the cache in a debug/diagnostic setting, and I recall it was powered by
this. Maybe I'm a bit off. Any way, fine for it to go for now. If we want it
back, it ought to come back in a better way than this.
##########
solr/core/src/java/org/apache/solr/search/CaffeineCache.java:
##########
@@ -325,13 +330,14 @@ public int size() {
@Override
public void close() throws IOException {
- SolrCache.super.close();
+ IOUtils.closeQuietly(toClose);
Review Comment:
I see the metrics closure is here before cache.invalidateAll & cleanUp,
which at least superficially sound like operations that might trigger metrics
like invalidation. I dunno. Well your tests pass so I suppose not an issue.
##########
solr/core/src/java/org/apache/solr/search/CaffeineCache.java:
##########
@@ -458,60 +464,86 @@ public String getDescription() {
return description;
}
- // for unit tests only
- @VisibleForTesting
- MetricsMap getMetricsMap() {
- return cacheMap;
- }
-
@Override
public SolrMetricsContext getSolrMetricsContext() {
return solrMetricsContext;
}
@Override
public String toString() {
- return name() + (cacheMap != null ? cacheMap.getValue().toString() : "");
+ return name();
}
- // TODO SOLR-17458: Migrate to Otel
@Override
public void initializeMetrics(
SolrMetricsContext parentContext, Attributes attributes, String scope) {
- solrMetricsContext = parentContext.getChildContext(this);
- cacheMap =
- new MetricsMap(
- map -> {
- if (cache != null) {
- CacheStats stats = cache.stats();
- long hitCount = stats.hitCount() + hits.sum();
- long insertCount = inserts.sum();
- long lookupCount = stats.requestCount() + lookups.sum();
-
- map.put(LOOKUPS_PARAM, lookupCount);
- map.put(HITS_PARAM, hitCount);
- map.put(HIT_RATIO_PARAM, hitRate(hitCount, lookupCount));
- map.put(INSERTS_PARAM, insertCount);
- map.put(EVICTIONS_PARAM, stats.evictionCount());
- map.put(SIZE_PARAM, cache.asMap().size());
- map.put("warmupTime", warmupTime);
- map.put(RAM_BYTES_USED_PARAM, ramBytesUsed());
- map.put(MAX_RAM_MB_PARAM, getMaxRamMB());
-
- CacheStats cumulativeStats = priorStats.plus(stats);
- long cumLookups = priorLookups + lookupCount;
- long cumHits = priorHits + hitCount;
- map.put("cumulative_lookups", cumLookups);
- map.put("cumulative_hits", cumHits);
- map.put("cumulative_hitratio", hitRate(cumHits, cumLookups));
- map.put("cumulative_inserts", priorInserts + insertCount);
- map.put("cumulative_evictions",
cumulativeStats.evictionCount());
- }
- });
- solrMetricsContext.gauge(cacheMap, true, scope, getCategory().toString());
+ initializeMetrics(parentContext, attributes, false);
}
- private static double hitRate(long hitCount, long lookupCount) {
- return lookupCount == 0 ? 1.0 : (double) hitCount / lookupCount;
+ public void initializeMetrics(
+ SolrMetricsContext parentContext, Attributes attributes, boolean
isNodeLevelCache) {
+ Attributes cacheAttributes =
+ attributes.toBuilder().put(CATEGORY_ATTR,
getCategory().toString()).build();
+ solrMetricsContext = parentContext.getChildContext(this);
+ String metricPrefix = isNodeLevelCache ? "solr_node_cache" : "solr_cache";
+
+ ObservableLongMeasurement cacheLookupsMetric =
+ solrMetricsContext.longCounterMeasurement(
+ metricPrefix + "_lookups",
+ "Number of cumulative cache lookup results (hits and misses)");
+
+ ObservableLongMeasurement cacheOperationMetric =
+ solrMetricsContext.longCounterMeasurement(
+ metricPrefix + "_ops", "Number of cumulative cache operations
(inserts and evictions)");
+
+ ObservableLongMeasurement sizeMetric =
+ solrMetricsContext.longGaugeMeasurement(
+ metricPrefix + "_size", "Current number cache entries");
+
+ ObservableLongMeasurement ramBytesUsedMetric =
+ solrMetricsContext.longGaugeMeasurement(
+ metricPrefix + "_ram_used", "RAM bytes used by cache",
OtelUnit.BYTES);
+
+ ObservableLongMeasurement warmupTimeMetric =
+ solrMetricsContext.longGaugeMeasurement(
+ metricPrefix + "_warmup_time", "Cache warmup time",
OtelUnit.MILLISECONDS);
Review Comment:
Could clarify "Cache warmup time (most recent)"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
