dsmiley commented on code in PR #3458:
URL: https://github.com/apache/solr/pull/3458#discussion_r2264034281
##########
solr/core/src/test/org/apache/solr/metrics/SolrMetricTestUtils.java:
##########
@@ -68,6 +69,42 @@ public static Map<String, Counter> getRandomMetrics(Random
random, boolean shoul
return shouldDefineMetrics ? getRandomMetricsWithReplacements(random, new
HashMap<>()) : null;
}
+ /**
+ * Generate random OpenTelemetry metric names for testing Prometheus
metrics. Returns a map of
+ * metric names to their expected increment values.
+ */
+ public static Map<String, Long> getRandomPrometheusMetrics(Random random) {
+ return getRandomPrometheusMetrics(random, random.nextBoolean());
+ }
+
+ public static Map<String, Long> getRandomPrometheusMetrics(
Review Comment:
this looks useless. Pass a false and it always return null. Who would
explicitly call it as such? If it only exists for the random case above then
inline it (which IntelliJ can do in a single action).
##########
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:
I suggest improving this comment to say "**core** rename". "rename" alone
is ambiguous.
##########
solr/core/src/java/org/apache/solr/metrics/SolrCoreMetricManager.java:
##########
@@ -105,30 +111,35 @@ public void loadReporters() {
}
/**
- * Make sure that metrics already collected that correspond to the old core
name are carried over
- * and will be used under the new core name. This method also reloads
reporters so that they use
- * the new core name.
+ * Re-register all metric producers associated with this core. This
recreates the metric registry
+ * resetting its state
*/
- // NOCOMMIT SOLR-17458: Update for core renaming
- public void afterCoreRename() {
- assert core.getCoreDescriptor().getCloudDescriptor() == null;
- String oldRegistryName = solrMetricsContext.getRegistryName();
- String oldLeaderRegistryName = leaderRegistryName;
- String newRegistryName =
- createRegistryName(cloudMode, collectionName, shardName, replicaName,
core.getName());
- leaderRegistryName = createLeaderRegistryName(cloudMode, collectionName,
shardName);
- if (oldRegistryName.equals(newRegistryName)) {
- return;
- }
- // close old reporters
- metricManager.closeReporters(oldRegistryName, solrMetricsContext.getTag());
- if (oldLeaderRegistryName != null) {
- metricManager.closeReporters(oldLeaderRegistryName,
solrMetricsContext.getTag());
- }
- solrMetricsContext =
- new SolrMetricsContext(metricManager, newRegistryName,
solrMetricsContext.getTag());
- // load reporters again, using the new core name
- loadReporters();
+ public void reregisterCoreMetrics() {
Review Comment:
I would have hoped that removing then adding metrics allows us to use the
very same code we already have for those separate things.
##########
solr/core/src/test/org/apache/solr/metrics/SolrMetricTestUtils.java:
##########
@@ -68,6 +69,42 @@ public static Map<String, Counter> getRandomMetrics(Random
random, boolean shoul
return shouldDefineMetrics ? getRandomMetricsWithReplacements(random, new
HashMap<>()) : null;
}
+ /**
+ * Generate random OpenTelemetry metric names for testing Prometheus
metrics. Returns a map of
+ * metric names to their expected increment values.
+ */
+ public static Map<String, Long> getRandomPrometheusMetrics(Random random) {
+ return getRandomPrometheusMetrics(random, random.nextBoolean());
+ }
+
+ public static Map<String, Long> getRandomPrometheusMetrics(
+ Random random, boolean shouldDefineMetrics) {
+ return shouldDefineMetrics
+ ? getRandomPrometheusMetricsWithReplacements(random, new HashMap<>())
+ : null;
+ }
+
+ public static Map<String, Long> getRandomPrometheusMetricsWithReplacements(
+ Random random, Map<String, Long> existing) {
+ HashMap<String, Long> metrics = new HashMap<>();
+ ArrayList<String> existingKeys = new ArrayList<>(existing.keySet());
+
+ int numMetrics = TestUtil.nextInt(random, 1, MAX_ITERATIONS);
+ for (int i = 0; i < numMetrics; ++i) {
+ boolean shouldReplaceMetric = !existing.isEmpty() &&
random.nextBoolean();
+ String name =
+ shouldReplaceMetric
+ ? existingKeys.get(TestUtil.nextInt(random, 0,
existingKeys.size() - 1))
+ : TestUtil.randomSimpleString(random, 5, 10)
+ + SUFFIX; // must be simple string for JMX publishing
+
+ Long incrementValue = Math.abs(random.nextLong() % 1000) + 1;
Review Comment:
TextUtil.nextLong
##########
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:
This is unfortunate. CC @sigram
For system properties in particular,
http://localhost:8983/solr/admin/info/properties which you probably recall
visualized from Solr's admin UI. This change (with remedy) ought to be
mentioned in the upgrade guide so people know.
##########
solr/core/src/test/org/apache/solr/metrics/SolrMetricTestUtils.java:
##########
@@ -68,6 +69,42 @@ public static Map<String, Counter> getRandomMetrics(Random
random, boolean shoul
return shouldDefineMetrics ? getRandomMetricsWithReplacements(random, new
HashMap<>()) : null;
}
+ /**
+ * Generate random OpenTelemetry metric names for testing Prometheus
metrics. Returns a map of
+ * metric names to their expected increment values.
+ */
+ public static Map<String, Long> getRandomPrometheusMetrics(Random random) {
+ return getRandomPrometheusMetrics(random, random.nextBoolean());
+ }
+
+ public static Map<String, Long> getRandomPrometheusMetrics(
+ Random random, boolean shouldDefineMetrics) {
+ return shouldDefineMetrics
+ ? getRandomPrometheusMetricsWithReplacements(random, new HashMap<>())
+ : null;
+ }
+
+ public static Map<String, Long> getRandomPrometheusMetricsWithReplacements(
+ Random random, Map<String, Long> existing) {
+ HashMap<String, Long> metrics = new HashMap<>();
+ ArrayList<String> existingKeys = new ArrayList<>(existing.keySet());
+
+ int numMetrics = TestUtil.nextInt(random, 1, MAX_ITERATIONS);
+ for (int i = 0; i < numMetrics; ++i) {
+ boolean shouldReplaceMetric = !existing.isEmpty() &&
random.nextBoolean();
+ String name =
+ shouldReplaceMetric
+ ? existingKeys.get(TestUtil.nextInt(random, 0,
existingKeys.size() - 1))
Review Comment:
if existing is empty; this would probably fail
##########
solr/core/src/test/org/apache/solr/metrics/SolrMetricManagerTest.java:
##########
@@ -64,48 +64,6 @@ public void setUp() throws Exception {
this.reader = metricManager.getPrometheusMetricReader(METER_PROVIDER_NAME);
}
- // NOCOMMIT: We might not be supported core swapping in 10. Maybe remove
this test
Review Comment:
well we need _some_ test for the changes you did
##########
solr/core/src/test/org/apache/solr/metrics/SolrMetricTestUtils.java:
##########
@@ -68,6 +69,42 @@ public static Map<String, Counter> getRandomMetrics(Random
random, boolean shoul
return shouldDefineMetrics ? getRandomMetricsWithReplacements(random, new
HashMap<>()) : null;
}
+ /**
+ * Generate random OpenTelemetry metric names for testing Prometheus
metrics. Returns a map of
+ * metric names to their expected increment values.
+ */
+ public static Map<String, Long> getRandomPrometheusMetrics(Random random) {
+ return getRandomPrometheusMetrics(random, random.nextBoolean());
+ }
+
+ public static Map<String, Long> getRandomPrometheusMetrics(
+ Random random, boolean shouldDefineMetrics) {
+ return shouldDefineMetrics
+ ? getRandomPrometheusMetricsWithReplacements(random, new HashMap<>())
+ : null;
+ }
+
+ public static Map<String, Long> getRandomPrometheusMetricsWithReplacements(
+ Random random, Map<String, Long> existing) {
+ HashMap<String, Long> metrics = new HashMap<>();
+ ArrayList<String> existingKeys = new ArrayList<>(existing.keySet());
Review Comment:
I suggest `List.copyOf(...)` here
##########
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'd use `new` in place of `get` as applicable.
--
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]
