dsmiley commented on code in PR #2902:
URL: https://github.com/apache/solr/pull/2902#discussion_r1893230595
##########
solr/core/src/java/org/apache/solr/metrics/prometheus/core/SolrCoreCacheMetric.java:
##########
@@ -38,7 +37,7 @@ public SolrCoreCacheMetric(
public SolrCoreMetric parseLabels() {
String[] parsedMetric = metricName.split("\\.");
if (dropwizardMetric instanceof Gauge) {
- labels.put("cacheType", parsedMetric[2]);
+ labels.put("cacheType", parsedMetric[3]);
Review Comment:
Previously index 2 lined up with the sample above on line 33. But now; I
dunno. I suppose your data example string is now out of date.
Same comment as other source files you edited similarly.
##########
solr/core/src/java/org/apache/solr/handler/admin/MetricsHandler.java:
##########
@@ -186,10 +187,22 @@ private NamedList<Object>
handlePrometheusExport(SolrParams params) {
List<MetricType> metricTypes = parseMetricTypes(params);
List<MetricFilter> metricFilters =
metricTypes.stream().map(MetricType::asMetricFilter).collect(Collectors.toList());
+
Set<String> requestedRegistries = parseRegistries(params);
+ MetricRegistry mergedCoreRegistries = new MetricRegistry();
for (String registryName : requestedRegistries) {
MetricRegistry dropwizardRegistry = metricManager.registry(registryName);
+
+ // Merge all core registries into a single registry and
+ // append the core name to the metric to avoid duplicate metrics name
+ if (registryName.startsWith("solr.core")) {
+ mergedCoreRegistries.registerAll(
+
Arrays.stream(registryName.split("\\.")).skip(1).collect(Collectors.joining("_")),
Review Comment:
It'd be faster to do a substring on the index after the first period, and
then with the result of that, replace all periods with underscores. Having a
little utility method to do this with comment would ultimately be clearer IMO
##########
solr/core/src/java/org/apache/solr/metrics/prometheus/core/SolrCoreMetric.java:
##########
@@ -25,28 +26,26 @@
/** Base class is a wrapper to export a solr.core {@link
com.codahale.metrics.Metric} */
public abstract class SolrCoreMetric extends SolrMetric {
+
public String coreName;
- public SolrCoreMetric(
- Metric dropwizardMetric, String coreName, String metricName, boolean
cloudMode) {
+ public SolrCoreMetric(Metric dropwizardMetric, String metricName) {
super(dropwizardMetric, metricName);
- this.coreName = coreName;
- labels.put("core", coreName);
- if (cloudMode) {
- appendCloudModeLabels();
- }
- }
-
- private void appendCloudModeLabels() {
- Matcher m = CLOUD_CORE_PATTERN.matcher(coreName);
- if (m.find()) {
- labels.put("collection", m.group(1));
- labels.put("shard", m.group(2));
- labels.put("replica", m.group(3));
+ Matcher cloudPattern = CLOUD_CORE_PATTERN.matcher(metricName);
+ Matcher standalonePattern = STANDALONE_CORE_PATTERN.matcher(metricName);
+ if (cloudPattern.find()) {
+ coreName = cloudPattern.group(1);
+ labels.put("core", cloudPattern.group(1));
+ labels.put("collection", cloudPattern.group(2));
+ labels.put("shard", cloudPattern.group(3));
+ labels.put("replica", cloudPattern.group(4));
Review Comment:
I believe it's possible to have named groups in a regular expression. If
you enhance your regular expressions as such, you could generically apply the
pattern with Matcher and extract all named groups without hard-coding the
expectations separate from the regular expression, which is annoying and
error-prone to maintain.
--
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]
