mlbiscoc commented on code in PR #3458:
URL: https://github.com/apache/solr/pull/3458#discussion_r2263737412
##########
solr/core/src/java/org/apache/solr/metrics/SolrCoreMetricManager.java:
##########
@@ -52,6 +53,11 @@ public class SolrCoreMetricManager implements Closeable {
private String leaderRegistryName;
private boolean cloudMode;
+ // Track all metric producers registered for this core so we can
re-initialize them during rename
+ private final List<MetricProducerInfo> registeredProducers = new
ArrayList<>();
Review Comment:
In Dropwizard, the registries only needed to be renamed on which worked
because nothing had labels. Not possible in OTEL. We need to track our metric
producers so we can re-register and reinitialize its metrics with the correct
core name for labels.
##########
solr/core/src/java/org/apache/solr/metrics/SolrCoreMetricManager.java:
##########
@@ -147,11 +158,12 @@ public void registerMetricProducer(String scope,
SolrMetricProducer producer) {
+ producer);
}
- // NOCOMMIT SOLR-17458: These attributes may not work for standalone mode
and maybe make the
Review Comment:
Turns out you try putting a `null` value for a label it just gets ignored so
this always worked in standalone mode.
##########
solr/core/src/test/org/apache/solr/metrics/SolrMetricsIntegrationTest.java:
##########
Review Comment:
This was a headache to migrate. Will need to come back to finish it later
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/NodesSysPropsCacher.java:
##########
@@ -74,6 +74,9 @@ public Map<String, Object> getSysProps(String nodeName,
Collection<String> tags)
return result;
}
+ // NOCOMMIT: These properties were fetched from the /admin/metrics endpoint.
These properties were
+ // stored as strings instead of numeric values. This is not possible in OTEL
metrics. Need to
+ // revisit this later.
private Map<String, Object> fetchProps(String nodeName, Collection<String>
tags) {
Review Comment:
Not sure if these properties can be retrieved from somewhere else but it
cannot be from the /admin/metrics endpoint. Metrics are only numeric values
that can be aggregated so it didn't really make sense that the metrics had
strings as values. Is there a some endpoint available to get these system props
instead?
##########
solr/core/src/java/org/apache/solr/metrics/SolrMetricManager.java:
##########
@@ -755,68 +755,15 @@ private static MetricRegistry getOrCreateRegistry(
*
* @param registry name of the registry to remove
*/
- // TODO SOLR-17458: You can't delete OTEL meters
public void removeRegistry(String registry) {
- // NOCOMMIT Remove all closing Dropwizard registries
- // close any reporters for this registry first
- closeReporters(registry, null);
- // make sure we use a name with prefix
- registry = enforcePrefix(registry);
- if (isSharedRegistry(registry)) {
- SharedMetricRegistries.remove(registry);
- } else {
- swapLock.lock();
- try {
- registries.remove(registry);
- } finally {
- swapLock.unlock();
- }
- }
meterProviderAndReaders.computeIfPresent(
- registry,
+ enforcePrefix(registry),
(key, meterAndReader) -> {
meterAndReader.sdkMeterProvider().close();
return null;
});
}
- /**
- * Swap registries. This is useful eg. during {@link
org.apache.solr.core.SolrCore} rename or swap
- * operations. NOTE: this operation is not supported for shared registries.
- *
- * @param registry1 source registry
- * @param registry2 target registry. Note: when used after core rename the
target registry doesn't
- * exist, so the swap operation will only rename the existing registry
without creating an
- * empty one under the previous name.
- */
- // NOCOMMIT SOLR-17458: Don't think we need
- public void swapRegistries(String registry1, String registry2) {
Review Comment:
No such thing as "swapping or renaming registries" just recreating now.
##########
solr/core/src/test/org/apache/solr/metrics/SolrMetricTestUtils.java:
##########
@@ -193,4 +230,86 @@ public static HistogramSnapshot.HistogramDataPointSnapshot
getHistogramDatapoint
return getDatapoint(
core, metricName, labels,
HistogramSnapshot.HistogramDataPointSnapshot.class);
}
+
+ public static CounterSnapshot.CounterDataPointSnapshot
getStandaloneSelectRequestsDatapoint(
Review Comment:
I found myself very often using `select` and `update` requests as easy ways
of asserting these registries were getting updated values. Just created these
util methods to help. Will probably go back at the other tests that checked
these and just call this instead of constantly recreating the same set of
labels.
--
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]
