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
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,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); | ||
|
There was a problem hiding this 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. |
||
| } | ||
|
|
||
| @Override | ||
| public void inc() { | ||
|
||
| super.inc(); | ||
| nodeCounter.inc(); | ||
| } | ||
|
|
||
| @Override | ||
| public void add(Long value) { | ||
| super.add(value); | ||
| nodeCounter.add(value); | ||
| } | ||
| } | ||
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,44 @@ | ||
| /* | ||
| * 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.LongHistogram; | ||
|
|
||
| /** | ||
| * An AttributedLongTimer that records to both core and node registries with corresponding | ||
| * attributes. | ||
| */ | ||
| public class DualRegistryAttributedLongTimer extends AttributedLongTimer { | ||
|
|
||
| private final AttributedLongTimer nodeTimer; | ||
|
|
||
| public DualRegistryAttributedLongTimer( | ||
| LongHistogram coreHistogram, | ||
| Attributes coreAttributes, | ||
| LongHistogram nodeHistogram, | ||
| Attributes nodeAttributes) { | ||
| super(coreHistogram, coreAttributes); | ||
| this.nodeTimer = new AttributedLongTimer(nodeHistogram, nodeAttributes); | ||
| } | ||
|
|
||
| @Override | ||
| public void record(Long value) { | ||
| super.record(value); | ||
| nodeTimer.record(value); | ||
| } | ||
| } |
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