dsmiley commented on code in PR #3734:
URL: https://github.com/apache/solr/pull/3734#discussion_r2404684779
##########
solr/core/src/java/org/apache/solr/metrics/otel/instruments/AttributedDoubleCounter.java:
##########
@@ -30,7 +30,7 @@ public AttributedDoubleCounter(DoubleCounter counter,
Attributes attributes) {
}
public void inc() {
- add(1.0);
+ counter.add(1.0, attributes);
Review Comment:
why? I'd rather keep this and furthermore make the method final to prevent
something overloading it and maybe increase the likelihood of the JVM inlining
it.
Same goes with the other convenience methods on the other instruments.
##########
solr/core/src/java/org/apache/solr/metrics/otel/instruments/DualRegistryAttributedLongCounter.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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 io.opentelemetry.api.common.Attributes;
+import io.opentelemetry.api.metrics.LongCounter;
+
+/**
+ * An AttributedLongCounter that writes to both core and node registries with
corresponding
+ * attributes.
+ */
+public class DualRegistryAttributedLongCounter extends AttributedLongCounter {
+
+ private final AttributedLongCounter nodeCounter;
+
+ public DualRegistryAttributedLongCounter(
+ LongCounter coreCounter,
+ Attributes coreAttributes,
+ LongCounter nodeCounter,
+ Attributes nodeAttributes) {
+ super(coreCounter, coreAttributes);
+ this.nodeCounter = new AttributedLongCounter(nodeCounter, nodeAttributes);
+ }
+
+ @Override
+ public void inc() {
Review Comment:
if you follow my recommendation on making the underlying convenience methods
final, you can happily drop this and other overloads of convenience methods :-)
##########
solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java:
##########
@@ -167,21 +167,13 @@ 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);
// NOCOMMIT: I don't see value in this metric
Review Comment:
drop it, albeit maybe not this PR (I don't care)
##########
solr/core/src/java/org/apache/solr/metrics/otel/instruments/DualRegistryAttributedLongCounter.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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 io.opentelemetry.api.common.Attributes;
+import io.opentelemetry.api.metrics.LongCounter;
+
+/**
+ * An AttributedLongCounter that writes to both core and node registries with
corresponding
+ * attributes.
+ */
+public class DualRegistryAttributedLongCounter extends AttributedLongCounter {
+
+ private final AttributedLongCounter nodeCounter;
+
+ public DualRegistryAttributedLongCounter(
+ LongCounter coreCounter,
+ Attributes coreAttributes,
+ LongCounter nodeCounter,
+ Attributes nodeAttributes) {
+ super(coreCounter, coreAttributes);
+ this.nodeCounter = new AttributedLongCounter(nodeCounter, nodeAttributes);
Review Comment:
It'd be nice to have an assert here that checks that the arguments are not
the same instance, and furthermore, check say one attribute we know is only on
the node is there. No big deal.
##########
solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java:
##########
Review Comment:
The duplicity of code in here is _very_ unfortunate. No need to slightly
vary descriptions; core vs node can be implicit based on the metric name
structure. I feel like there should be a kind of inherit-aware factory of
instruments (that support it; no gauges) so that all inherit-possible metrics a
core might want to register (not just request handlers) could support it
automatically. I sympathize I may be suggesting massive scope creep... if I am
then we can table it. At least consider the notion and tell me what you think
please :-)
FYI some years ago I worked on a competing implementation of the underlying
node/core inheritance we're talking about. It was a hack that doesn't matter
but the underlying need/feature, I think, is really valuable. I don't love
that the ultimate implementation we see now is limited to only RequestHandlers
and requires someone to modify their solrconfig.xml to opt-in to this on a
handler-by-handler basis. Imagine that such aspects are up for change!
Ideally you could simply do this at startup (a singular boolean option)
affecting all supported instrument types of all cores. Even the core's own
instance need not exist (i.e. node only, core only, or both).
##########
solr/core/src/test/org/apache/solr/handler/RequestHandlerBaseTest.java:
##########
@@ -196,6 +196,8 @@ private RequestHandlerBase.HandlerMetrics
createHandlerMetrics() {
when(metricsContext.longHistogram(any(),
any())).thenReturn(mockLongHistogram);
return new RequestHandlerBase.HandlerMetrics(
- metricsContext, Attributes.of(AttributeKey.stringKey("/handler"),
"/someBaseMetricPath"));
+ metricsContext,
+ Attributes.of(AttributeKey.stringKey("/handler"),
"/someBaseMetricPath"),
Review Comment:
Why do we have a key with a leading symbol?
--
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]
