mlbiscoc commented on code in PR #3458:
URL: https://github.com/apache/solr/pull/3458#discussion_r2266979434
##########
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:
We can't remove metrics from an existing registry after creation so we have
to just completely tear it down and recreate it.
##########
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:
`!existing.isEmpty()` is checked right above so it shouldn't fail.
##########
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:
Did you mean TestUtil.nextLong? Updated to that.
##########
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:
SolrMetricManager doesn't have a public swap registries method anymore to
unit test. I tested the changes at a higher level you can see in
`TestCoreAdmin` which does an initial metrics check -> swap and check
registries reset to 0 -> select and check the registries are still updating
according to correct cores.
--
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]
