You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/11/09 11:23:26 UTC
[GitHub] [solr] janhoy opened a new pull request, #1168: SOLR-16532 New OTEL module with OTLP trace exporter
janhoy opened a new pull request, #1168:
URL: https://github.com/apache/solr/pull/1168
https://issues.apache.org/jira/browse/SOLR-16532
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on PR #1168:
URL: https://github.com/apache/solr/pull/1168#issuecomment-1347386908
> Sounds good but how?
I was thinking in a `static {}` context, but there are some interactions there, so let's defer such feature, at least to another JIRA.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on PR #1168:
URL: https://github.com/apache/solr/pull/1168#issuecomment-1368824084
Happy New Year!
I updated this PR with an integration test similar to the one in `JaegerTracerConfigurator`, starting the tracer in a cluster.
I then discovered that the OTEL tracer was not being closed correctly, and there was a leaking background thread from the batch span exporter. Turned out that OTEL's [OpenTracing Shim](https://github.com/open-telemetry/opentelemetry-java/tree/main/opentracing-shim) class by design [will not close the actual tracer](https://github.com/open-telemetry/opentelemetry-java/blob/v1.21.0/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/TracerShim.java#L98), so when `CoreContainer#shutdown()` is called, the tracer is left open.
The workaround I chose is to wrap the `TracerShim` in a thin `ClosableTracerShim` delegate class that will properly close the tracer through the SDK. Seems to work, though I have yet to test this version of the module with a real collector.
I also had to adjust the dependencies, since I started using the SDK directly and not only through ENV auto-config.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1061822568
##########
versions.props:
##########
@@ -20,8 +20,10 @@ commons-codec:commons-codec=1.15
commons-collections:commons-collections=3.2.2
commons-io:commons-io=2.11.0
io.dropwizard.metrics:*=4.2.12
+io.grpc:grpc-*=1.50.2
io.jaegertracing:*=1.8.1
io.netty:*=4.1.82.Final
+io.opentelemetry:opentelemetry-bom-alpha=1.21.0-alpha
Review Comment:
Well, it is a bit misleading here, since this BOM depends also on the `1.21.0` bom. Most APIs are stable, but the Log SDK is still in Alpha. See https://github.com/open-telemetry/opentelemetry-java#sdk for which are stable and alpha.
The reason we include the alpha bom is that we use the `opentelemetry-sdk-extension-autoconfigure` extension which is still alpha. I guess when it graduates, we can move to only using the stable BOM. In other words, gradle will pick the stable version if it exists, else fallback to alpha, as can be seen from the `lib/` listing I posted above.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1043963255
##########
solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/OtelTracerConfigurator.java:
##########
@@ -0,0 +1,62 @@
+package org.apache.solr.opentelemetry;
+
+import io.opentelemetry.opentracingshim.OpenTracingShim;
+import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdk;
+import io.opentracing.Tracer;
+import java.lang.invoke.MethodHandles;
+import java.util.Locale;
+import java.util.Objects;
+import org.apache.solr.core.TracerConfigurator;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class OtelTracerConfigurator extends TracerConfigurator {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ @Override
+ public Tracer getTracer() {
+ log.info("Configuring tracer {}...", getClass().getName());
+
+ setDefaultIfNotConfigured("OTEL_SERVICE_NAME", "solr");
+ setDefaultIfNotConfigured("OTEL_TRACES_EXPORTER", "otlp");
+ setDefaultIfNotConfigured("OTEL_EXPORTER_OTLP_PROTOCOL", "grpc");
+ setDefaultIfNotConfigured("OTEL_TRACES_SAMPLER", "parentbased_always_on");
+
+ System.getenv().entrySet().stream()
+ .filter(e -> e.getKey().startsWith("OTEL_"))
+ .forEach(entry -> log.info("Environment {}={}", entry.getKey(), entry.getValue()));
+
+ // Need to disable the exporters for metrics and logs
+ String metricsExporter = getEnvOrSysprop("OTEL_METRICS_EXPORTER");
+ String logsExporter = getEnvOrSysprop("OTEL_LOGS_EXPORTER");
+ if (metricsExporter != null && !Objects.equals(metricsExporter, "none")
Review Comment:
*[OperatorPrecedence](https://errorprone.info/bugpattern/OperatorPrecedence):* Use grouping parenthesis to make the operator precedence explicit
---
```suggestion
if ((metricsExporter != null && !Objects.equals(metricsExporter, "none"))
```
---
<details><summary><b>âšī¸ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=359492009&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=359492009&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=359492009&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=359492009&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=359492009&lift_comment_rating=5) ]
##########
solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/OtelTracerConfigurator.java:
##########
@@ -0,0 +1,62 @@
+package org.apache.solr.opentelemetry;
+
+import io.opentelemetry.opentracingshim.OpenTracingShim;
+import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdk;
+import io.opentracing.Tracer;
+import java.lang.invoke.MethodHandles;
+import java.util.Locale;
+import java.util.Objects;
+import org.apache.solr.core.TracerConfigurator;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class OtelTracerConfigurator extends TracerConfigurator {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ @Override
+ public Tracer getTracer() {
+ log.info("Configuring tracer {}...", getClass().getName());
+
+ setDefaultIfNotConfigured("OTEL_SERVICE_NAME", "solr");
+ setDefaultIfNotConfigured("OTEL_TRACES_EXPORTER", "otlp");
+ setDefaultIfNotConfigured("OTEL_EXPORTER_OTLP_PROTOCOL", "grpc");
+ setDefaultIfNotConfigured("OTEL_TRACES_SAMPLER", "parentbased_always_on");
+
+ System.getenv().entrySet().stream()
+ .filter(e -> e.getKey().startsWith("OTEL_"))
+ .forEach(entry -> log.info("Environment {}={}", entry.getKey(), entry.getValue()));
+
+ // Need to disable the exporters for metrics and logs
+ String metricsExporter = getEnvOrSysprop("OTEL_METRICS_EXPORTER");
+ String logsExporter = getEnvOrSysprop("OTEL_LOGS_EXPORTER");
+ if (metricsExporter != null && !Objects.equals(metricsExporter, "none")
+ || logsExporter != null && !Objects.equals(logsExporter, "none")) {
Review Comment:
*[OperatorPrecedence](https://errorprone.info/bugpattern/OperatorPrecedence):* Use grouping parenthesis to make the operator precedence explicit
---
```suggestion
|| (logsExporter != null && !Objects.equals(logsExporter, "none"))) {
```
---
<details><summary><b>âšī¸ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=359492041&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=359492041&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=359492041&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=359492041&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=359492041&lift_comment_rating=5) ]
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1061838234
##########
versions.props:
##########
@@ -20,8 +20,10 @@ commons-codec:commons-codec=1.15
commons-collections:commons-collections=3.2.2
commons-io:commons-io=2.11.0
io.dropwizard.metrics:*=4.2.12
+io.grpc:grpc-*=1.50.2
io.jaegertracing:*=1.8.1
io.netty:*=4.1.82.Final
+io.opentelemetry:opentelemetry-bom-alpha=1.21.0-alpha
Review Comment:
yea just hoping we can move from "alpha" when possible.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on PR #1168:
URL: https://github.com/apache/solr/pull/1168#issuecomment-1344021399
This is one step closer commit.
How to you propse we add tests for this? It does not feel necessary to mock a collector to prove that traces are sent. We have to trust the library... So I plan to add a few unit tests checking the configuration logic we have.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1045099651
##########
solr/modules/opentelemetry/build.gradle:
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Open Telemetry (OTEL) tracer'
+
+dependencies {
+ implementation project(':solr:core')
+
+ implementation 'io.opentracing:opentracing-api'
+ implementation 'org.slf4j:slf4j-api'
+
+ implementation platform('io.opentelemetry:opentelemetry-bom-alpha')
+
+ implementation 'io.opentelemetry:opentelemetry-sdk-trace'
+ // TODO: permitUnusedDeclared does not work for the io.opentelemetry dependencies for some reason
+ permitUnusedDeclared 'io.opentelemetry:opentelemetry-sdk-trace'
Review Comment:
@risdenk Can you help with this, why don't `permitUnusedDeclared` work here? I get:
```
Could not determine the dependencies of task ':solr:modules:opentelemetry:analyzeClassesDependencies'.
> Could not resolve all task dependencies for configuration ':solr:modules:opentelemetry:permitUnusedDeclared'.
> Could not find io.opentelemetry:opentelemetry-sdk-trace:.
Required by:
project :solr:modules:opentelemetry
> Could not find io.opentelemetry:opentelemetry-semconv:.
Required by:
project :solr:modules:opentelemetry
> Could not find io.opentelemetry:opentelemetry-exporter-otlp:.
Required by:
project :solr:modules:opentelemetry
Possible solution:
- Declare repository providing the artifact, see the documentation at https://docs.gradle.org/current/userguide/declaring_repositories.html
```
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1060202102
##########
solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/OtelTracerConfiguratorTest.java:
##########
@@ -73,4 +86,24 @@ public void testSetDefaultIfNotConfigured() {
assertEquals("default", instance.getCurrentOtelConfig().get("OTEL_YEY"));
assertEquals("prop-k1", instance.getCurrentOtelConfig().get("OTEL_K1"));
}
+
+ @Test
+ public void testInjected() throws Exception {
+ // Make sure the batch exporter times out before our thread lingering time of 10s
+ instance.setDefaultIfNotConfigured("OTEL_BSP_SCHEDULE_DELAY", "1000");
+ instance.setDefaultIfNotConfigured("OTEL_BSP_EXPORT_TIMEOUT", "2000");
+ MiniSolrCloudCluster cluster =
+ new MiniSolrCloudCluster.Builder(2, createTempDir())
+ .addConfig("config", TEST_PATH().resolve("collection1").resolve("conf"))
+ .withSolrXml(getFile("solr/solr.xml").toPath())
+ .build();
+ try {
+ TimeOut timeOut = new TimeOut(2, TimeUnit.MINUTES, TimeSource.NANO_TIME);
+ timeOut.waitFor(
+ "Waiting for GlobalTracer is registered",
+ () -> GlobalTracer.get().toString().contains("TracerShim"));
Review Comment:
Yeah. MiniSolrCloudCluster is certainly finished when build is returned. Look at the end of MSCC's constructor -- it waits for all servers to be available.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1061842663
##########
solr/modules/opentelemetry/build.gradle:
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Open Telemetry (OTEL) tracer'
+
+dependencies {
+ implementation project(':solr:core')
+
+ platform('io.opentelemetry:opentelemetry-bom-alpha')
Review Comment:
arg I misremembered:
```suggestion
implementation platform('io.opentelemetry:opentelemetry-bom-alpha')
```
this follows the other `platform` examples in our build.gradle files.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1025744026
##########
solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/OtelTracerConfigurator.java:
##########
@@ -0,0 +1,29 @@
+package org.apache.solr.opentelemetry;
+
+import io.opentelemetry.opentracingshim.OpenTracingShim;
+import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdk;
+import io.opentracing.Tracer;
+import org.apache.solr.core.TracerConfigurator;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.invoke.MethodHandles;
+
+public class OtelTracerConfigurator extends TracerConfigurator {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ @Override
+ public Tracer getTracer() {
+ // TODO: Find better way to set defaults
+ System.setProperty("OTEL_SERVICE_NAME", "solr");
+ System.setProperty("OTEL_EXPORTER_OTLP_PROTOCOL", "grpc");
+ System.setProperty("OTEL_TRACES_SAMPLER", "parentbased_always_on");
+ System.setProperty("OTEL_TRACES_EXPORTER", "otlp");
+ // Need to disable the exporters for metrics and logs
+ System.setProperty("OTEL_METRICS_EXPORTER", "none");
+ System.setProperty("OTEL_LOGS_EXPORTER", "none");
+ log.info("Configuring tracer {}...", getClass().getName());
Review Comment:
This line looks generic; could be called by Solr once instead of each impl doing this.
##########
solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/OtelTracerConfigurator.java:
##########
@@ -0,0 +1,29 @@
+package org.apache.solr.opentelemetry;
+
+import io.opentelemetry.opentracingshim.OpenTracingShim;
+import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdk;
+import io.opentracing.Tracer;
+import org.apache.solr.core.TracerConfigurator;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.invoke.MethodHandles;
+
+public class OtelTracerConfigurator extends TracerConfigurator {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ @Override
+ public Tracer getTracer() {
+ // TODO: Find better way to set defaults
+ System.setProperty("OTEL_SERVICE_NAME", "solr");
+ System.setProperty("OTEL_EXPORTER_OTLP_PROTOCOL", "grpc");
+ System.setProperty("OTEL_TRACES_SAMPLER", "parentbased_always_on");
+ System.setProperty("OTEL_TRACES_EXPORTER", "otlp");
+ // Need to disable the exporters for metrics and logs
+ System.setProperty("OTEL_METRICS_EXPORTER", "none");
+ System.setProperty("OTEL_LOGS_EXPORTER", "none");
Review Comment:
A utility method that sets it if it's not set would be good. Also check ENV vars which probably already work.
##########
solr/modules/opentelemetry/build.gradle:
##########
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Open Telemetry (OTEL) tracer'
+
+dependencies {
+ implementation project(':solr:core')
+
+ // TODO: Prune down dependencies to the absolute necessary
+
+ implementation platform('io.opentelemetry:opentelemetry-bom-alpha')
+
+ implementation 'io.opentelemetry:opentelemetry-sdk-trace'
+ implementation 'io.opentelemetry:opentelemetry-sdk-extension-autoconfigure'
+ implementation 'io.opentelemetry:opentelemetry-opentracing-shim'
+ implementation 'io.opentelemetry:opentelemetry-semconv'
+ implementation 'io.opentelemetry:opentelemetry-exporter-otlp'
+ implementation 'io.opentelemetry:opentelemetry-exporter-jaeger'
+ implementation 'io.opentelemetry:opentelemetry-exporter-zipkin'
+
+ // gRPC transport via netty - since we already ship netty this is more lightweight than netty-shaded
+ runtimeOnly 'io.grpc:grpc-netty'
+ implementation 'io.grpc:grpc-protobuf'
+ implementation 'io.grpc:grpc-stub'
+ implementation 'io.grpc:grpc-context'
+ // See https://issues.apache.org/jira/browse/LOG4J2-3609 due to needing these annotations
+ compileOnly 'org.apache.tomcat:annotations-api'
+
+ // TODO: Both this and jaegertracer-configurator pull in com.squareup.okhttp3:okhttp HTTP client and kotlin stdlib. They get duplicated in each module, so consider declaring these explicitly on solr-core
Review Comment:
Pointing out this is okay but I think adding to solr-core just because 2 modules use them is not the right direction. I'd rather see another approach or just accept it. The jaeger one is going away.
##########
solr/modules/opentelemetry/build.gradle:
##########
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Open Telemetry (OTEL) tracer'
+
+dependencies {
+ implementation project(':solr:core')
+
+ // TODO: Prune down dependencies to the absolute necessary
+
+ implementation platform('io.opentelemetry:opentelemetry-bom-alpha')
+
+ implementation 'io.opentelemetry:opentelemetry-sdk-trace'
+ implementation 'io.opentelemetry:opentelemetry-sdk-extension-autoconfigure'
+ implementation 'io.opentelemetry:opentelemetry-opentracing-shim'
+ implementation 'io.opentelemetry:opentelemetry-semconv'
+ implementation 'io.opentelemetry:opentelemetry-exporter-otlp'
Review Comment:
if the industry is standardizing on otlp, then it's reasonable to not include the other 2 if they add more transitive dependencies. Users can add them if they wish.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1062223079
##########
solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/OtelTracerConfigurator.java:
##########
@@ -0,0 +1,138 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.opentelemetry;
+
+import io.opentelemetry.opentracingshim.OpenTracingShim;
+import io.opentelemetry.sdk.OpenTelemetrySdk;
+import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdk;
+import io.opentracing.Tracer;
+import java.lang.invoke.MethodHandles;
+import java.util.HashMap;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Objects;
+import java.util.stream.Collectors;
+import org.apache.solr.core.TracerConfigurator;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * OpenTracing TracerConfigurator implementation which exports spans to OpenTelemetry in OTLP
+ * format. This impl re-uses the existing OpenTracing instrumentation through a shim, and takes care
+ * of properly closing the backing Tracer when Solr shuts down.
+ */
+public class OtelTracerConfigurator extends TracerConfigurator {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ // Copy of environment. Can be overridden by tests
+ Map<String, String> currentEnv = System.getenv();
+
+ @Override
+ public Tracer getTracer() {
+ setDefaultIfNotConfigured("OTEL_SERVICE_NAME", "solr");
+ setDefaultIfNotConfigured("OTEL_TRACES_EXPORTER", "otlp");
+ setDefaultIfNotConfigured("OTEL_EXPORTER_OTLP_PROTOCOL", "grpc");
+ setDefaultIfNotConfigured("OTEL_TRACES_SAMPLER", "parentbased_always_on");
+
+ final String currentConfig =
+ String.join(
+ "; ",
+ getCurrentOtelConfig().entrySet().stream()
+ .map(e -> e.getKey() + "=" + e.getValue())
+ .collect(Collectors.toSet()));
+
+ log.info("OpenTelemetry tracer enabled with configuration: {}", currentConfig);
Review Comment:
In fact I do. it's a one-liner and will spell out in cleartext whether and how tracer was configured. Imagine someone asking on user list that they cannot get tracing to work, they have configured an otel collector on port blabla but no traces show up. Now we can ask them to verify this line in the log, and perhaps they discover some ENV variable that is wrong etc.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1059953698
##########
solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/ClosableTracerShim.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.opentelemetry;
+
+import io.opentelemetry.sdk.trace.SdkTracerProvider;
+import io.opentracing.Scope;
+import io.opentracing.ScopeManager;
+import io.opentracing.Span;
+import io.opentracing.SpanContext;
+import io.opentracing.Tracer;
+import io.opentracing.propagation.Format;
+import java.lang.invoke.MethodHandles;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Delegate shim that forwards all calls to the actual {@link
+ * io.opentelemetry.opentracingshim.OpenTracingShim}, and in addition calls {@link
+ * SdkTracerProvider#close()} to really close the OTEL SDK tracer when the OT shim is closed.
+ *
+ * <p>TODO: This can be removed once we migrate Solr instrumentation from OpenTracing to
+ * OpenTelemetry
+ */
+public class ClosableTracerShim implements Tracer {
Review Comment:
I'd have subclassed [TracerShim](https://github.com/open-telemetry/opentelemetry-java/blob/v1.21.0/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/TracerShim.java#L20) instead if it was not final.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on PR #1168:
URL: https://github.com/apache/solr/pull/1168#issuecomment-1370580585
Thanks for the review. I just updated to the latest version of opentelementry.
The contents of `modules/opentelemetry/lib/` folder now totals ~10Mb, see list below.
I made an attempt to slim it down by excluding `opentelemetry-sdk-logs`, `opentelemetry-sdk-metrics` (since we only use trace), but the auto-configuration mode we use to support configuration by ENV variable has hard dependencies on them during initialization. Also I thought we could skip support for JSON/HTTP and only do protobuf/gRPC (using netty), but for similar reasons I could not.
So even if I don't like the weight increase I'm voting for shipping this, giving users a choice between transports. Perhaps also in a later release we can try to activate OTLP metrics and logs export in this module.
```
20K annotations-13.0.jar
4.0K annotations-4.1.1.4.jar
256K grpc-api-1.50.2.jar
32K grpc-context-1.50.2.jar
716K grpc-core-1.50.2.jar
276K grpc-netty-1.50.2.jar
8.0K grpc-protobuf-1.50.2.jar
8.0K grpc-protobuf-lite-1.50.2.jar
52K grpc-stub-1.50.2.jar
260K gson-2.9.1.jar
1.5M kotlin-stdlib-1.7.21.jar
208K kotlin-stdlib-common-1.7.21.jar
48K kotlin-stdlib-jdk7-1.7.21.jar
20K kotlin-stdlib-jdk8-1.7.21.jar
632K netty-codec-http-4.1.82.Final.jar
464K netty-codec-http2-4.1.82.Final.jar
120K netty-codec-socks-4.1.82.Final.jar
24K netty-handler-proxy-4.1.82.Final.jar
768K okhttp-4.10.0.jar
344K okio-jvm-3.0.0.jar
128K opentelemetry-api-1.21.0.jar
12K opentelemetry-api-logs-1.21.0-alpha.jar
48K opentelemetry-context-1.21.0.jar
88K opentelemetry-exporter-common-1.21.0.jar
32K opentelemetry-exporter-otlp-1.21.0.jar
108K opentelemetry-exporter-otlp-common-1.21.0.jar
32K opentelemetry-opentracing-shim-1.21.0-alpha.jar
8.0K opentelemetry-sdk-1.21.0.jar
40K opentelemetry-sdk-common-1.21.0.jar
48K opentelemetry-sdk-extension-autoconfigure-1.21.0-alpha.jar
20K opentelemetry-sdk-extension-autoconfigure-spi-1.21.0.jar
44K opentelemetry-sdk-logs-1.21.0-alpha.jar
280K opentelemetry-sdk-metrics-1.21.0.jar
116K opentelemetry-sdk-trace-1.21.0.jar
36K opentelemetry-semconv-1.21.0-alpha.jar
8.0K perfmark-api-0.25.0.jar
1.7M proto-google-common-protos-2.9.2.jar
1.6M protobuf-java-3.21.12.jar
20K solr-opentelemetry-10.0.0-SNAPSHOT.jar
10M
```
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1018213452
##########
solr/modules/otel/build.gradle:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Open Telemetry (OTEL) tracer'
+
+dependencies {
+ implementation project(':solr:core')
+
+ // TODO: Prune down dependencies to the absolute necessary
+
+ implementation platform('io.opentelemetry:opentelemetry-bom')
+ implementation platform('io.opentelemetry:opentelemetry-bom-alpha')
+
+// implementation 'io.opentelemetry:opentelemetry-sdk-common'
+ implementation 'io.opentelemetry:opentelemetry-sdk-trace'
+ implementation 'io.opentelemetry:opentelemetry-sdk-extension-autoconfigure'
+ implementation 'io.opentelemetry:opentelemetry-opentracing-shim'
+ implementation 'io.opentelemetry:opentelemetry-semconv'
+ implementation 'io.opentelemetry:opentelemetry-exporter-otlp-trace:1.14.0'
+ implementation 'io.opentelemetry:opentelemetry-exporter-jaeger'
+ implementation 'io.opentelemetry:opentelemetry-exporter-zipkin'
+
+ // GRPC transport via netty - since we already ship netty this is cheaper than netty-shaded
+ runtimeOnly 'io.grpc:grpc-netty'
+ implementation 'io.grpc:grpc-protobuf'
+ implementation 'io.grpc:grpc-stub'
+ implementation 'io.grpc:grpc-context'
Review Comment:
We have to choose what transports to support for sending the traces. The spec recommends that grpc is the preferred, but alternatively could support `http`. Would like to support the most common format that is most likely that people have enabled in their OTEL collector. I guess both grpc and http is quite standard??
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #1168:
URL: https://github.com/apache/solr/pull/1168#issuecomment-1352478724
I love the docker-compose idea! We'd ideally have one docker-compose sample for Solr with this in and and users could take it and comment out whatever they don't want.
Integration tests: BATS -- yeah I guess; I can sympathize a test isn't easy. Maybe this could test your proposed docker-compose so we test that *that* is configured properly as well.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1061815367
##########
solr/modules/opentelemetry/build.gradle:
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Open Telemetry (OTEL) tracer'
+
+// This is a hacky way to use permitTestUnusedDeclared with bom declared dependencies.
+// See https://github.com/gradle-dependency-analyze/gradle-dependency-analyze/issues/108
+configurations {
+ constraintsOnly
+ permitUnusedDeclared.extendsFrom constraintsOnly
+ implementation.extendsFrom constraintsOnly
+}
+
+dependencies {
+ implementation project(':solr:core')
+
+ constraintsOnly(platform('io.opentelemetry:opentelemetry-bom-alpha'))
+
+ implementation 'io.opentracing:opentracing-api'
+ implementation 'org.slf4j:slf4j-api'
+
+ implementation 'io.opentelemetry:opentelemetry-sdk'
+ implementation 'io.opentelemetry:opentelemetry-sdk-trace'
+ implementation 'io.opentelemetry:opentelemetry-sdk-extension-autoconfigure'
+ implementation 'io.opentelemetry:opentelemetry-opentracing-shim'
+ implementation 'io.opentelemetry:opentelemetry-semconv'
Review Comment:
The three in lines 39-41 needed to be 'implementation', or else I got errors like:
```
/Users/janhoy/git/solr/solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/OtelTracerConfigurator.java:21: error: package io.opentelemetry.sdk.autoconfigure does not exist
import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdk;
```
I converted these to `runtimeOnly` in 800d5e0693fa7b7c85de7ad5b91314e27174119d:
* 'io.opentelemetry:opentelemetry-semconv'
* 'io.opentelemetry:opentelemetry-exporter-otlp'
* and all the grpc ones
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1061832334
##########
solr/modules/opentelemetry/build.gradle:
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Open Telemetry (OTEL) tracer'
+
+dependencies {
+ implementation project(':solr:core')
+
+ runtimeOnly(platform('io.opentelemetry:opentelemetry-bom-alpha'))
Review Comment:
I don't even think we need the `runtimeOnly` just `platform('io.opentelemetry:opentelemetry-bom-alpha')` should do it :)
```suggestion
platform('io.opentelemetry:opentelemetry-bom-alpha')
```
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1061895876
##########
solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/ClosableTracerShim.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.opentelemetry;
+
+import io.opentelemetry.sdk.trace.SdkTracerProvider;
+import io.opentracing.Scope;
+import io.opentracing.ScopeManager;
+import io.opentracing.Span;
+import io.opentracing.SpanContext;
+import io.opentracing.Tracer;
+import io.opentracing.propagation.Format;
+import java.lang.invoke.MethodHandles;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Delegate shim that forwards all calls to the actual {@link
+ * io.opentelemetry.opentracingshim.OpenTracingShim}, and in addition calls {@link
+ * SdkTracerProvider#close()} to really close the OTEL SDK tracer when the OT shim is closed.
+ *
+ * <p>TODO: This can be removed once we migrate Solr instrumentation from OpenTracing to
+ * OpenTelemetry
+ */
+public class ClosableTracerShim implements Tracer {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ private final Tracer shim;
+ private final SdkTracerProvider sdkTracerProvider;
+
+ public ClosableTracerShim(Tracer shim, SdkTracerProvider sdkTracerProvider) {
+ this.shim = shim;
+ this.sdkTracerProvider = sdkTracerProvider;
+ }
+
+ @Override
+ public ScopeManager scopeManager() {
+ return shim.scopeManager();
+ }
+
+ @Override
+ public Span activeSpan() {
+ return shim.activeSpan();
+ }
+
+ @Override
+ public Scope activateSpan(Span span) {
+ return shim.activateSpan(span);
+ }
+
+ @Override
+ public SpanBuilder buildSpan(String operationName) {
+ return shim.buildSpan(operationName);
+ }
+
+ @Override
+ public <C> void inject(SpanContext spanContext, Format<C> format, C carrier) {
+ shim.inject(spanContext, format, carrier);
+ }
+
+ @Override
+ public <C> SpanContext extract(Format<C> format, C carrier) {
+ return shim.extract(format, carrier);
+ }
+
+ @Override
+ public void close() {
+ shim.close();
+ log.info("Closing wrapped OTEL tracer instance.");
Review Comment:
Honestly seems like debug level of importance. If all our closeables logged they were closing.... ouch.
##########
solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/OtelTracerConfigurator.java:
##########
@@ -0,0 +1,138 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.opentelemetry;
+
+import io.opentelemetry.opentracingshim.OpenTracingShim;
+import io.opentelemetry.sdk.OpenTelemetrySdk;
+import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdk;
+import io.opentracing.Tracer;
+import java.lang.invoke.MethodHandles;
+import java.util.HashMap;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Objects;
+import java.util.stream.Collectors;
+import org.apache.solr.core.TracerConfigurator;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * OpenTracing TracerConfigurator implementation which exports spans to OpenTelemetry in OTLP
+ * format. This impl re-uses the existing OpenTracing instrumentation through a shim, and takes care
+ * of properly closing the backing Tracer when Solr shuts down.
+ */
+public class OtelTracerConfigurator extends TracerConfigurator {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ // Copy of environment. Can be overridden by tests
+ Map<String, String> currentEnv = System.getenv();
+
+ @Override
+ public Tracer getTracer() {
+ setDefaultIfNotConfigured("OTEL_SERVICE_NAME", "solr");
+ setDefaultIfNotConfigured("OTEL_TRACES_EXPORTER", "otlp");
+ setDefaultIfNotConfigured("OTEL_EXPORTER_OTLP_PROTOCOL", "grpc");
+ setDefaultIfNotConfigured("OTEL_TRACES_SAMPLER", "parentbased_always_on");
+
+ final String currentConfig =
+ String.join(
+ "; ",
+ getCurrentOtelConfig().entrySet().stream()
+ .map(e -> e.getKey() + "=" + e.getValue())
+ .collect(Collectors.toSet()));
+
+ log.info("OpenTelemetry tracer enabled with configuration: {}", currentConfig);
Review Comment:
Do you think outputting the config warrants INFO level? ... I have no doubt it was useful during development but we should consider our log levels when we ship.
##########
solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/OtelTracerConfigurator.java:
##########
@@ -0,0 +1,138 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.opentelemetry;
+
+import io.opentelemetry.opentracingshim.OpenTracingShim;
+import io.opentelemetry.sdk.OpenTelemetrySdk;
+import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdk;
+import io.opentracing.Tracer;
+import java.lang.invoke.MethodHandles;
+import java.util.HashMap;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Objects;
+import java.util.stream.Collectors;
+import org.apache.solr.core.TracerConfigurator;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * OpenTracing TracerConfigurator implementation which exports spans to OpenTelemetry in OTLP
+ * format. This impl re-uses the existing OpenTracing instrumentation through a shim, and takes care
+ * of properly closing the backing Tracer when Solr shuts down.
+ */
+public class OtelTracerConfigurator extends TracerConfigurator {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ // Copy of environment. Can be overridden by tests
+ Map<String, String> currentEnv = System.getenv();
+
+ @Override
+ public Tracer getTracer() {
+ setDefaultIfNotConfigured("OTEL_SERVICE_NAME", "solr");
+ setDefaultIfNotConfigured("OTEL_TRACES_EXPORTER", "otlp");
+ setDefaultIfNotConfigured("OTEL_EXPORTER_OTLP_PROTOCOL", "grpc");
+ setDefaultIfNotConfigured("OTEL_TRACES_SAMPLER", "parentbased_always_on");
+
+ final String currentConfig =
+ String.join(
+ "; ",
+ getCurrentOtelConfig().entrySet().stream()
+ .map(e -> e.getKey() + "=" + e.getValue())
+ .collect(Collectors.toSet()));
Review Comment:
If the objective is to create a String (which I see it is), then it's better to use a `Collectors.joining("; ")` instead of producing a Set to then join it.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1044748495
##########
solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/OtelTracerConfigurator.java:
##########
@@ -0,0 +1,29 @@
+package org.apache.solr.opentelemetry;
+
+import io.opentelemetry.opentracingshim.OpenTracingShim;
+import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdk;
+import io.opentracing.Tracer;
+import org.apache.solr.core.TracerConfigurator;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.invoke.MethodHandles;
+
+public class OtelTracerConfigurator extends TracerConfigurator {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ @Override
+ public Tracer getTracer() {
+ // TODO: Find better way to set defaults
+ System.setProperty("OTEL_SERVICE_NAME", "solr");
+ System.setProperty("OTEL_EXPORTER_OTLP_PROTOCOL", "grpc");
+ System.setProperty("OTEL_TRACES_SAMPLER", "parentbased_always_on");
+ System.setProperty("OTEL_TRACES_EXPORTER", "otlp");
+ // Need to disable the exporters for metrics and logs
+ System.setProperty("OTEL_METRICS_EXPORTER", "none");
+ System.setProperty("OTEL_LOGS_EXPORTER", "none");
+ log.info("Configuring tracer {}...", getClass().getName());
Review Comment:
Removed the line and instead log a initializing line including detected configuration.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1061815367
##########
solr/modules/opentelemetry/build.gradle:
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Open Telemetry (OTEL) tracer'
+
+// This is a hacky way to use permitTestUnusedDeclared with bom declared dependencies.
+// See https://github.com/gradle-dependency-analyze/gradle-dependency-analyze/issues/108
+configurations {
+ constraintsOnly
+ permitUnusedDeclared.extendsFrom constraintsOnly
+ implementation.extendsFrom constraintsOnly
+}
+
+dependencies {
+ implementation project(':solr:core')
+
+ constraintsOnly(platform('io.opentelemetry:opentelemetry-bom-alpha'))
+
+ implementation 'io.opentracing:opentracing-api'
+ implementation 'org.slf4j:slf4j-api'
+
+ implementation 'io.opentelemetry:opentelemetry-sdk'
+ implementation 'io.opentelemetry:opentelemetry-sdk-trace'
+ implementation 'io.opentelemetry:opentelemetry-sdk-extension-autoconfigure'
+ implementation 'io.opentelemetry:opentelemetry-opentracing-shim'
+ implementation 'io.opentelemetry:opentelemetry-semconv'
Review Comment:
The three in lines 39-41 needed to be 'implementation', or else I got errors like:
```
/Users/janhoy/git/solr/solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/OtelTracerConfigurator.java:21: error: package io.opentelemetry.sdk.autoconfigure does not exist
import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdk;
```
I converted these to `runtimeOnly`:
* 'io.opentelemetry:opentelemetry-semconv'
* 'io.opentelemetry:opentelemetry-exporter-otlp'
* and all the grpc ones
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1061819187
##########
solr/solr-ref-guide/antora.yml:
##########
@@ -40,16 +40,17 @@ asciidoc:
# 'patch-version' is the 'z' part of x.y.z semantic version
page-solr-javadocs: 'https://solr.apache.org/docs/10_0_0'
solr-javadocs: 'https://solr.apache.org/docs/10_0_0'
- lucene-javadocs: 'https://lucene.apache.org/core/9_4_0'
+ lucene-javadocs: 'https://lucene.apache.org/core/9_4_2'
Review Comment:
Actually I found it strange that this was checked in at all, and as you can see it was somewhat out of date already. It is auto generated from template during build, so this time I copied it from the build/site folder after I ran `gradle solr:solr-ref-guide:buildLocalSite`. I wonder @HoustonPutman / @cpoerschke if there would be a way to auto update this file during a build, so that it would be flagged for commit.
It is not super critical if it is out of date, but the reason I updated it was due to IntelliJ's built-in asciidoc plugin complaining about my new variable not being defined, so it obviously reads this file.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy merged pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy merged PR #1168:
URL: https://github.com/apache/solr/pull/1168
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1061625026
##########
solr/modules/opentelemetry/build.gradle:
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Open Telemetry (OTEL) tracer'
+
+// This is a hacky way to use permitTestUnusedDeclared with bom declared dependencies.
+// See https://github.com/gradle-dependency-analyze/gradle-dependency-analyze/issues/108
+configurations {
+ constraintsOnly
+ permitUnusedDeclared.extendsFrom constraintsOnly
+ implementation.extendsFrom constraintsOnly
+}
+
+dependencies {
+ implementation project(':solr:core')
+
+ constraintsOnly(platform('io.opentelemetry:opentelemetry-bom-alpha'))
+
+ implementation 'io.opentracing:opentracing-api'
+ implementation 'org.slf4j:slf4j-api'
+
+ implementation 'io.opentelemetry:opentelemetry-sdk'
+ implementation 'io.opentelemetry:opentelemetry-sdk-trace'
+ implementation 'io.opentelemetry:opentelemetry-sdk-extension-autoconfigure'
+ implementation 'io.opentelemetry:opentelemetry-opentracing-shim'
+ implementation 'io.opentelemetry:opentelemetry-semconv'
Review Comment:
Are these actually `implementation`? RuntimeOnly might work? Especially since these are permit unused declared?
The same goes for the grpc stuff below.
##########
versions.props:
##########
@@ -20,8 +20,10 @@ commons-codec:commons-codec=1.15
commons-collections:commons-collections=3.2.2
commons-io:commons-io=2.11.0
io.dropwizard.metrics:*=4.2.12
+io.grpc:grpc-*=1.50.2
io.jaegertracing:*=1.8.1
io.netty:*=4.1.82.Final
+io.opentelemetry:opentelemetry-bom-alpha=1.21.0-alpha
Review Comment:
What version do we need to get to to NOT use an alpha version?
##########
solr/solr-ref-guide/antora.yml:
##########
@@ -40,16 +40,17 @@ asciidoc:
# 'patch-version' is the 'z' part of x.y.z semantic version
page-solr-javadocs: 'https://solr.apache.org/docs/10_0_0'
solr-javadocs: 'https://solr.apache.org/docs/10_0_0'
- lucene-javadocs: 'https://lucene.apache.org/core/9_4_0'
+ lucene-javadocs: 'https://lucene.apache.org/core/9_4_2'
Review Comment:
There is a note about this file not being manually edited:
```
# This file is autogenerated by the "setOfficialAntoraYaml" Gradle task, please do not edit manually.
# Do not run this task outside of the release process on a release branch (branch_9_0, branch_10_2, etc.).
# Feel free to run the task on unreleased branches (main, branch_9x, branch_10x) to update the dependency versions,
# but there is usually no need besides more up-to-date nightly builds of the ref-guide.
```
was this manually updated or did the task do it?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1062247311
##########
solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/OtelTracerConfigurator.java:
##########
@@ -0,0 +1,138 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.opentelemetry;
+
+import io.opentelemetry.opentracingshim.OpenTracingShim;
+import io.opentelemetry.sdk.OpenTelemetrySdk;
+import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdk;
+import io.opentracing.Tracer;
+import java.lang.invoke.MethodHandles;
+import java.util.HashMap;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Objects;
+import java.util.stream.Collectors;
+import org.apache.solr.core.TracerConfigurator;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * OpenTracing TracerConfigurator implementation which exports spans to OpenTelemetry in OTLP
+ * format. This impl re-uses the existing OpenTracing instrumentation through a shim, and takes care
+ * of properly closing the backing Tracer when Solr shuts down.
+ */
+public class OtelTracerConfigurator extends TracerConfigurator {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ // Copy of environment. Can be overridden by tests
+ Map<String, String> currentEnv = System.getenv();
+
+ @Override
+ public Tracer getTracer() {
+ setDefaultIfNotConfigured("OTEL_SERVICE_NAME", "solr");
+ setDefaultIfNotConfigured("OTEL_TRACES_EXPORTER", "otlp");
+ setDefaultIfNotConfigured("OTEL_EXPORTER_OTLP_PROTOCOL", "grpc");
+ setDefaultIfNotConfigured("OTEL_TRACES_SAMPLER", "parentbased_always_on");
+
+ final String currentConfig =
+ String.join(
+ "; ",
+ getCurrentOtelConfig().entrySet().stream()
+ .map(e -> e.getKey() + "=" + e.getValue())
+ .collect(Collectors.toSet()));
Review Comment:
See new PR #1275, i split it out as a method and sorted by key
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1044217897
##########
solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/OtelTracerConfigurator.java:
##########
@@ -0,0 +1,29 @@
+package org.apache.solr.opentelemetry;
+
+import io.opentelemetry.opentracingshim.OpenTracingShim;
+import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdk;
+import io.opentracing.Tracer;
+import org.apache.solr.core.TracerConfigurator;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.invoke.MethodHandles;
+
+public class OtelTracerConfigurator extends TracerConfigurator {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ @Override
+ public Tracer getTracer() {
+ // TODO: Find better way to set defaults
+ System.setProperty("OTEL_SERVICE_NAME", "solr");
+ System.setProperty("OTEL_EXPORTER_OTLP_PROTOCOL", "grpc");
+ System.setProperty("OTEL_TRACES_SAMPLER", "parentbased_always_on");
+ System.setProperty("OTEL_TRACES_EXPORTER", "otlp");
+ // Need to disable the exporters for metrics and logs
+ System.setProperty("OTEL_METRICS_EXPORTER", "none");
+ System.setProperty("OTEL_LOGS_EXPORTER", "none");
+ log.info("Configuring tracer {}...", getClass().getName());
Review Comment:
I'm keeping it for now. Could be that it could be moved to CoreContainer, but there will ever only be one tracer configured at a time, so not less code..
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1045099651
##########
solr/modules/opentelemetry/build.gradle:
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Open Telemetry (OTEL) tracer'
+
+dependencies {
+ implementation project(':solr:core')
+
+ implementation 'io.opentracing:opentracing-api'
+ implementation 'org.slf4j:slf4j-api'
+
+ implementation platform('io.opentelemetry:opentelemetry-bom-alpha')
+
+ implementation 'io.opentelemetry:opentelemetry-sdk-trace'
+ // TODO: permitUnusedDeclared does not work for the io.opentelemetry dependencies for some reason
+ permitUnusedDeclared 'io.opentelemetry:opentelemetry-sdk-trace'
Review Comment:
@risdenk Can you help with this, why don't `permitUnusedDeclared` work here? I get:
> Could not determine the dependencies of task ':solr:modules:opentelemetry:analyzeClassesDependencies'.
> Could not resolve all task dependencies for configuration ':solr:modules:opentelemetry:permitUnusedDeclared'.
> Could not find io.opentelemetry:opentelemetry-sdk-trace:.
> Required by:
> project :solr:modules:opentelemetry
> Could not find io.opentelemetry:opentelemetry-semconv:.
> Required by:
> project :solr:modules:opentelemetry
> Could not find io.opentelemetry:opentelemetry-exporter-otlp:.
> Required by:
> project :solr:modules:opentelemetry
>Possible solution:
> - Declare repository providing the artifact, see the documentation at
> https://docs.gradle.org/current/userguide/declaring_repositories.html
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on PR #1168:
URL: https://github.com/apache/solr/pull/1168#issuecomment-1308609799
Draft PR, just to collect some early feedback.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1018483452
##########
solr/modules/otel/build.gradle:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Open Telemetry (OTEL) tracer'
+
+dependencies {
+ implementation project(':solr:core')
+
+ // TODO: Prune down dependencies to the absolute necessary
+
+ implementation platform('io.opentelemetry:opentelemetry-bom')
+ implementation platform('io.opentelemetry:opentelemetry-bom-alpha')
+
+// implementation 'io.opentelemetry:opentelemetry-sdk-common'
+ implementation 'io.opentelemetry:opentelemetry-sdk-trace'
+ implementation 'io.opentelemetry:opentelemetry-sdk-extension-autoconfigure'
+ implementation 'io.opentelemetry:opentelemetry-opentracing-shim'
+ implementation 'io.opentelemetry:opentelemetry-semconv'
+ implementation 'io.opentelemetry:opentelemetry-exporter-otlp-trace:1.14.0'
+ implementation 'io.opentelemetry:opentelemetry-exporter-jaeger'
+ implementation 'io.opentelemetry:opentelemetry-exporter-zipkin'
+
+ // GRPC transport via netty - since we already ship netty this is cheaper than netty-shaded
+ runtimeOnly 'io.grpc:grpc-netty'
+ implementation 'io.grpc:grpc-protobuf'
+ implementation 'io.grpc:grpc-stub'
+ implementation 'io.grpc:grpc-context'
Review Comment:
I'm leaving in grpc for now, as it is also much more efficient (binary protobuf over HTTP/2) than the alternatives. The okhttp library is used for both jaeger and zipkin exporters, as well as for OTLP/HTTP.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1018211222
##########
solr/modules/otel/README.md:
##########
@@ -0,0 +1,33 @@
+Apache Solr Open Telemetry Tracer
+=====================================
+
+Introduction
+------------
+This module brings support for the new [OTEL](https://opentelemetry.io) standard,
+and exposes a tracer configurator that can be enabled in the
+`<tracerConfig>` tag of `solr.xml`:
+
+```xml
+<tracerConfig name="tracerConfig" class="org.apache.solr.otel.OtelTracerConfigurator" />
Review Comment:
Yes, I agree to change module and package name. But I like a descriptive class name too, since `TracerConfigurator` is the abstract class name. So if package name is `opentelemetry` i think class name can safely be `OtelTracerConfigurator`.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on PR #1168:
URL: https://github.com/apache/solr/pull/1168#issuecomment-1371092716
I have now also tested locally, exporting to a local otel-collector, on both default grpc, port 4317 and with `OTEL_EXPORTER_OTLP_PROTOCOL=http/protobuf` on port 4318. Both works fine. If the collector is not started you get this log error msg in stderr:
```
jan. 04, 2023 3:37:31 P.M. io.opentelemetry.sdk.internal.ThrottlingLogger doLog
SEVERE: Failed to export spans. The request could not be executed. Full error message: unexpected end of stream on http://localhost:4318/...
```
Any last reqeusts before I merge for 9.2?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on PR #1168:
URL: https://github.com/apache/solr/pull/1168#issuecomment-1370582267
PS: Imagine if Java had a http-client abstraction lib, like slf4j for logs. Then we could delegate to Jetty-client.. Well well.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1061837911
##########
solr/modules/opentelemetry/build.gradle:
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Open Telemetry (OTEL) tracer'
+
+// This is a hacky way to use permitTestUnusedDeclared with bom declared dependencies.
+// See https://github.com/gradle-dependency-analyze/gradle-dependency-analyze/issues/108
+configurations {
+ constraintsOnly
+ permitUnusedDeclared.extendsFrom constraintsOnly
+ implementation.extendsFrom constraintsOnly
+ runtimeOnly.extendsFrom constraintsOnly
+}
+
+dependencies {
+ implementation project(':solr:core')
+
+ constraintsOnly(platform('io.opentelemetry:opentelemetry-bom-alpha'))
+
+ implementation 'io.opentracing:opentracing-api'
+ implementation 'org.slf4j:slf4j-api'
+
+ implementation 'io.opentelemetry:opentelemetry-sdk'
+ implementation 'io.opentelemetry:opentelemetry-sdk-trace'
+ implementation 'io.opentelemetry:opentelemetry-sdk-extension-autoconfigure'
+ implementation 'io.opentelemetry:opentelemetry-opentracing-shim'
+ runtimeOnly 'io.opentelemetry:opentelemetry-semconv'
+ permitUnusedDeclared 'io.opentelemetry:opentelemetry-semconv'
+ runtimeOnly 'io.opentelemetry:opentelemetry-exporter-otlp'
+ permitUnusedDeclared 'io.opentelemetry:opentelemetry-exporter-otlp'
+ // End users must recompile with jaeger exporter and/or zipkin exporter if they need these
+
+ // NOTE: sdk-autoconfigure needs both opentelemetry-sdk-metrics and opentelemetry-sdk-logs even if we don't use them
+
+ // gRPC transport via netty - since we already ship netty this is more lightweight than netty-shaded
+ runtimeOnly 'io.grpc:grpc-netty'
+ permitUnusedDeclared 'io.grpc:grpc-netty'
Review Comment:
added a separate comment but - `platform('io.opentelemetry:opentelemetry-bom-alpha')` should do it.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1061828235
##########
solr/modules/opentelemetry/build.gradle:
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Open Telemetry (OTEL) tracer'
+
+// This is a hacky way to use permitTestUnusedDeclared with bom declared dependencies.
+// See https://github.com/gradle-dependency-analyze/gradle-dependency-analyze/issues/108
+configurations {
+ constraintsOnly
+ permitUnusedDeclared.extendsFrom constraintsOnly
+ implementation.extendsFrom constraintsOnly
+ runtimeOnly.extendsFrom constraintsOnly
+}
+
+dependencies {
+ implementation project(':solr:core')
+
+ constraintsOnly(platform('io.opentelemetry:opentelemetry-bom-alpha'))
+
+ implementation 'io.opentracing:opentracing-api'
+ implementation 'org.slf4j:slf4j-api'
+
+ implementation 'io.opentelemetry:opentelemetry-sdk'
+ implementation 'io.opentelemetry:opentelemetry-sdk-trace'
+ implementation 'io.opentelemetry:opentelemetry-sdk-extension-autoconfigure'
+ implementation 'io.opentelemetry:opentelemetry-opentracing-shim'
+ runtimeOnly 'io.opentelemetry:opentelemetry-semconv'
+ permitUnusedDeclared 'io.opentelemetry:opentelemetry-semconv'
+ runtimeOnly 'io.opentelemetry:opentelemetry-exporter-otlp'
+ permitUnusedDeclared 'io.opentelemetry:opentelemetry-exporter-otlp'
+ // End users must recompile with jaeger exporter and/or zipkin exporter if they need these
+
+ // NOTE: sdk-autoconfigure needs both opentelemetry-sdk-metrics and opentelemetry-sdk-logs even if we don't use them
+
+ // gRPC transport via netty - since we already ship netty this is more lightweight than netty-shaded
+ runtimeOnly 'io.grpc:grpc-netty'
+ permitUnusedDeclared 'io.grpc:grpc-netty'
Review Comment:
Yey, after moving those to runtimeOnly above, we no longer need `permitUnusedDeclared`. I removed it all, and moved the bom declaration to `runtimeOnly`, would that be correct? gradle check does not complain...
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1062246728
##########
solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/ClosableTracerShim.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.opentelemetry;
+
+import io.opentelemetry.sdk.trace.SdkTracerProvider;
+import io.opentracing.Scope;
+import io.opentracing.ScopeManager;
+import io.opentracing.Span;
+import io.opentracing.SpanContext;
+import io.opentracing.Tracer;
+import io.opentracing.propagation.Format;
+import java.lang.invoke.MethodHandles;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Delegate shim that forwards all calls to the actual {@link
+ * io.opentelemetry.opentracingshim.OpenTracingShim}, and in addition calls {@link
+ * SdkTracerProvider#close()} to really close the OTEL SDK tracer when the OT shim is closed.
+ *
+ * <p>TODO: This can be removed once we migrate Solr instrumentation from OpenTracing to
+ * OpenTelemetry
+ */
+public class ClosableTracerShim implements Tracer {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ private final Tracer shim;
+ private final SdkTracerProvider sdkTracerProvider;
+
+ public ClosableTracerShim(Tracer shim, SdkTracerProvider sdkTracerProvider) {
+ this.shim = shim;
+ this.sdkTracerProvider = sdkTracerProvider;
+ }
+
+ @Override
+ public ScopeManager scopeManager() {
+ return shim.scopeManager();
+ }
+
+ @Override
+ public Span activeSpan() {
+ return shim.activeSpan();
+ }
+
+ @Override
+ public Scope activateSpan(Span span) {
+ return shim.activateSpan(span);
+ }
+
+ @Override
+ public SpanBuilder buildSpan(String operationName) {
+ return shim.buildSpan(operationName);
+ }
+
+ @Override
+ public <C> void inject(SpanContext spanContext, Format<C> format, C carrier) {
+ shim.inject(spanContext, format, carrier);
+ }
+
+ @Override
+ public <C> SpanContext extract(Format<C> format, C carrier) {
+ return shim.extract(format, carrier);
+ }
+
+ @Override
+ public void close() {
+ shim.close();
+ log.info("Closing wrapped OTEL tracer instance.");
Review Comment:
See new PR #1275
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on PR #1168:
URL: https://github.com/apache/solr/pull/1168#issuecomment-1347390755
Thanks @risdenk for fixing the weird gradle thing, that was a tricky one!
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on PR #1168:
URL: https://github.com/apache/solr/pull/1168#issuecomment-1350668244
Any idea for how to or whether to build an integration test for this? Perpaps a BATS test verifying some log lines is sufficient. I'm not keen on trying to mock an OTEL collector.
I have not tested as much manually as I wanted either, I'll probably do that next and I can publish a recipe here with a docker-compose.yml that stands up a collector and a jaeger UI on localhost that "just works" with the default config.
Wrt bootstrapping without adding a tag to solr.xml, I'm thinking SPI could be a solution, but that's for another JIRA.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1046063624
##########
solr/modules/opentelemetry/build.gradle:
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Open Telemetry (OTEL) tracer'
+
+dependencies {
+ implementation project(':solr:core')
+
+ implementation 'io.opentracing:opentracing-api'
+ implementation 'org.slf4j:slf4j-api'
+
+ implementation platform('io.opentelemetry:opentelemetry-bom-alpha')
+
+ implementation 'io.opentelemetry:opentelemetry-sdk-trace'
+ // TODO: permitUnusedDeclared does not work for the io.opentelemetry dependencies for some reason
+ permitUnusedDeclared 'io.opentelemetry:opentelemetry-sdk-trace'
Review Comment:
Hmm interesting yea I can hopefully take a look this week.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on PR #1168:
URL: https://github.com/apache/solr/pull/1168#issuecomment-1344026969
Also, I'm wondering. Could we not let this module register itself as `GlobalTracer` if enabled? I mean, if you enable the module with `-Dsolr.modules=opentelemetry` then chances are you want to use it as well? It would relieve users from having to edit `solr.xml`.
Alternatively we could gate this on the environment variable `OTEL_SDK_DISABLED`, so that if a user for some reason configures several tracer modules, or adds the Datadog agent, then setting `-Dotel.sdk.disabled=true` would prevent our module from registering as `GlobalTracer`. But I'd like that the experience for typical use should be as simple as possible. Add the module and configure it with env.vars/sysprops.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1061818303
##########
solr/modules/opentelemetry/build.gradle:
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Open Telemetry (OTEL) tracer'
+
+// This is a hacky way to use permitTestUnusedDeclared with bom declared dependencies.
+// See https://github.com/gradle-dependency-analyze/gradle-dependency-analyze/issues/108
+configurations {
+ constraintsOnly
+ permitUnusedDeclared.extendsFrom constraintsOnly
+ implementation.extendsFrom constraintsOnly
+ runtimeOnly.extendsFrom constraintsOnly
+}
+
+dependencies {
+ implementation project(':solr:core')
+
+ constraintsOnly(platform('io.opentelemetry:opentelemetry-bom-alpha'))
+
+ implementation 'io.opentracing:opentracing-api'
+ implementation 'org.slf4j:slf4j-api'
+
+ implementation 'io.opentelemetry:opentelemetry-sdk'
+ implementation 'io.opentelemetry:opentelemetry-sdk-trace'
+ implementation 'io.opentelemetry:opentelemetry-sdk-extension-autoconfigure'
+ implementation 'io.opentelemetry:opentelemetry-opentracing-shim'
+ runtimeOnly 'io.opentelemetry:opentelemetry-semconv'
+ permitUnusedDeclared 'io.opentelemetry:opentelemetry-semconv'
+ runtimeOnly 'io.opentelemetry:opentelemetry-exporter-otlp'
+ permitUnusedDeclared 'io.opentelemetry:opentelemetry-exporter-otlp'
+ // End users must recompile with jaeger exporter and/or zipkin exporter if they need these
+
+ // NOTE: sdk-autoconfigure needs both opentelemetry-sdk-metrics and opentelemetry-sdk-logs even if we don't use them
+
+ // gRPC transport via netty - since we already ship netty this is more lightweight than netty-shaded
+ runtimeOnly 'io.grpc:grpc-netty'
+ permitUnusedDeclared 'io.grpc:grpc-netty'
Review Comment:
If these are runtimeOnly, I'm 90% sure that we don't need `permitUnusedDeclared` anymore.
I think this means that `permitUnusedDeclared` isn't needed can be removed from the entire build.gradle and the section for
```
configurations {
constraintsOnly
permitUnusedDeclared.extendsFrom constraintsOnly
implementation.extendsFrom constraintsOnly
runtimeOnly.extendsFrom constraintsOnly
}
```
which is just to handle these bom unused declared can be removed as well?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1017973727
##########
settings.gradle:
##########
@@ -34,6 +34,7 @@ include "solr:server"
include "solr:modules:analysis-extras"
include "solr:modules:analytics"
include "solr:modules:clustering"
+include "solr:modules:otel"
Review Comment:
I'd prefer the module name to be `opentelemetry`
I think we should use the full name where possible.
##########
solr/modules/otel/README.md:
##########
@@ -0,0 +1,33 @@
+Apache Solr Open Telemetry Tracer
+=====================================
+
+Introduction
+------------
+This module brings support for the new [OTEL](https://opentelemetry.io) standard,
+and exposes a tracer configurator that can be enabled in the
+`<tracerConfig>` tag of `solr.xml`:
+
+```xml
+<tracerConfig name="tracerConfig" class="org.apache.solr.otel.OtelTracerConfigurator" />
Review Comment:
How about the package and class being: `org.apache.solr.opentelemetry.TracerConfigurator`
##########
solr/modules/otel/build.gradle:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Open Telemetry (OTEL) tracer'
+
+dependencies {
+ implementation project(':solr:core')
+
+ // TODO: Prune down dependencies to the absolute necessary
+
+ implementation platform('io.opentelemetry:opentelemetry-bom')
+ implementation platform('io.opentelemetry:opentelemetry-bom-alpha')
+
+// implementation 'io.opentelemetry:opentelemetry-sdk-common'
+ implementation 'io.opentelemetry:opentelemetry-sdk-trace'
+ implementation 'io.opentelemetry:opentelemetry-sdk-extension-autoconfigure'
+ implementation 'io.opentelemetry:opentelemetry-opentracing-shim'
+ implementation 'io.opentelemetry:opentelemetry-semconv'
+ implementation 'io.opentelemetry:opentelemetry-exporter-otlp-trace:1.14.0'
+ implementation 'io.opentelemetry:opentelemetry-exporter-jaeger'
+ implementation 'io.opentelemetry:opentelemetry-exporter-zipkin'
+
+ // GRPC transport via netty - since we already ship netty this is cheaper than netty-shaded
+ runtimeOnly 'io.grpc:grpc-netty'
+ implementation 'io.grpc:grpc-protobuf'
+ implementation 'io.grpc:grpc-stub'
+ implementation 'io.grpc:grpc-context'
Review Comment:
Do we need these `grpc` dependencies? I don't see them in any of our code? If we can remove them we don't need the `grpc` version in `versions.props` either.
##########
solr/modules/otel/build.gradle:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Open Telemetry (OTEL) tracer'
+
+dependencies {
+ implementation project(':solr:core')
+
+ // TODO: Prune down dependencies to the absolute necessary
+
+ implementation platform('io.opentelemetry:opentelemetry-bom')
+ implementation platform('io.opentelemetry:opentelemetry-bom-alpha')
Review Comment:
Seems weird to need both bom and bom-alpha
##########
solr/modules/otel/README.md:
##########
@@ -0,0 +1,33 @@
+Apache Solr Open Telemetry Tracer
+=====================================
+
+Introduction
+------------
+This module brings support for the new [OTEL](https://opentelemetry.io) standard,
+and exposes a tracer configurator that can be enabled in the
+`<tracerConfig>` tag of `solr.xml`:
+
+```xml
+<tracerConfig name="tracerConfig" class="org.apache.solr.otel.OtelTracerConfigurator" />
+```
+
+The tracer can be configured with environment variables, see https://opentelemetry.io/docs/concepts/sdk-configuration/otlp-exporter-configuration/ and users can change both the exprter, trace propagator and many other settings.
+
+The defaults are:
+
+```
+OTEL_SDK_DISABLED=false
Review Comment:
I think I'd prefer `OPENTELEMETRY_...` but it is a bit longer. It does make it 100% clear what this is for.
##########
solr/modules/otel/build.gradle:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Open Telemetry (OTEL) tracer'
+
+dependencies {
+ implementation project(':solr:core')
+
+ // TODO: Prune down dependencies to the absolute necessary
+
+ implementation platform('io.opentelemetry:opentelemetry-bom')
+ implementation platform('io.opentelemetry:opentelemetry-bom-alpha')
+
+// implementation 'io.opentelemetry:opentelemetry-sdk-common'
+ implementation 'io.opentelemetry:opentelemetry-sdk-trace'
+ implementation 'io.opentelemetry:opentelemetry-sdk-extension-autoconfigure'
+ implementation 'io.opentelemetry:opentelemetry-opentracing-shim'
+ implementation 'io.opentelemetry:opentelemetry-semconv'
+ implementation 'io.opentelemetry:opentelemetry-exporter-otlp-trace:1.14.0'
+ implementation 'io.opentelemetry:opentelemetry-exporter-jaeger'
+ implementation 'io.opentelemetry:opentelemetry-exporter-zipkin'
+
+ // GRPC transport via netty - since we already ship netty this is cheaper than netty-shaded
+ runtimeOnly 'io.grpc:grpc-netty'
+ implementation 'io.grpc:grpc-protobuf'
+ implementation 'io.grpc:grpc-stub'
+ implementation 'io.grpc:grpc-context'
+ compileOnly 'org.apache.tomcat:annotations-api'
Review Comment:
Can you add a comment like:
```
// See https://issues.apache.org/jira/browse/LOG4J2-3609 due to needing these annotations
```
since my guess is LOG4J2-3609 is why we need this compileOnly annotation.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1060205004
##########
solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/OtelTracerConfiguratorTest.java:
##########
@@ -73,4 +86,24 @@ public void testSetDefaultIfNotConfigured() {
assertEquals("default", instance.getCurrentOtelConfig().get("OTEL_YEY"));
assertEquals("prop-k1", instance.getCurrentOtelConfig().get("OTEL_K1"));
}
+
+ @Test
+ public void testInjected() throws Exception {
+ // Make sure the batch exporter times out before our thread lingering time of 10s
+ instance.setDefaultIfNotConfigured("OTEL_BSP_SCHEDULE_DELAY", "1000");
+ instance.setDefaultIfNotConfigured("OTEL_BSP_EXPORT_TIMEOUT", "2000");
+ MiniSolrCloudCluster cluster =
+ new MiniSolrCloudCluster.Builder(2, createTempDir())
+ .addConfig("config", TEST_PATH().resolve("collection1").resolve("conf"))
+ .withSolrXml(getFile("solr/solr.xml").toPath())
+ .build();
+ try {
+ TimeOut timeOut = new TimeOut(2, TimeUnit.MINUTES, TimeSource.NANO_TIME);
+ timeOut.waitFor(
+ "Waiting for GlobalTracer is registered",
+ () -> GlobalTracer.get().toString().contains("TracerShim"));
Review Comment:
When running this test locally, it certainly executes fast. Perhaps MiniSolrCloudCluster behaved differently back in 2019 when Jaeger was added? Anyway, I removed the timeout stuff.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1060089112
##########
solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/OtelTracerConfiguratorTest.java:
##########
@@ -73,4 +86,24 @@ public void testSetDefaultIfNotConfigured() {
assertEquals("default", instance.getCurrentOtelConfig().get("OTEL_YEY"));
assertEquals("prop-k1", instance.getCurrentOtelConfig().get("OTEL_K1"));
}
+
+ @Test
+ public void testInjected() throws Exception {
+ // Make sure the batch exporter times out before our thread lingering time of 10s
+ instance.setDefaultIfNotConfigured("OTEL_BSP_SCHEDULE_DELAY", "1000");
+ instance.setDefaultIfNotConfigured("OTEL_BSP_EXPORT_TIMEOUT", "2000");
+ MiniSolrCloudCluster cluster =
+ new MiniSolrCloudCluster.Builder(2, createTempDir())
+ .addConfig("config", TEST_PATH().resolve("collection1").resolve("conf"))
+ .withSolrXml(getFile("solr/solr.xml").toPath())
+ .build();
+ try {
+ TimeOut timeOut = new TimeOut(2, TimeUnit.MINUTES, TimeSource.NANO_TIME);
+ timeOut.waitFor(
+ "Waiting for GlobalTracer is registered",
+ () -> GlobalTracer.get().toString().contains("TracerShim"));
Review Comment:
this is surprising... is the shim installed in some async thread?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1060199981
##########
solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/OtelTracerConfiguratorTest.java:
##########
@@ -73,4 +86,24 @@ public void testSetDefaultIfNotConfigured() {
assertEquals("default", instance.getCurrentOtelConfig().get("OTEL_YEY"));
assertEquals("prop-k1", instance.getCurrentOtelConfig().get("OTEL_K1"));
}
+
+ @Test
+ public void testInjected() throws Exception {
+ // Make sure the batch exporter times out before our thread lingering time of 10s
+ instance.setDefaultIfNotConfigured("OTEL_BSP_SCHEDULE_DELAY", "1000");
+ instance.setDefaultIfNotConfigured("OTEL_BSP_EXPORT_TIMEOUT", "2000");
+ MiniSolrCloudCluster cluster =
+ new MiniSolrCloudCluster.Builder(2, createTempDir())
+ .addConfig("config", TEST_PATH().resolve("collection1").resolve("conf"))
+ .withSolrXml(getFile("solr/solr.xml").toPath())
+ .build();
+ try {
+ TimeOut timeOut = new TimeOut(2, TimeUnit.MINUTES, TimeSource.NANO_TIME);
+ timeOut.waitFor(
+ "Waiting for GlobalTracer is registered",
+ () -> GlobalTracer.get().toString().contains("TracerShim"));
Review Comment:
What's surprising? That we need to wait for the global tracer to be registered? Guess we're waiting for the `MiniSolrCloudCluster` to bootstrap, which will eventually lead to CoreContainer registering the tracer. This test was copied from Jaeger module.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on PR #1168:
URL: https://github.com/apache/solr/pull/1168#issuecomment-1370609810
Opened a discussion here https://github.com/open-telemetry/opentelemetry-java/discussions/5079 to explore options.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1043945805
##########
solr/modules/opentelemetry/build.gradle:
##########
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Open Telemetry (OTEL) tracer'
+
+dependencies {
+ implementation project(':solr:core')
+
+ // TODO: Prune down dependencies to the absolute necessary
+
+ implementation platform('io.opentelemetry:opentelemetry-bom-alpha')
+
+ implementation 'io.opentelemetry:opentelemetry-sdk-trace'
+ implementation 'io.opentelemetry:opentelemetry-sdk-extension-autoconfigure'
+ implementation 'io.opentelemetry:opentelemetry-opentracing-shim'
+ implementation 'io.opentelemetry:opentelemetry-semconv'
+ implementation 'io.opentelemetry:opentelemetry-exporter-otlp'
Review Comment:
Yes, makes sense to only ship otlp exporter, removed the others.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1046121655
##########
solr/modules/opentelemetry/build.gradle:
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Open Telemetry (OTEL) tracer'
+
+dependencies {
+ implementation project(':solr:core')
+
+ implementation 'io.opentracing:opentracing-api'
+ implementation 'org.slf4j:slf4j-api'
+
+ implementation platform('io.opentelemetry:opentelemetry-bom-alpha')
+
+ implementation 'io.opentelemetry:opentelemetry-sdk-trace'
+ // TODO: permitUnusedDeclared does not work for the io.opentelemetry dependencies for some reason
+ permitUnusedDeclared 'io.opentelemetry:opentelemetry-sdk-trace'
Review Comment:
Fixed in 3628770635f09747c6ad0c0ee00c77c3e9a3cff3
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1046256426
##########
solr/modules/opentelemetry/src/java/org/apache/solr/opentelemetry/OtelTracerConfigurator.java:
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.opentelemetry;
+
+import io.opentelemetry.opentracingshim.OpenTracingShim;
+import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdk;
+import io.opentracing.Tracer;
+import java.lang.invoke.MethodHandles;
+import java.util.HashMap;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Objects;
+import java.util.stream.Collectors;
+import org.apache.solr.core.TracerConfigurator;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * OpenTracing TracerConfigurator implementation which exports spans to OpenTelemetry in OTLP
+ * format. This impl re-uses the existing OpenTracing instrumentation through a shim.
+ */
+public class OtelTracerConfigurator extends TracerConfigurator {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ // Copy of environment. Can be overridden by tests
+ Map<String, String> currentEnv = System.getenv();
+
+ @Override
+ public Tracer getTracer() {
+ setDefaultIfNotConfigured("OTEL_SERVICE_NAME", "solr");
+ setDefaultIfNotConfigured("OTEL_TRACES_EXPORTER", "otlp");
+ setDefaultIfNotConfigured("OTEL_EXPORTER_OTLP_PROTOCOL", "grpc");
+ setDefaultIfNotConfigured("OTEL_TRACES_SAMPLER", "parentbased_always_on");
+
+ if (log.isInfoEnabled()) {
+ log.info(
+ "OpenTelemetry tracer enabled with configuration: {}",
+ String.join(
+ "; ",
+ getCurrentOtelConfig().entrySet().stream()
+ .map(e -> String.format(Locale.ROOT, "%s=%s", e.getKey(), e.getValue()))
Review Comment:
dubious use of String.format when trivial String concatenation is shorter and just as clear
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1018206430
##########
solr/modules/otel/build.gradle:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Open Telemetry (OTEL) tracer'
+
+dependencies {
+ implementation project(':solr:core')
+
+ // TODO: Prune down dependencies to the absolute necessary
+
+ implementation platform('io.opentelemetry:opentelemetry-bom')
+ implementation platform('io.opentelemetry:opentelemetry-bom-alpha')
Review Comment:
I followed a blog post. The `opentelemetry-sdk-extension-autoconfigure` is still alpha. But I think you're right that it's enough with the alpha bom.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1168: SOLR-16532 New OTEL module with OTLP trace exporter
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1168:
URL: https://github.com/apache/solr/pull/1168#discussion_r1018209370
##########
solr/modules/otel/README.md:
##########
@@ -0,0 +1,33 @@
+Apache Solr Open Telemetry Tracer
+=====================================
+
+Introduction
+------------
+This module brings support for the new [OTEL](https://opentelemetry.io) standard,
+and exposes a tracer configurator that can be enabled in the
+`<tracerConfig>` tag of `solr.xml`:
+
+```xml
+<tracerConfig name="tracerConfig" class="org.apache.solr.otel.OtelTracerConfigurator" />
+```
+
+The tracer can be configured with environment variables, see https://opentelemetry.io/docs/concepts/sdk-configuration/otlp-exporter-configuration/ and users can change both the exprter, trace propagator and many other settings.
+
+The defaults are:
+
+```
+OTEL_SDK_DISABLED=false
Review Comment:
These are OTEL standardized env variables, which works across all language implementations for the clients. So we cannot change them :) See the link above.
Anyway, need to keep this README quite short, as only devs will see it. Will probably move stuff to RefGuide...
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org