dsmiley commented on code in PR #3671:
URL: https://github.com/apache/solr/pull/3671#discussion_r2364565335
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -555,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() {
+ this.coreAttributes =
+ (coreContainer.isZooKeeperAware())
+ ? Attributes.builder()
+ .put(COLLECTION_ATTR, coreDescriptor.getCollectionName())
+ .put(CORE_ATTR, getName())
Review Comment:
cores can be renamed (sadly; I hate it). We'll need to rebuild when that
happens.
##########
solr/core/src/java/org/apache/solr/search/CaffeineCache.java:
##########
@@ -471,47 +477,90 @@ public SolrMetricsContext getSolrMetricsContext() {
@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) {
+ Attributes cacheAttributes =
+ attributes.toBuilder().put(CATEGORY_ATTR,
getCategory().toString()).build();
solrMetricsContext = parentContext.getChildContext(this);
- cacheMap =
- new MetricsMap(
- map -> {
+
+ ObservableLongMeasurement cacheOperation =
+ solrMetricsContext.longCounterMeasurement(
+ "solr_searcher_cache_ops",
+ "Number of cache operations (hits, lookups, inserts and
evictions)");
+
+ ObservableLongMeasurement hitRatioMetric =
+
solrMetricsContext.longGaugeMeasurement("solr_searcher_cache_hit_ratio", "Cache
hit ratio");
+
+ ObservableLongMeasurement sizeMetric =
+ solrMetricsContext.longGaugeMeasurement(
+ "solr_searcher_cache_size", "Current number cache entries");
+
+ ObservableLongMeasurement ramBytesUsedMetric =
+ solrMetricsContext.longGaugeMeasurement(
+ "solr_searcher_cache_ram_used", "RAM bytes used by cache",
OtelUnit.BYTES);
+
+ ObservableLongMeasurement warmupTimeMetric =
+ solrMetricsContext.longGaugeMeasurement(
+ "solr_searcher_cache_warmup_time", "Cache warmup time",
OtelUnit.MILLISECONDS);
+
+ ObservableLongMeasurement cumulativeCacheOperation =
+ solrMetricsContext.longGaugeMeasurement(
+ "solr_cache_cumulative_ops",
+ "Number of cumulative cache operations (hits, lookups, inserts and
evictions)");
+
+ this.toClose =
+ solrMetricsContext.batchCallback(
+ () -> {
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());
+ cacheOperation.record(
+ hitCount, cacheAttributes.toBuilder().put(OPERATION_ATTR,
"hits").build());
Review Comment:
this is the result/outcome of an operation, not an operation.
It could be interesting to have two pseudo-operations -- lookups_hit and
lookups_miss or put the outcomes into a separate attribute like "hit" (1 or 0).
##########
solr/core/src/test/org/apache/solr/core/TestSolrConfigHandler.java:
##########
@@ -1045,20 +1046,19 @@ public void testSetPropertyEnableAndDisableCache()
throws Exception {
String payload = "{'set-property' : { 'query.documentCache.enabled': true}
}";
runConfigCommand(restTestHarness, "/config", payload);
restTestHarness.setServerProvider(() -> getBaseUrl());
- MapWriter confMap = getRespMap("/admin/metrics", restTestHarness);
- assertNotNull(
- confMap._get(
- asList("metrics", "solr.core.collection1",
"CACHE.searcher.documentCache"), null));
+
+ String prometheusMetrics =
restTestHarness.query("/admin/metrics?wt=prometheus");
+ assertTrue(prometheusMetrics.contains("cache_name=\"documentCache\""));
// Disabling Cache
payload = "{ 'set-property' : { 'query.documentCache.enabled': false } }";
restTestHarness.setServerProvider(oldProvider);
+
runConfigCommand(restTestHarness, "/config", payload);
restTestHarness.setServerProvider(() -> getBaseUrl());
- confMap = getRespMap("/admin/metrics", restTestHarness);
- assertNull(
- confMap._get(
- asList("metrics", "solr.core.collection1",
"CACHE.searcher.documentCache"), null));
+
+ prometheusMetrics = restTestHarness.query("/admin/metrics?wt=prometheus");
Review Comment:
this if fine but isn't prometheus always the format if not specified
henceforth?
##########
solr/core/src/java/org/apache/solr/search/CaffeineCache.java:
##########
@@ -471,47 +477,90 @@ public SolrMetricsContext getSolrMetricsContext() {
@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) {
+ Attributes cacheAttributes =
+ attributes.toBuilder().put(CATEGORY_ATTR,
getCategory().toString()).build();
solrMetricsContext = parentContext.getChildContext(this);
- cacheMap =
- new MetricsMap(
- map -> {
+
+ ObservableLongMeasurement cacheOperation =
+ solrMetricsContext.longCounterMeasurement(
+ "solr_searcher_cache_ops",
+ "Number of cache operations (hits, lookups, inserts and
evictions)");
+
+ ObservableLongMeasurement hitRatioMetric =
+
solrMetricsContext.longGaugeMeasurement("solr_searcher_cache_hit_ratio", "Cache
hit ratio");
+
+ ObservableLongMeasurement sizeMetric =
+ solrMetricsContext.longGaugeMeasurement(
+ "solr_searcher_cache_size", "Current number cache entries");
Review Comment:
I see you are putting "searcher" in the name yet CaffeineCache could be used
anywhere.
##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -603,9 +604,10 @@ public void register() {
this.solrMetricsContext =
core.getSolrMetricsContext().getChildContext(this);
for (SolrCache<?, ?> cache : cacheList) {
cache.initializeMetrics(
- // TODO SOLR-17458: Add Otel
solrMetricsContext,
- Attributes.empty(),
+ core.getCoreAttributes().toBuilder()
+ .put(AttributeKey.stringKey("cache_name"), cache.name())
Review Comment:
would the "cache_" prefix be redundant?
##########
solr/core/src/java/org/apache/solr/search/CaffeineCache.java:
##########
@@ -326,6 +331,7 @@ public int size() {
@Override
public void close() throws IOException {
SolrCache.super.close();
Review Comment:
Generally, closure works like a stack, so we close super _last_, not first.
I know you didn't touch that but...
##########
solr/core/src/test/org/apache/solr/CursorPagingTest.java:
##########
@@ -715,30 +715,19 @@ public void testCacheImpacts() throws Exception {
final Collection<String> allFieldNames = getAllSortFieldNames();
- final MetricsMap filterCacheStats =
- (MetricsMap)
- ((SolrMetricManager.GaugeWrapper)
- h.getCore()
- .getCoreMetricManager()
- .getRegistry()
- .getMetrics()
- .get("CACHE.searcher.filterCache"))
- .getGauge();
- assertNotNull(filterCacheStats);
- final MetricsMap queryCacheStats =
- (MetricsMap)
- ((SolrMetricManager.GaugeWrapper)
- h.getCore()
- .getCoreMetricManager()
- .getRegistry()
- .getMetrics()
- .get("CACHE.searcher.queryResultCache"))
- .getGauge();
- assertNotNull(queryCacheStats);
-
- final long preQcIn = (Long) queryCacheStats.getValue().get("inserts");
- final long preFcIn = (Long) filterCacheStats.getValue().get("inserts");
- final long preFcHits = (Long) filterCacheStats.getValue().get("hits");
+ SolrCore solrCore = h.getCore();
+
+ // Get initial metrics for filter and query cache
+ var preFilterInserts =
+ SolrMetricTestUtils.getCacheSearcherOpsInserts(solrCore,
SolrMetricTestUtils.FILTER_CACHE)
Review Comment:
nice; these will be useful across a number of tests
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
