mlbiscoc commented on code in PR #3835:
URL: https://github.com/apache/solr/pull/3835#discussion_r2511038545
##########
solr/core/src/java/org/apache/solr/handler/admin/MetricsHandler.java:
##########
@@ -122,6 +122,11 @@ public void handleRequestBody(SolrQueryRequest req,
SolrQueryResponse rsp) throw
+ format);
}
+ if (!isEnabled()) {
+ rsp.add("metrics", MetricSnapshots.builder().build());
Review Comment:
It shouldn't because above this the format looks for Prometheus/OpenMetrics
and appends `wt=prometheus` if neither is passed in to default to
PrometheusResponseWriter. It will also throw an exception of a Bad Request if
any other format of wt is passed in. Also when you say `#` comment, you want a
log line like `### Metrics disabled` or something?
##########
solr/core/src/java/org/apache/solr/core/MetricsConfig.java:
##########
@@ -16,133 +16,24 @@
*/
package org.apache.solr.core;
-import java.util.Collections;
-
-/** */
+/**
+ * Configuration for metrics collection in Solr. Currently only supports both
enabled and disabled
+ * states. TODO: Extend to support more configuration options in the future.
+ */
public class MetricsConfig {
- private final PluginInfo[] metricReporters;
- private final PluginInfo counterSupplier;
- private final PluginInfo meterSupplier;
- private final PluginInfo timerSupplier;
- private final PluginInfo histogramSupplier;
- private final Object nullNumber;
- private final Object notANumber;
- private final Object nullString;
- private final Object nullObject;
private final boolean enabled;
- private final CacheConfig cacheConfig;
- private MetricsConfig(
- boolean enabled,
- PluginInfo[] metricReporters,
- PluginInfo counterSupplier,
- PluginInfo meterSupplier,
- PluginInfo timerSupplier,
- PluginInfo histogramSupplier,
- Object nullNumber,
- Object notANumber,
- Object nullString,
- Object nullObject,
- CacheConfig cacheConfig) {
+ private MetricsConfig(boolean enabled) {
this.enabled = enabled;
Review Comment:
I moved to check the `PluginInfo.isEnabled` in the latest commit but are you
saying remove enabled from the MetricsConfig class here? The enabled flag here
is used as a check around Solr unless you want to remove this and on
`SolrXmlConfig` just return null?
##########
solr/solr-ref-guide/modules/deployment-guide/pages/metrics-reporting.adoc:
##########
@@ -130,6 +130,23 @@ Metrics collection for index merges can be configured in
the `<metrics>` section
</config>
----
+== Metrics Configuration
+
+The metrics available in your system can be customized by modifying the
`<metrics>` element in `solr.xml`.
+
+TIP: See also the section xref:configuration-guide:configuring-solr-xml.adoc[]
for more information about the `solr.xml` file, where to find it, and how to
edit it.
+
+=== Disabling the Metrics Collection
+The `<metrics>` element in `solr.xml` supports one attribute `metricsEnabled`,
which takes a boolean value,
+for example `<metricsEnabled="true">`.
Review Comment:
Fixed with plugin info so now it is just `enable`
--
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]
