SOLR-17865: Support optional node level metrics recording with OTEL #3734
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 = | ||
|
Comment on lines
+59
to
+60
There was a problem hiding this comment. why would this be thie case? There was a problem hiding this comment. ADMIN requests here go through the same block but don't write dual core and node. Its just node level requests here so I called it primary. |
||
| 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)); | ||
dsmiley marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| return builder.build(); | ||
| } | ||
|
|
||
| private Attributes appendAttributes(Attributes base, Attributes additional) { | ||
| return base.toBuilder().putAll(additional).build(); | ||
| } | ||
|
|
||
| public interface MetricNameProvider { | ||
dsmiley marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| String getPrimaryMetricName(); | ||
|
|
||
| String getNodeMetricName(); | ||
| } | ||
|
|
||
| public static MetricNameProvider standardNameProvider( | ||
| String corePrefix, String nodePrefix, String metricSuffix) { | ||
| return new MetricNameProvider() { | ||
| @Override | ||
| public String getPrimaryMetricName() { | ||
| return corePrefix + metricSuffix; | ||
| } | ||
|
|
||
| @Override | ||
| public String getNodeMetricName() { | ||
| return nodePrefix + metricSuffix; | ||
| } | ||
| }; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote this a while ago and my view of this has kind of changed since then. How about to reduce some of the duplicate code here, we merge these 2 metrics names together:
into just
If there are core attributes then you know it was requests to a core. No core level attributes, then it has to mean node level requests as there is nothing to aggregate on it and we just need 1 metric name.
Example
Definitely scope creep but at the same time I see value in what you are saying here. I'd have to see what implementing this would look like but off the top of my head it'd be a somewhat big refactor. Could we push this back to a 10.x release? You can still calculate your node level requests by summing all the cores in your backend or at aggregation. This is a feature that could very well help your backend aggregation load but doesn't seem like a necessary thing now. Also working on separate branch right now that is removing Dropwizard that is taking time.
I can see about renaming requests to
solr_requests_totalso that in 10.x its easy to just add this node level recording later if you agree.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The refactor for inheritance can be deferred to a minor release.
BTW the JIRA issue for what you are reworking is here, which is a useful examination for providing context / use-case. Imagine thousands of cores at any one time, and furthermore coming and going! It's a metrics cardinality explosion to send any core level metrics to an aggregator. In such a world (that I am very familiar with), you think of the Solr node as a whole that does work. No core metrics. CC @magibney
I don't love the absence of core metrics to mean it's the node level. I worry that filtering is hard. It's imperative that it be possible to easily filter for node metrics at our endpoint. I think our current filtering (fresh custom code) can't handle that. The previous filtering (DropWizard) could definitely support that by filtering to the node registry. Since we register core metrics in a special way (for unloading), maybe our metrics could support a similar filter to not even examine the core ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once I started migrating
DirectUpdateHandler2this duplicate code became way more prevalent and messy and really began to regret the paradigm I adopted when I originally wrote this. So rewrote it with a factory. Let me know what you think