Copilot commented on code in PR #4892:
URL: https://github.com/apache/solr/pull/4892#discussion_r4019599247
##########
solr/core/src/java/org/apache/solr/util/tracing/SimplePropagator.java:
##########
@@ -38,11 +38,12 @@
public class SimplePropagator implements TextMapPropagator {
private static final String TRACE_HOST_NAME =
- EnvUtils.getProperty("solr.traceHostName",
EnvUtils.getProperty("solr.host.advertise"));
+ EnvUtils.getProperty("solr.tracing.host.name",
EnvUtils.getProperty("solr.host.advertise"));
private static final TextMapPropagator INSTANCE = new SimplePropagator();
private static final ContextKey<String> TRACE_ID_KEY =
ContextKey.named("trace_id");
- static final String TRACE_ID = EnvUtils.getProperty("solr.traceIdHeader",
"X-Trace-Id");
+ static final String TRACE_ID =
+ EnvUtils.getProperty("solr.tracing.always.on.header", "X-Trace-Id");
Review Comment:
The new header property does not match the name specified in this PR
(`solr.tracing.id.header`, also the name used by the live-properties
verification). As written, configuring `-Dsolr.tracing.id.header` is ignored
and the propagator remains on `X-Trace-Id`; use the same key as the
compatibility mapping and documentation.
##########
solr/solrj/src/resources/DeprecatedSystemPropertyMappings.properties:
##########
@@ -63,6 +64,8 @@
solr.streamingexpressions.facet.tiered.enabled=solr.facet.stream.tiered
solr.streamingexpressions.macros.enabled=streaming.expression.macros
solr.tracing.always.on.enabled=solr.always.on.trace.id
+solr.tracing.host.name=solr.traceHostName
+solr.tracing.always.on.header=solr.traceIdHeader
Review Comment:
This deprecation shim forwards `solr.traceIdHeader` to
`solr.tracing.always.on.header`, but the requested new public name is
`solr.tracing.id.header`. Once the propagator reads the requested name, this
mapping would set a key it no longer reads, so old deployments would silently
lose their custom header; change the target to the same `id.header` name.
##########
solr/solr-ref-guide/modules/configuration-guide/pages/solr-properties.adoc:
##########
@@ -118,6 +120,10 @@ NOTE: Properties marked with "!" indicate inverted meaning
between pre Solr 10 a
|solr.tracing.always.on.enabled|solr.alwaysOnTraceId|true|Controls whether
trace IDs are always generated, even when no request contains a trace ID.
+|solr.tracing.host.name|solr.traceHostName|Value of
`solr.host.advertise`|Overrides the host name added to the trace ID generated
for a request, when no trace ID is present on an incoming request.
+
+|solr.tracing.always.on.header|solr.traceIdHeader|X-Trace-Id|Specifies the
HTTP header name used by Solr's built-in non-OTEL tracing to propagate the
trace ID between Solr nodes.
Review Comment:
The PR description says documentation is migrated away from deprecated
tracing names, but
`solr-ref-guide/modules/deployment-guide/pages/distributed-tracing.adoc:35`
still instructs users to set `solr.traceIdHeader`. Please update that tracing
guide to the new property name as part of this change; otherwise the primary
tracing documentation continues to direct users to the deprecated setting.
##########
changelog/unreleased/SOLR-17864-rename-skip-autorecovery-and-tracing-props.yml:
##########
@@ -0,0 +1,11 @@
+title: >
+ Renamed the system properties solrcloud.skip.autorecovery to
+ test.solr.cloud.replica.autorecovery.skip.enabled, solr.traceHostName to
solr.tracing.host.name, and
+ solr.traceIdHeader to solr.tracing.always.on.header, to follow Solr's
standard system property naming
Review Comment:
The changelog repeats `solr.tracing.always.on.header` as the migration
target, but the PR's stated canonical property is `solr.tracing.id.header`.
This makes the release note give users the same incorrect configuration name;
update it to match the implementation and mapping.
##########
solr/solr-ref-guide/modules/configuration-guide/pages/solr-properties.adoc:
##########
@@ -118,6 +120,10 @@ NOTE: Properties marked with "!" indicate inverted meaning
between pre Solr 10 a
|solr.tracing.always.on.enabled|solr.alwaysOnTraceId|true|Controls whether
trace IDs are always generated, even when no request contains a trace ID.
+|solr.tracing.host.name|solr.traceHostName|Value of
`solr.host.advertise`|Overrides the host name added to the trace ID generated
for a request, when no trace ID is present on an incoming request.
+
+|solr.tracing.always.on.header|solr.traceIdHeader|X-Trace-Id|Specifies the
HTTP header name used by Solr's built-in non-OTEL tracing to propagate the
trace ID between Solr nodes.
Review Comment:
This new reference row publishes `solr.tracing.always.on.header`, which is
not the requested canonical property name (`solr.tracing.id.header`). Correct
the user-facing property table so users configure the same key that the
implementation reads.
--
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]
