mlbiscoc commented on code in PR #3745:
URL: https://github.com/apache/solr/pull/3745#discussion_r2415273324
##########
solr/core/src/java/org/apache/solr/metrics/SolrMetricProducer.java:
##########
@@ -50,16 +50,18 @@ static String getUniqueMetricTag(Object o, String
parentName) {
}
/**
- * NOCOMMIT SOLR-17458: The Scope parameter will be removed with Dropwizard
+ * Implementation should initialize all metrics to a {@link
SolrMetricsContext}
+ * Registry/MeterProvider with {@link Attributes} as the common set of
attributes that will be
+ * attached to every metric that is initialized for that class/component
*
- * <p>{@link Attributes} passed is the base or common set of attributes that
should be attached to
- * every metric that will be initialized
+ * @param parentContext The registry that the component will initialize
metrics to
+ * @param attributes Base set of attributes that will be bound to all
metrics for that component
*/
- void initializeMetrics(SolrMetricsContext parentContext, Attributes
attributes, String scope);
Review Comment:
`scope` String gone
##########
solr/core/src/java/org/apache/solr/util/stats/InstrumentedHttpListenerFactory.java:
##########
@@ -121,16 +123,16 @@ public void onComplete(Result result) {
};
}
- private Timer timer(Request request) {
- return solrMetricsContext.timer(nameStrategy.getNameFor(scope, request));
- }
-
- // TODO SOLR-17458: Add Otel
@Override
- public void initializeMetrics(
- SolrMetricsContext parentContext, Attributes attributes, String scope) {
+ public void initializeMetrics(SolrMetricsContext parentContext, Attributes
attributes) {
this.solrMetricsContext = parentContext;
- this.scope = scope;
+ this.requestTimer =
+ new AttributedLongTimer(
+ solrMetricsContext.longHistogram(
+ "http_client_request_duration",
+ "HTTP client request duration",
+ OtelUnit.MILLISECONDS),
+ attributes);
Review Comment:
I just found this metric wasn't migrated. Need to prefix the metric with
`solr_` and what should the name of this be?
##########
solr/core/src/test/org/apache/solr/handler/admin/AdminHandlersProxyTest.java:
##########
@@ -71,7 +71,9 @@ public void proxySystemInfoHandlerAllNodes() throws
IOException, SolrServerExcep
assertEquals(nl.getName(2), ((NamedList)
nl.get(nl.getName(2))).get("node"));
}
+ // NOCOMMIT: The nodes view might be broken because of this
@Test
+ @AwaitsFix(bugUrl = "https://issues.apache.org/jira/browse/SOLR-8207";)
public void proxyMetricsHandlerAllNodes() throws IOException,
SolrServerException {
Review Comment:
The admin UI might be broken based on this test. I need to come back to it
##########
solr/core/src/java/org/apache/solr/update/DirectUpdateHandler2.java:
##########
@@ -225,15 +222,15 @@ public DirectUpdateHandler2(SolrCore core, UpdateHandler
updateHandler) {
}
@Override
- public void initializeMetrics(
- SolrMetricsContext parentContext, Attributes attributes, String scope) {
+ public void initializeMetrics(SolrMetricsContext parentContext, Attributes
attributes) {
if
(core.getSolrConfig().getUpdateHandlerInfo().aggregateNodeLevelMetricsEnabled) {
- this.solrMetricsContext =
- new SolrDelegateRegistryMetricsContext(
- parentContext.getMetricManager(),
- parentContext.getRegistryName(),
- SolrMetricProducer.getUniqueMetricTag(this,
parentContext.getTag()),
- SolrMetricManager.getRegistryName(SolrInfoBean.Group.node));
+ // NOCOMMIT: SOLR-17865
+ // this.solrMetricsContext =
+ // new SolrDelegateRegistryMetricsContext(
+ // parentContext.getMetricManager(),
+ // parentContext.getRegistryName(),
+ // SolrMetricProducer.getUniqueMetricTag(this,
parentContext.getTag()),
+ //
SolrMetricManager.getRegistryName(SolrInfoBean.Group.node));
Review Comment:
Need to fix this as part of this
[PR](https://github.com/apache/solr/pull/3734). Might be more duplicate code
which might end up looking very similar to `requestHandlerBase`
##########
solr/core/src/test/org/apache/solr/handler/admin/SystemInfoHandlerTest.java:
##########
Review Comment:
SystemInfoHandler might be missing some metrics because of the migration. I
don't really know how dropwizard placed gauges in this or what this handler was
even showing.
--
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]
