dsmiley commented on code in PR #3734:
URL: https://github.com/apache/solr/pull/3734#discussion_r2417957584
##########
solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java:
##########
@@ -167,83 +165,75 @@ public SolrMetricsContext getSolrMetricsContext() {
@Override
public void initializeMetrics(
SolrMetricsContext parentContext, Attributes attributes, String scope) {
- if (aggregateNodeLevelMetricsEnabled) {
- this.solrMetricsContext =
- new SolrDelegateRegistryMetricsContext(
- parentContext.getMetricManager(),
- parentContext.getRegistryName(),
- SolrMetricProducer.getUniqueMetricTag(this,
parentContext.getTag()),
- SolrMetricManager.getRegistryName(SolrInfoBean.Group.node));
- } else {
- this.solrMetricsContext = parentContext.getChildContext(this);
- }
+ this.solrMetricsContext = parentContext.getChildContext(this);
metrics =
new HandlerMetrics(
solrMetricsContext,
- attributes.toBuilder().put(CATEGORY_ATTR,
getCategory().toString()).build());
+ attributes.toBuilder().put(CATEGORY_ATTR,
getCategory().toString()).build(),
+ aggregateNodeLevelMetricsEnabled);
}
/** Metrics for this handler. */
public static class HandlerMetrics {
public static final HandlerMetrics NO_OP =
new HandlerMetrics(
new SolrMetricsContext(new SolrMetricManager(null), "NO_OP",
"NO_OP"),
- Attributes.empty());
+ Attributes.empty(),
+ false);
public AttributedLongCounter requests;
public AttributedLongCounter numServerErrors;
public AttributedLongCounter numClientErrors;
public AttributedLongCounter numTimeouts;
public AttributedLongTimer requestTimes;
- public HandlerMetrics(SolrMetricsContext solrMetricsContext, Attributes
attributes) {
- LongCounter requestMetric;
- LongCounter errorRequestMetric;
- LongCounter timeoutRequestMetric;
- LongHistogram requestTimeMetric;
-
- if (solrMetricsContext.getRegistryName().equals("solr.node")) {
- requestMetric =
- solrMetricsContext.longCounter("solr_node_requests", "Http Solr
node requests");
- errorRequestMetric =
- solrMetricsContext.longCounter(
- "solr_node_requests_errors", "HTTP Solr node request errors");
- timeoutRequestMetric =
- solrMetricsContext.longCounter(
- "solr_node_requests_timeout", "HTTP Solr node request
timeouts");
- requestTimeMetric =
- solrMetricsContext.longHistogram(
- "solr_node_requests_times", "HTTP Solr node request times",
OtelUnit.MILLISECONDS);
- } else {
- requestMetric =
- solrMetricsContext.longCounter("solr_core_requests", "HTTP Solr
core requests");
- errorRequestMetric =
- solrMetricsContext.longCounter(
- "solr_core_requests_errors", "HTTP Solr core request errors");
- timeoutRequestMetric =
- solrMetricsContext.longCounter(
- "solr_core_requests_timeout", "HTTP Solr core request
timeouts");
- requestTimeMetric =
- solrMetricsContext.longHistogram(
- "solr_core_requests_times", "HTTP Solr core request times",
OtelUnit.MILLISECONDS);
- }
+ public HandlerMetrics(
+ SolrMetricsContext solrMetricsContext,
+ Attributes coreAttributes,
+ boolean aggregateNodeLevelMetricsEnabled) {
- requests = new AttributedLongCounter(requestMetric, attributes);
+ AttributedInstrumentFactory factory =
+ new AttributedInstrumentFactory(
+ solrMetricsContext, coreAttributes,
aggregateNodeLevelMetricsEnabled);
- numServerErrors =
- new AttributedLongCounter(
- errorRequestMetric,
- attributes.toBuilder().put(AttributeKey.stringKey("source"),
"server").build());
+ String corePrefix = "solr_core_";
+ String nodePrefix = "solr_node_";
- numClientErrors =
- new AttributedLongCounter(
- errorRequestMetric,
- attributes.toBuilder().put(AttributeKey.stringKey("source"),
"client").build());
+ requests =
+ factory.attributedLongCounter(
+ AttributedInstrumentFactory.standardNameProvider(corePrefix,
nodePrefix, "requests"),
+ "HTTP Solr requests",
+ Attributes.empty());
- numTimeouts = new AttributedLongCounter(timeoutRequestMetric,
attributes);
+ numServerErrors =
+ factory.attributedLongCounter(
+ AttributedInstrumentFactory.standardNameProvider(
+ corePrefix, nodePrefix, "requests_errors"),
+ "HTTP Solr request errors",
+ Attributes.of(SOURCE_ATTR, "server"));
- requestTimes = new AttributedLongTimer(requestTimeMetric, attributes);
+ numClientErrors =
+ factory.attributedLongCounter(
+ AttributedInstrumentFactory.standardNameProvider(
+ corePrefix, nodePrefix, "requests_errors"),
+ "HTTP Solr request errors",
+ Attributes.of(SOURCE_ATTR, "client"));
+
+ numTimeouts =
+ factory.attributedLongCounter(
+ AttributedInstrumentFactory.standardNameProvider(
+ corePrefix, nodePrefix, "requests_timeout"),
+ "HTTP Solr request timeouts",
+ Attributes.empty());
+
+ requestTimes =
+ factory.attributedLongTimer(
+ AttributedInstrumentFactory.standardNameProvider(
+ "solr_core_", nodePrefix, "requests_times"),
Review Comment:
here you use a string literal instead of `corePrefix`. But moreover, if the
factory knows the choice (it has the boolean), then can't it provide the proper
prefix instead of each one of these metric instantiations having to pass both?
##########
solr/core/src/java/org/apache/solr/metrics/otel/instruments/AttributedInstrumentFactory.java:
##########
@@ -0,0 +1,184 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.metrics.otel.instruments;
+
+import static org.apache.solr.metrics.SolrMetricProducer.CATEGORY_ATTR;
+import static org.apache.solr.metrics.SolrMetricProducer.HANDLER_ATTR;
+
+import io.opentelemetry.api.common.Attributes;
+import io.opentelemetry.api.metrics.LongCounter;
+import io.opentelemetry.api.metrics.LongHistogram;
+import io.opentelemetry.api.metrics.LongUpDownCounter;
+import org.apache.solr.core.SolrInfoBean;
+import org.apache.solr.metrics.SolrMetricManager;
+import org.apache.solr.metrics.SolrMetricsContext;
+import org.apache.solr.metrics.otel.OtelUnit;
+
+/**
+ * Factory for creating metrics instruments that can write to either single or
dual registries (core
+ * and node).
+ */
+public class AttributedInstrumentFactory {
+
+ private final SolrMetricsContext primaryMetricsContext;
+ private final Attributes primaryAttributes;
+ private final boolean aggregateToNodeRegistry;
+ private final boolean primaryIsNodeRegistry;
+ private SolrMetricsContext nodeMetricsContext = null;
+ private Attributes nodeAttributes = null;
+
+ public AttributedInstrumentFactory(
+ SolrMetricsContext primaryMetricsContext,
+ Attributes primaryAttributes,
+ boolean aggregateToNodeRegistry) {
+ this.primaryMetricsContext = primaryMetricsContext;
+ this.primaryAttributes = primaryAttributes;
+ this.aggregateToNodeRegistry = aggregateToNodeRegistry;
+
+ // Primary could be a node
+ this.primaryIsNodeRegistry =
+ primaryMetricsContext
+ .getRegistryName()
+
.equals(SolrMetricManager.getRegistryName(SolrInfoBean.Group.node));
+
+ // Only create node registry if we want dual registry mode AND primary is
not already a node
+ // registry
+ if (aggregateToNodeRegistry && !primaryIsNodeRegistry) {
+ this.nodeMetricsContext =
+ new SolrMetricsContext(
+ primaryMetricsContext.getMetricManager(),
+ SolrMetricManager.getRegistryName(SolrInfoBean.Group.node),
+ null);
+ this.nodeAttributes = createNodeAttributes(primaryAttributes);
+ }
+ }
+
+ public AttributedLongCounter attributedLongCounter(
+ MetricNameProvider metricNameProvider, String description, Attributes
additionalAttributes) {
+ Attributes finalPrimaryAttrs = appendAttributes(primaryAttributes,
additionalAttributes);
+
+ if (aggregateToNodeRegistry && nodeMetricsContext != null) {
+ Attributes finalNodeAttrs = appendAttributes(nodeAttributes,
additionalAttributes);
+
+ LongCounter primaryCounter =
+
primaryMetricsContext.longCounter(metricNameProvider.getPrimaryMetricName(),
description);
+ LongCounter nodeCounter =
+
nodeMetricsContext.longCounter(metricNameProvider.getNodeMetricName(),
description);
+ return new DualRegistryAttributedLongCounter(
+ primaryCounter, finalPrimaryAttrs, nodeCounter, finalNodeAttrs);
+ } else {
+ String metricName =
+ primaryIsNodeRegistry
+ ? metricNameProvider.getNodeMetricName()
+ : metricNameProvider.getPrimaryMetricName();
+
+ LongCounter counter = primaryMetricsContext.longCounter(metricName,
description);
+ return new AttributedLongCounter(counter, finalPrimaryAttrs);
+ }
+ }
+
+ public AttributedLongUpDownCounter attributedLongUpDownCounter(
+ MetricNameProvider metricNameProvider, String description, Attributes
additionalAttributes) {
+ Attributes finalPrimaryAttrs = appendAttributes(primaryAttributes,
additionalAttributes);
+
+ if (aggregateToNodeRegistry && nodeMetricsContext != null) {
+ Attributes finalNodeAttrs = appendAttributes(nodeAttributes,
additionalAttributes);
+
+ LongUpDownCounter primaryCounter =
+ primaryMetricsContext.longUpDownCounter(
+ metricNameProvider.getPrimaryMetricName(), description);
+ LongUpDownCounter nodeCounter =
+
nodeMetricsContext.longUpDownCounter(metricNameProvider.getNodeMetricName(),
description);
+
+ return new DualRegistryAttributedLongUpDownCounter(
+ primaryCounter, finalPrimaryAttrs, nodeCounter, finalNodeAttrs);
+ } else {
+ String metricName =
+ primaryIsNodeRegistry
+ ? metricNameProvider.getNodeMetricName()
+ : metricNameProvider.getPrimaryMetricName();
+
+ LongUpDownCounter counter =
primaryMetricsContext.longUpDownCounter(metricName, description);
+ return new AttributedLongUpDownCounter(counter, finalPrimaryAttrs);
+ }
+ }
+
+ public AttributedLongTimer attributedLongTimer(
+ MetricNameProvider metricNameProvider,
+ String description,
+ OtelUnit unit,
+ Attributes additionalAttributes) {
+ Attributes finalPrimaryAttrs = appendAttributes(primaryAttributes,
additionalAttributes);
+
+ if (aggregateToNodeRegistry) {
+ Attributes finalNodeAttrs = appendAttributes(nodeAttributes,
additionalAttributes);
+ LongHistogram coreHistogram =
+ primaryMetricsContext.longHistogram(
+ metricNameProvider.getPrimaryMetricName(), description, unit);
+ LongHistogram nodeHistogram =
+ nodeMetricsContext.longHistogram(
+ metricNameProvider.getNodeMetricName(), description, unit);
+ return new DualRegistryAttributedLongTimer(
+ coreHistogram, finalPrimaryAttrs, nodeHistogram, finalNodeAttrs);
+ } else {
+ String metricName =
+ primaryIsNodeRegistry
+ ? metricNameProvider.getNodeMetricName()
+ : metricNameProvider.getPrimaryMetricName();
+
+ LongHistogram histogram =
primaryMetricsContext.longHistogram(metricName, description, unit);
+ return new AttributedLongTimer(histogram, finalPrimaryAttrs);
+ }
+ }
+
+ // Filter out core attributes and keep only category and handler if they
exist
+ private Attributes createNodeAttributes(Attributes coreAttributes) {
+ var builder = Attributes.builder();
+
+ if (coreAttributes.get(CATEGORY_ATTR) != null)
+ builder.put(CATEGORY_ATTR, coreAttributes.get(CATEGORY_ATTR));
+ if (coreAttributes.get(HANDLER_ATTR) != null)
+ builder.put(HANDLER_ATTR, coreAttributes.get(HANDLER_ATTR));
Review Comment:
You are hard-coding attributes specific to the metric; there could easily be
more.
I think you should instead loop coreAttributes and add them to builder
unless it's in a disallow-list of core attributes.
##########
solr/core/src/java/org/apache/solr/metrics/otel/instruments/AttributedInstrumentFactory.java:
##########
@@ -0,0 +1,184 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.metrics.otel.instruments;
+
+import static org.apache.solr.metrics.SolrMetricProducer.CATEGORY_ATTR;
+import static org.apache.solr.metrics.SolrMetricProducer.HANDLER_ATTR;
+
+import io.opentelemetry.api.common.Attributes;
+import io.opentelemetry.api.metrics.LongCounter;
+import io.opentelemetry.api.metrics.LongHistogram;
+import io.opentelemetry.api.metrics.LongUpDownCounter;
+import org.apache.solr.core.SolrInfoBean;
+import org.apache.solr.metrics.SolrMetricManager;
+import org.apache.solr.metrics.SolrMetricsContext;
+import org.apache.solr.metrics.otel.OtelUnit;
+
+/**
+ * Factory for creating metrics instruments that can write to either single or
dual registries (core
+ * and node).
+ */
+public class AttributedInstrumentFactory {
+
+ private final SolrMetricsContext primaryMetricsContext;
+ private final Attributes primaryAttributes;
+ private final boolean aggregateToNodeRegistry;
+ private final boolean primaryIsNodeRegistry;
+ private SolrMetricsContext nodeMetricsContext = null;
+ private Attributes nodeAttributes = null;
+
+ public AttributedInstrumentFactory(
+ SolrMetricsContext primaryMetricsContext,
+ Attributes primaryAttributes,
+ boolean aggregateToNodeRegistry) {
+ this.primaryMetricsContext = primaryMetricsContext;
+ this.primaryAttributes = primaryAttributes;
+ this.aggregateToNodeRegistry = aggregateToNodeRegistry;
+
+ // Primary could be a node
+ this.primaryIsNodeRegistry =
Review Comment:
why would this be thie case?
##########
solr/core/src/java/org/apache/solr/metrics/otel/instruments/AttributedInstrumentFactory.java:
##########
@@ -0,0 +1,184 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.metrics.otel.instruments;
+
+import static org.apache.solr.metrics.SolrMetricProducer.CATEGORY_ATTR;
+import static org.apache.solr.metrics.SolrMetricProducer.HANDLER_ATTR;
+
+import io.opentelemetry.api.common.Attributes;
+import io.opentelemetry.api.metrics.LongCounter;
+import io.opentelemetry.api.metrics.LongHistogram;
+import io.opentelemetry.api.metrics.LongUpDownCounter;
+import org.apache.solr.core.SolrInfoBean;
+import org.apache.solr.metrics.SolrMetricManager;
+import org.apache.solr.metrics.SolrMetricsContext;
+import org.apache.solr.metrics.otel.OtelUnit;
+
+/**
+ * Factory for creating metrics instruments that can write to either single or
dual registries (core
+ * and node).
+ */
+public class AttributedInstrumentFactory {
+
+ private final SolrMetricsContext primaryMetricsContext;
+ private final Attributes primaryAttributes;
+ private final boolean aggregateToNodeRegistry;
+ private final boolean primaryIsNodeRegistry;
+ private SolrMetricsContext nodeMetricsContext = null;
+ private Attributes nodeAttributes = null;
+
+ public AttributedInstrumentFactory(
+ SolrMetricsContext primaryMetricsContext,
+ Attributes primaryAttributes,
+ boolean aggregateToNodeRegistry) {
+ this.primaryMetricsContext = primaryMetricsContext;
+ this.primaryAttributes = primaryAttributes;
+ this.aggregateToNodeRegistry = aggregateToNodeRegistry;
+
+ // Primary could be a node
+ this.primaryIsNodeRegistry =
+ primaryMetricsContext
+ .getRegistryName()
+
.equals(SolrMetricManager.getRegistryName(SolrInfoBean.Group.node));
+
+ // Only create node registry if we want dual registry mode AND primary is
not already a node
+ // registry
+ if (aggregateToNodeRegistry && !primaryIsNodeRegistry) {
+ this.nodeMetricsContext =
+ new SolrMetricsContext(
+ primaryMetricsContext.getMetricManager(),
+ SolrMetricManager.getRegistryName(SolrInfoBean.Group.node),
+ null);
+ this.nodeAttributes = createNodeAttributes(primaryAttributes);
+ }
+ }
+
+ public AttributedLongCounter attributedLongCounter(
+ MetricNameProvider metricNameProvider, String description, Attributes
additionalAttributes) {
+ Attributes finalPrimaryAttrs = appendAttributes(primaryAttributes,
additionalAttributes);
+
+ if (aggregateToNodeRegistry && nodeMetricsContext != null) {
+ Attributes finalNodeAttrs = appendAttributes(nodeAttributes,
additionalAttributes);
+
+ LongCounter primaryCounter =
+
primaryMetricsContext.longCounter(metricNameProvider.getPrimaryMetricName(),
description);
+ LongCounter nodeCounter =
+
nodeMetricsContext.longCounter(metricNameProvider.getNodeMetricName(),
description);
+ return new DualRegistryAttributedLongCounter(
+ primaryCounter, finalPrimaryAttrs, nodeCounter, finalNodeAttrs);
+ } else {
+ String metricName =
+ primaryIsNodeRegistry
+ ? metricNameProvider.getNodeMetricName()
+ : metricNameProvider.getPrimaryMetricName();
+
+ LongCounter counter = primaryMetricsContext.longCounter(metricName,
description);
+ return new AttributedLongCounter(counter, finalPrimaryAttrs);
+ }
+ }
+
+ public AttributedLongUpDownCounter attributedLongUpDownCounter(
+ MetricNameProvider metricNameProvider, String description, Attributes
additionalAttributes) {
+ Attributes finalPrimaryAttrs = appendAttributes(primaryAttributes,
additionalAttributes);
+
+ if (aggregateToNodeRegistry && nodeMetricsContext != null) {
+ Attributes finalNodeAttrs = appendAttributes(nodeAttributes,
additionalAttributes);
+
+ LongUpDownCounter primaryCounter =
+ primaryMetricsContext.longUpDownCounter(
+ metricNameProvider.getPrimaryMetricName(), description);
+ LongUpDownCounter nodeCounter =
+
nodeMetricsContext.longUpDownCounter(metricNameProvider.getNodeMetricName(),
description);
+
+ return new DualRegistryAttributedLongUpDownCounter(
+ primaryCounter, finalPrimaryAttrs, nodeCounter, finalNodeAttrs);
+ } else {
+ String metricName =
+ primaryIsNodeRegistry
+ ? metricNameProvider.getNodeMetricName()
+ : metricNameProvider.getPrimaryMetricName();
+
+ LongUpDownCounter counter =
primaryMetricsContext.longUpDownCounter(metricName, description);
+ return new AttributedLongUpDownCounter(counter, finalPrimaryAttrs);
+ }
+ }
+
+ public AttributedLongTimer attributedLongTimer(
+ MetricNameProvider metricNameProvider,
+ String description,
+ OtelUnit unit,
+ Attributes additionalAttributes) {
+ Attributes finalPrimaryAttrs = appendAttributes(primaryAttributes,
additionalAttributes);
+
+ if (aggregateToNodeRegistry) {
+ Attributes finalNodeAttrs = appendAttributes(nodeAttributes,
additionalAttributes);
+ LongHistogram coreHistogram =
+ primaryMetricsContext.longHistogram(
+ metricNameProvider.getPrimaryMetricName(), description, unit);
+ LongHistogram nodeHistogram =
+ nodeMetricsContext.longHistogram(
+ metricNameProvider.getNodeMetricName(), description, unit);
+ return new DualRegistryAttributedLongTimer(
+ coreHistogram, finalPrimaryAttrs, nodeHistogram, finalNodeAttrs);
+ } else {
+ String metricName =
+ primaryIsNodeRegistry
+ ? metricNameProvider.getNodeMetricName()
+ : metricNameProvider.getPrimaryMetricName();
+
+ LongHistogram histogram =
primaryMetricsContext.longHistogram(metricName, description, unit);
+ return new AttributedLongTimer(histogram, finalPrimaryAttrs);
+ }
+ }
+
+ // Filter out core attributes and keep only category and handler if they
exist
+ private Attributes createNodeAttributes(Attributes coreAttributes) {
+ var builder = Attributes.builder();
+
+ if (coreAttributes.get(CATEGORY_ATTR) != null)
+ builder.put(CATEGORY_ATTR, coreAttributes.get(CATEGORY_ATTR));
+ if (coreAttributes.get(HANDLER_ATTR) != null)
+ builder.put(HANDLER_ATTR, coreAttributes.get(HANDLER_ATTR));
+
+ return builder.build();
+ }
+
+ private Attributes appendAttributes(Attributes base, Attributes additional) {
+ return base.toBuilder().putAll(additional).build();
+ }
+
+ public interface MetricNameProvider {
Review Comment:
TBH I think the interface & implementation here isn't that useful.
BTW another approach to the naming is to simply assume that the node version
of a metric name is to replace solr_core with solr_node -- just make this
assumption built-in / hard-coded. If the call site (where the factory method
is called) looks very _normal_ (looks like creating a normal core counter)
that's a good thing IMO.
--
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]
