Copilot commented on code in PR #5245:
URL: https://github.com/apache/solr/pull/5245#discussion_r4389157556
##########
solr/core/src/java/org/apache/solr/metrics/otel/MetricExporterFactory.java:
##########
@@ -17,24 +17,81 @@
package org.apache.solr.metrics.otel;
import io.opentelemetry.sdk.metrics.export.MetricExporter;
+import java.util.Locale;
import org.apache.solr.common.util.EnvUtils;
+import org.apache.solr.core.OpenTelemetryConfigurator;
+/**
+ * Configuration of Solr's OTLP metric exporter. Each setting is resolved from
its Solr-specific
+ * property first, then from the equivalent standard OpenTelemetry environment
variable (or {@code
+ * otel.*} system property), then from a default.
+ */
public interface MetricExporterFactory {
- Boolean OTLP_EXPORTER_ENABLED =
-
Boolean.parseBoolean(EnvUtils.getProperty("solr.metrics.otlpExporterEnabled",
"false"));
+ Boolean OTLP_EXPORTER_ENABLED = resolveEnabled();
- String OTLP_EXPORTER_PROTOCOL =
EnvUtils.getProperty("solr.metrics.otlpExporterProtocol", "http");
+ String OTLP_EXPORTER_PROTOCOL = resolveProtocol();
- int OTLP_EXPORTER_INTERVAL =
-
Integer.parseInt(EnvUtils.getProperty("solr.metrics.otlpExporterInterval",
"60000"));
+ int OTLP_EXPORTER_INTERVAL = resolveInterval();
String OTLP_EXPORTER_GRPC_ENDPOINT =
- EnvUtils.getProperty("solr.metrics.otlpGrpcExporterEndpoint",
"http://localhost:4317";);
+ resolveEndpoint("solr.metrics.otlpGrpcExporterEndpoint", false);
String OTLP_EXPORTER_HTTP_ENDPOINT =
- EnvUtils.getProperty(
- "solr.metrics.otlpHttpExporterEndpoint",
"http://localhost:4318/v1/metrics";);
+ resolveEndpoint("solr.metrics.otlpHttpExporterEndpoint", true);
MetricExporter getExporter();
+
+ /**
+ * Also enabled by {@code OTEL_METRICS_EXPORTER} naming {@code otlp}, the
only one Solr exports.
+ */
+ private static boolean resolveEnabled() {
+ if
(Boolean.parseBoolean(EnvUtils.getProperty("solr.metrics.otlpExporterEnabled",
"false"))) {
+ return true;
+ }
Review Comment:
An explicit `solr.metrics.otlpExporterEnabled=false` falls through to
`OTEL_METRICS_EXPORTER=otlp` and enables export anyway. This contradicts the
documented Solr-property precedence and prevents operators from disabling
metrics export independently of their shared OTEL configuration. Only consult
the standard exporter setting when the Solr property is absent.
##########
solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/OtelTracerConfigurator.java:
##########
@@ -57,7 +59,20 @@ public void init(NamedList<?> args) {
public OpenTelemetry createOpenTelemetry() {
// deliberately build() and not initialize(); the latter sets
GlobalOpenTelemetry, which is
// OpenTelemetryConfigurator's job
- return
AutoConfiguredOpenTelemetrySdk.builder().build().getOpenTelemetrySdk();
+ return AutoConfiguredOpenTelemetrySdk.builder()
+ // Hand the resolved resource back to Solr so its metrics carry the
same one as these traces
+ .addResourceCustomizer(
+ (resource, config) -> {
+ this.resource = resource;
+ return resource;
Review Comment:
With the module enabled through `<tracerConfig>`,
`OTEL_RESOURCE_ATTRIBUTES=service.name=search` and no `OTEL_SERVICE_NAME`,
`prepareConfiguration()` supplies `otel.service.name=solr`, which overrides the
resource attribute. Adopting the SDK resource here therefore replaces the
correctly resolved metrics service name with `solr`. Apply the service-name
fallback only when neither configuration source supplies a name, and add a
module test for this case.
##########
solr/core/src/java/org/apache/solr/core/OpenTelemetryConfigurator.java:
##########
@@ -79,6 +109,12 @@ public static synchronized void initializeOpenTelemetrySdk(
if (loaded) return;
loaded = true;
+ // Publish Solr's identity through OTEL_RESOURCE_ATTRIBUTES before any
configurator reads it, so
+ // an autoconfigured SDK and Solr's own meter providers resolve the same
resource. Deliberately
+ // not otel.service.name: that one doubles as the module's auto-activation
switch.
+ setDefaultResourceAttributes(cfg, System.getenv());
Review Comment:
Publishing `otel.resource.attributes` before `configurator.init()` makes the
module treat generated defaults as operator configuration and ignore
`OTEL_RESOURCE_ATTRIBUTES`/`otel.resource.attributes` supplied through
`<tracerConfig>`. Existing namespace, environment, or node-identity attributes
configured in XML are consequently lost from traces and metrics. Apply plugin
configuration before merging resource defaults, while preserving precedence for
actual environment variables and system properties.
--
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]
