mlbiscoc commented on code in PR #3671:
URL: https://github.com/apache/solr/pull/3671#discussion_r2369757819
##########
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:
Just made it name then.
##########
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:
I rebuilt it in the `setName` method right above this. I found a test that
failed so I caught the rename and test passes now
##########
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:
Ok so I split into 2 metrics. `solr_cache_lookups` and `solr_cache_ops`.
`solr_cache_lookups` will have a `result` attirbute of `hit/miss` and if
missing `result` is just total lookups. WDYT?
##########
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:
It will always be prometheus but right now this branch does both dropwizard
and prometheus because the number of tests failing while doing incremental
migration made it difficult to track changes. Once I remove Dropwizard, going
to default it to prometheus and remove all the `wt=prometheus` parameters. I
placed NOCOMMITS around other tests to remember.
--
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]
