SOLR-17865: Support optional node level metrics recording with OTEL #3734
Conversation
|
This PR was forgotten by mistake. Made a new PR based off rebased and test pass. |
| attributes.toBuilder().put(CATEGORY_ATTR, getCategory().toString()).build(), | ||
| aggregateNodeLevelMetricsEnabled); | ||
|
|
||
| // NOCOMMIT: I don't see value in this metric |
There was a problem hiding this comment.
drop it, albeit maybe not this PR (I don't care)
| 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.
|
|
||
| public void inc() { | ||
| add(1.0); | ||
| counter.add(1.0, attributes); |
There was a problem hiding this 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.
| } | ||
|
|
||
| @Override | ||
| public void inc() { |
There was a problem hiding this comment.
if you follow my recommendation on making the underlying convenience methods final, you can happily drop this and other overloads of convenience methods :-)
| return new RequestHandlerBase.HandlerMetrics( | ||
| metricsContext, Attributes.of(AttributeKey.stringKey("/handler"), "/someBaseMetricPath")); | ||
| metricsContext, | ||
| Attributes.of(AttributeKey.stringKey("/handler"), "/someBaseMetricPath"), |
There was a problem hiding this comment.
Why do we have a key with a leading symbol?
There was a problem hiding this 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).
There was a problem hiding this comment.
No need to slightly vary descriptions; core vs node can be implicit based on the metric name structure.
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:
solr_core_requests_total
solr_node_requests_total
into just
solr_requests_total
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
solr_requests_total{handler="/select",core="foo"} 5
solr_requests_total{handler="/select",core="bar"} 5
solr_requests_total{handler="/select"} 10 <- Node level
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...
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_total so that in 10.x its easy to just add this node level recording later if you agree.
There was a problem hiding this comment.
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.
Once I started migrating DirectUpdateHandler2 this 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
There was a problem hiding this comment.
Once I started migrating DirectUpdateHandler2 this 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
| <luceneMatchVersion>${tests.luceneMatchVersion:LATEST}</luceneMatchVersion> | ||
|
|
||
| <updateHandler class="solr.DirectUpdateHandler2"> | ||
| <updateHandler class="solr.DirectUpdateHandler2" aggregateNodeLevelMetricsEnabled="true"> |
There was a problem hiding this comment.
These aggregation metrics were added to this but surprisingly never tested? I added to the existing aggregation test to check for at least one of these metrics
solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java
Outdated
Show resolved
Hide resolved
solr/core/src/java/org/apache/solr/metrics/otel/instruments/AttributedInstrumentFactory.java
Outdated
Show resolved
Hide resolved
| // Primary could be a node | ||
| this.primaryIsNodeRegistry = |
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.
solr/core/src/java/org/apache/solr/metrics/otel/instruments/AttributedInstrumentFactory.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| // Filter out core attributes and keep only category and handler if they exist | ||
| // Replace core metric name prefix to node prefix |
There was a problem hiding this comment.
that is a plain comment but I think should be javadoc. (I merely mean to tweak the slash style; no need to document anything more)
| return coreMetricName.replace("solr_core", "solr_node"); | ||
| } | ||
|
|
||
| // Filter out core attributes and keep all others for node-level metrics |
|
I'm going to merge this tonight. |
2 participants
https://issues.apache.org/jira/browse/SOLR-17865
In RequestHandlerBase, user can enable aggregateNodeLevelMetricsEnabled to rollup core level metrics at a node level. We need to migrate this logic from Dropwizard to still have this as a supported option