dsmiley commented on code in PR #3835:
URL: https://github.com/apache/solr/pull/3835#discussion_r2492101814
##########
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:
There's no need for this; *all* PluginInfo innately support an `enable`
attribute. See `PluginInfo.isEnabled`
BTW I wish it was changed from "enable" to "enabled".... I sometimes get
that mixed up.
##########
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:
typo/wrong
##########
solr/core/src/test/org/apache/solr/cloud/CloudExitableDirectoryReaderTest.java:
##########
@@ -73,6 +73,8 @@ public class CloudExitableDirectoryReaderTest extends
SolrCloudTestCase {
@BeforeClass
public static void setupCluster() throws Exception {
+ System.setProperty("metricsEnabled", "true");
Review Comment:
I think it should be disabled by default in all tests IMO. A test can
opt-in easily enough. If we were to debate and agree that some tests
categorically should be enable-by-default, the SolrCloud ones would be ones to
enable as they are kind of integration in nature. Ironically, that's the
inverse of how things are now :-). Okay to defer this. We use
`jetty.testMode` (poor name) to universally mean that we're running in test
mode (not necessarily with Jetty).
##########
solr/core/src/java/org/apache/solr/metrics/SolrMetricManager.java:
##########
@@ -440,12 +444,17 @@ public boolean hasRegistry(String name) {
}
/**
- * Get (or create if not present) a named {@link SdkMeterProvider}.
+ * Get (or create if not present) a named {@link SdkMeterProvider}. If
metrics are disabled,
+ * returns a NOOP implementation from OpenTelemetry.
*
* @param providerName name of the meter provider and prometheus metric
reader
- * @return existing or newly created meter provider
+ * @return existing or newly created meter provider, or NOOP if metrics
disabled
*/
- public SdkMeterProvider meterProvider(String providerName) {
+ public MeterProvider meterProvider(String providerName) {
+ if (!metricsEnabled) {
+ return OpenTelemetry.noop().getMeterProvider();
Review Comment:
nice
##########
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:
Hard to tell from the PR view but is this causing the handler to return
Solr-XML/JSON/JavaBin? We don't want that. I think a line with a "#" comment
saying it's disabled would be useful
--
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]
