dsmiley commented on code in PR #3745:
URL: https://github.com/apache/solr/pull/3745#discussion_r2415431558
##########
gradle/libs.versions.toml:
##########
@@ -320,11 +320,7 @@ cybozulabs-langdetect = { module =
"com.cybozu.labs:langdetect", version.ref = "
decompose-decompose = { module = "com.arkivanov.decompose:decompose",
version.ref = "decompose" }
decompose-extensions-compose = { module =
"com.arkivanov.decompose:extensions-compose", version.ref = "decompose" }
dropwizard-metrics-core = { module = "io.dropwizard.metrics:metrics-core",
version.ref = "dropwizard-metrics" }
Review Comment:
I still see dropwizard dependencies
##########
solr/server/etc/jetty.xml:
##########
@@ -29,16 +29,6 @@
<!-- Consult the javadoc of o.e.j.util.thread.QueuedThreadPool -->
<!-- for all configuration that may be set here. -->
<!-- =========================================================== -->
- <Arg name="threadPool">
Review Comment:
hm; I wonder if Solr starts -- i.e. do any BATS integration tests work
##########
solr/core/src/test/org/apache/solr/handler/admin/SystemInfoHandlerTest.java:
##########
Review Comment:
The test here is nice that it tells you how to actually implement the code
you removed from SystemInfoHandler (name, version, arch)
##########
solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java:
##########
@@ -124,7 +122,6 @@ public class SearchHandler extends RequestHandlerBase
Boolean.getBoolean("solr.disableRequestId");
private HandlerMetrics metricsShard = HandlerMetrics.NO_OP;
- private final Map<String, Counter> shardPurposes = new ConcurrentHashMap<>();
Review Comment:
huh.... I know of this thing and it's surprising that we don't actually
consume from it. Well good riddance then.
##########
solr/core/src/test/org/apache/solr/cloud/CloudExitableDirectoryReaderTest.java:
##########
@@ -98,27 +96,11 @@ public static void setupCluster() throws Exception {
(n, c) -> DocCollection.isFullyActive(n, c, 2, 1));
fiveHundredsByNode = new LinkedHashMap<>();
Review Comment:
`assertNo500s` uses that
##########
solr/core/src/java/org/apache/solr/core/HttpSolrClientProvider.java:
##########
@@ -69,8 +69,7 @@ private void initializeMetrics(SolrMetricsContext
parentContext) {
var solrMetricsContext = parentContext.getChildContext(this);
String expandedScope =
Review Comment:
now not needed
##########
solr/core/src/java/org/apache/solr/handler/admin/SystemInfoHandler.java:
##########
@@ -217,17 +215,6 @@ public static SimpleOrderedMap<Object> getSystemInfo() {
OperatingSystemMXBean os = ManagementFactory.getOperatingSystemMXBean();
info.add(NAME, os.getName()); // add at least this one
- // add remaining ones dynamically using Java Beans API
Review Comment:
looks like a nocommit to me; we want to return this info
##########
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",
Review Comment:
no solr prefix? hmmm.... I thought we did this universally. I'd say this
would be something like `solr_client_request_duration`
--
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]
