Allow OTLP gRPC exporters to work without grpc-java using okhttp directly. #3684

anuraaga · 2021-09-30T05:28:06Z

Serializing in gRPC wire format is actually quite trivial (parsing is way harder) - I've used this approach in Zipkin server in the past to allow using gRPC without any dependencies on grpc-java.

Throwing this out there, I used an approach that checks if grpc-java is on the classpath, and if it isn't while okhttp is, switches to the okhttp implementation. This exporter should work fine in general, and avoids ~4MB of dependencies brought in by grpc-okhttp, such as its guava dependencies. There is interest in this for a slim distribution of the javaagent which would clock in at 10MB without any difference in semantics from the full.

anuraaga · 2021-09-30T05:28:43Z

dependencyManagement/build.gradle.kts

@@ -75,8 +75,8 @@ val DEPENDENCIES = listOf(
  // using old version of okhttp to avoid pulling in kotlin stdlib
  // not using (old) okhttp bom because that is pulling in old guava version
  // and overriding the guava bom
-  "com.squareup.okhttp3:okhttp:3.12.13",
-  "com.squareup.okhttp3:okhttp-tls:3.12.13",
+  "com.squareup.okhttp3:okhttp:3.14.9",


We were accidentally not using the latest version (.12.13 appeared newer but was actually a backport of a fix to older okhttp3)

anuraaga · 2021-09-30T05:29:44Z

If it seems OK to proceed, then I'll start by extracting common logic in the HTTP exporters into an internal utility first

anuraaga · 2021-09-30T06:01:28Z

...http/trace/src/main/java/io/opentelemetry/exporter/otlp/http/trace/OtlpHttpSpanExporter.java

+    String grpcStatus = response.header(GRPC_STATUS);
+    if (grpcStatus == null) {
+      try {
+        grpcStatus = response.trailers().get(GRPC_STATUS);


By the way, I had an idea of trying out a completely no-deps artifact for Java 11. But then I found this 😂

https://github.com/openjdk/jdk/blob/master/src/java.net.http/share/classes/jdk/internal/net/http/HttpResponseImpl.java#L65

Well that's unfortunate. 😬

jack-berg · 2021-09-30T14:58:31Z

I think this is a cool idea!

jack-berg · 2021-09-30T15:01:52Z

...ace/src/main/java/io/opentelemetry/exporter/otlp/http/trace/OtlpHttpSpanExporterBuilder.java

@@ -127,9 +129,28 @@ public OtlpHttpSpanExporter build() {
      }
    }


I would consider adding a new method, maybe called setGrpcEndpoint(String endpoint), which expects just an endpoint with scheme, host, and port (like in the otlp grpc exporter builders). If that endpoint is called, grpc is enabled and the correct path GRPC_ENDPOINT_PATH is automatically appended.

An upside of your approach where grpc is detected based on the endpoint is that this would work out of the box with autoconfigure - no additional properties needed.

anuraaga · 2021-10-01T08:17:47Z

I'm working on a different approach that actually has the normal otlp-trace artifact (the gRPC OTLP exporter) detect if grpc-java isn't on the classpath while okhttp is and use the okhttp version instead. This makes the "API" for using this exporter a POM exclusion - not ideal due to low discoverability, but as the objective is reducing dependencies, at the same time it sort of feels reasonable to use the POM for it too. Will ping back after prototyping.

anuraaga · 2021-10-01T08:41:01Z

@jack-berg By the way you mentioned having ideas for cleaning up some duplication among the exporters. I've ended up doing some of that in my not-yet-pushed code you may want to check to see if that pattern aligns with it or should be changed to align with it.

anuraaga · 2021-10-04T06:48:48Z

I have tacked on a different approach to this PR, everything except what's in otlp/http module.

7929de0

It detects whether the classpath is missing grpc-java, and includes okhttp, and uses an okhttp implementation automatically for that case.

While the code may look like it's highly invasive for this use case, it's actually at the same time solving an issue we currently have in that all the gRPC exporters duplicate a lot of code. The extraction of GrpcExporter and GrpcExporterBuilder which are delegated to from each of the 4 gRPC exporters (which I'll do if the approach seems fine, currently only applied it to trace) is probably a good idea regardless and will cut down on a lot of code. So supporting the okhttp-only approach means extracting the interface and having the okhttp implementation - I think it's low enough code overhead over grpc-only and has good enough benefit for the agent to be worth it. We could also consider shading okhttp/okio (~500KB) in now as it would allow any of the gRPC or HTTP exporters to function without dependencies (notably, solving the grpc-netty needs to be added by end users issue that spring has raised in the past). @jkwatson what do you think?

jkwatson · 2021-10-04T19:32:17Z

This does seem like a decent approach to removing the code duplication. I do find the various names of the classes a bit hard to keep straight in my head without a bunch of thinking:

OtlpGrpcSpanExporter
OtlpHttpSpanExporter
GrpcExporter
OkHttpGrpcExporter
DefaultGrpcExporter

(and the various Builders as well)

I'm not saying any of these names is wrong...just that they all blur together in my brain when I try to think about which is which. I suspect it will get worse when Metrics are involved.

I guess the question is... is it worth the increase in inherent complexity in these classes to remove the code duplication? How often will we need to touch these classes in the future?

jack-berg · 2021-10-04T20:59:45Z

@anuraaga I went down the road of trying to use interfaces / abstract classes to DRY up the OTLP builders / exporters in the past and at the time concluded that the increased cognitive overhead wasn't worth the code reduction, especially considering how these are likely to stabilize soon.

My idea for code reuse was to have a variety of utility functions that the builders / exporters could call.

For example, we could have a grpc export function like:

  public static <REQUEST, RESPONSE> CompletableResultCode grpcExport(
      ThrottlingLogger logger,
      String type,
      Function<REQUEST, ListenableFuture<RESPONSE>> exporter,
      REQUEST request) {
    final CompletableResultCode result = new CompletableResultCode();

    Futures.addCallback(
        exporter.apply(request),
        new FutureCallback<RESPONSE>() {
          @Override
          public void onSuccess(@Nullable RESPONSE response) {
            result.succeed();
          }

          @Override
          public void onFailure(Throwable t) {
            Status status = Status.fromThrowable(t);
            switch (status.getCode()) {
              case UNIMPLEMENTED:
                logger.log(
                    Level.SEVERE,
                    "Failed to export "
                        + type
                        + ". Server responded with UNIMPLEMENTED. "
                        + "This usually means that your collector is not configured with an otlp "
                        + "receiver in the \"pipelines\" section of the configuration. "
                        + "Full error message: "
                        + t.getMessage());
                break;
              case UNAVAILABLE:
                logger.log(
                    Level.SEVERE,
                    "Failed to export "
                        + type
                        + ". Server is UNAVAILABLE. "
                        + "Make sure your collector is running and reachable from this network. "
                        + "Full error message:"
                        + t.getMessage());
                break;
              default:
                logger.log(
                    Level.WARNING,
                    "Failed to export " + type + ". Error message: " + t.getMessage());
                break;
            }
            if (logger.isLoggable(Level.FINEST)) {
              logger.log(Level.FINEST, "Failed to export + " + type + ". Details follow: " + t);
            }
            result.fail();
          }
        },
        MoreExecutors.directExecutor());
    return result;
  }

Which would be called in the grpc exporters such as OtlpGrpcSpanExporter with:

  /**
   * Submits all the given spans in a single batch to the OpenTelemetry collector.
   *
   * @param spans the list of sampled Spans to be exported.
   * @return the result of the operation
   */
  @Override
  public CompletableResultCode export(Collection<SpanData> spans) {
    spansSeen.add(spans.size());
    TraceRequestMarshaler request = TraceRequestMarshaler.create(spans);
    MarshalerTraceServiceGrpc.TraceServiceFutureStub exporter =
        traceService.withDeadlineAfter(timeoutNanos, TimeUnit.NANOSECONDS);
    return GrpcExportUtil.grpcExport(logger, "spans", exporter::export, request);
  }

Or for example take the endpoint validation that is duplicated across all the builders. We could add a utility method like:

  public static URI validateAndGetEndpointUri(String endpoint) {
    requireNonNull(endpoint, "endpoint");

    URI uri;
    try {
      uri = new URI(endpoint);
    } catch (URISyntaxException e) {
      throw new IllegalArgumentException("Invalid endpoint, must be a URL: " + endpoint, e);
    }

    if (uri.getScheme() == null
        || (!uri.getScheme().equals("http") && !uri.getScheme().equals("https"))) {
      throw new IllegalArgumentException(
          "Invalid endpoint, must start with http:// or https://: " + uri);
    }
    return uri;
  }

And the builders such as OtlpGrpcSpanExporterBuilder could use it like:

  /**
   * Sets the OTLP endpoint to connect to. If unset, defaults to {@value DEFAULT_ENDPOINT_URL}. The
   * endpoint must start with either http:// or https://.
   */
  public OtlpGrpcSpanExporterBuilder setEndpoint(String endpoint) {
    this.endpoint = OtlpBuilderUtil.validateAndGetEndpointUri(endpoint);
    return this;
  }

The idea being to DRY up the consequential code without fussing with OOP questions like inheritance.

anuraaga · 2021-10-05T00:53:59Z

@jkwatson Yeah the names were definitely something that came to mind, especially Default*. But this is what I came up with. Should I go ahead and apply the pattern to the other exporters the and see where we end up?

@jack-berg Here the shared code is in a class that is delegated to, not part of the inheritance hierarchy so think it may not have that problem. Notably the only difference with helper methods seems to be whether it's a static method or not - static methods would have the downside of not allowing this approach of switching implementations for okhttp.

…nto grpc-nodeps

anuraaga · 2021-10-05T07:44:23Z

...p/common/src/main/java/io/opentelemetry/exporter/otlp/internal/grpc/DefaultGrpcExporter.java

+ * <p>This class is internal and is hence not for public use. Its APIs are unstable and can change
+ * at any time.
+ */
+public final class DefaultGrpcExporter<T extends Marshaler> implements GrpcExporter<T> {


Open to any naming suggestions. On the bright side, the actual exporters don't need to worry about the two types, they just use GrpcExporter.builder().

anuraaga · 2021-10-05T07:46:03Z

...p/logs/src/testOkHttpOnly/java/io/opentelemetry/exporter/otlp/logs/OkHttpOnlyExportTest.java

+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+class OkHttpOnlyExportTest {


Future work is to remove duplication among the exporter test sets. A lot of the new lines in this PR are this copy-pasted export test, it'll be cool to stop doing this

jkwatson

I think this is fine. A ton of changes here, but I'm assuming it's almost entirely pure refactoring. Given the size of the change, do we want to hold off until the next release to merge this, or are we confident enough in our tests & integration tests that we can release this right away in 1.7.0?

anuraaga · 2021-10-05T16:17:26Z

Personally I'm confident - the only new code for existing users is the calculation of USE_OKHTTP which I think we can verify code-wise and we have thorough tests for here. After that it's all the same code as now, just through the delegate. So I don't think we can expect regressions - the new code path requires intent and significant POM changes to get into. It's also well tested, but in the off chance of a bug it wouldn't be a regression.

jkwatson · 2021-10-05T17:11:45Z

this needs a rebase, then

jack-berg · 2021-10-05T22:30:04Z

...p/common/src/main/java/io/opentelemetry/exporter/otlp/internal/grpc/DefaultGrpcExporter.java

+      long timeoutNanos,
+      boolean compressionEnabled) {
+    this.type = type;
+    Meter meter = GlobalMeterProvider.get().get("io.opentelemetry.exporters.otlp-grpc");


Do we want to call out in the changelog that the name of these metrics are changing? The instrumentation library will change, and also the metric name will change from io.opentelemetry.exporters.otlp to io.opentelemetry.exporters.otlp-grpc.

That's a good call, although this shouldn't change the name of any metrics (unless you have a custom exporter that appends the meter name to the metric name, I guess?). I would be slightly surprised if anyone was actually using these metrics yet, but in case they are, it is definitely reasonable to call it out.

I renamed the meter names too, instead of having the type in the meter name it's an attribute now. The camelcase before also didn't seem conventional. I suspect calling out in release notes while making the change is good here - the instrumentation library change isn't that important so I could revert it, but given the metric name change anyways, I figured it's good to more easily differentiate metrics from -grpc, -grpc-okhttp, -http

jack-berg · 2021-10-05T22:33:55Z

...otlp/common/src/main/java/io/opentelemetry/exporter/otlp/internal/grpc/GrpcExporterUtil.java

+
+  static <T extends Marshaler> GrpcExporterBuilder<T> exporterBuilder(
+      String type,
+      long defaultTimeoutSecs,


#nit: Default timeout is always the same, no? Could omit this argument.

Yeah I don't think we can lose the reference in the javadoc though for users in the public API like

https://github.com/open-telemetry/opentelemetry-java/blob/main/exporters/otlp/trace/src/main/java/io/opentelemetry/exporter/otlp/trace/OtlpGrpcSpanExporterBuilder.java#L52

Could move the constant to a shared place and reference it, but this seemed more straightforward.

jack-berg · 2021-10-05T22:35:39Z

...lp/common/src/main/java/io/opentelemetry/exporter/otlp/internal/grpc/OkHttpGrpcExporter.java

+                String errorMessage = grpcMessage(response);
+                logger.log(
+                    Level.WARNING,
+                    "Failed to export spans. Server responded with "


Oops will want to change this log message to include the configured type.

jack-berg · 2021-10-05T22:38:37Z

.../otlp/logs/src/main/java/io/opentelemetry/exporter/otlp/logs/OtlpGrpcLogExporterBuilder.java

@@ -100,9 +92,7 @@ public OtlpGrpcLogExporterBuilder setCompression(String compressionMethod) {
    checkArgument(
        compressionMethod.equals("gzip") || compressionMethod.equals("none"),
        "Unsupported compression method. Supported compression methods include: gzip, none.");


Isn't all the validation in here unnecessary now? Duplicates logic in the delegate.

Same feedback applies to the other builders.

Ah yeah did want to confirm what people think on this. There is a school of thought that validation mentions are more user friendly being as close to the user code as possible, even if it means some duplicated code (prioritize UX over duplication). I follow this school :) If it's ok I'd like to remove from the delegate and keep in the callers, what do you think?

jack-berg · 2021-10-05T22:41:20Z

.../otlp/logs/src/main/java/io/opentelemetry/exporter/otlp/logs/OtlpGrpcLogExporterBuilder.java


 /** Builder for {@link OtlpGrpcLogExporter}. */
 public final class OtlpGrpcLogExporterBuilder {

+  // Visible for testing
+  static final String GRPC_ENDPOINT_PATH =
+      "/opentelemetry.proto.collector.logs.v1.LogsService/Export";


#nit: These values appear as constants in the Marshaler{Type}ServiceGrpc classes. Would be good to move them to :exporters:otlp:common at some point.

Yeah I think I'll try that in a followup

jack-berg · 2021-10-05T22:44:54Z

Noticed a couple of minor things, but looks ok to me. Nice to add the okhttp grpc capability and simultaneously help to reduce code repetition 👍.

…nto grpc-nodeps

anuraaga · 2021-10-06T07:26:06Z

Happy to followup on anything, want to link some of these files from a doc :)

anuraaga requested review from arminru, bogdandrutu, carlosalberto and jkwatson as code owners September 30, 2021 05:28

anuraaga requested a review from a user September 30, 2021 05:28

anuraaga requested review from Oberon00, pavolloffay, thisthat and tylerbenson as code owners September 30, 2021 05:28

anuraaga commented Sep 30, 2021

View reviewed changes

anuraaga marked this pull request as draft September 30, 2021 05:29

anuraaga force-pushed the grpc-nodeps branch from f8cf9bf to abeefbc Compare September 30, 2021 05:33

Allow OTLP HTTP exporter to also export in gRPC format.

08fda86

anuraaga force-pushed the grpc-nodeps branch from abeefbc to 08fda86 Compare September 30, 2021 05:55

anuraaga commented Sep 30, 2021

View reviewed changes

anuraaga mentioned this pull request Sep 30, 2021

Is it safe to exclude the okhttp dependency? open-telemetry/opentelemetry-java-instrumentation#4240

Closed

jack-berg reviewed Sep 30, 2021

View reviewed changes

detect classpath

7929de0

Anuraag Agrawal added 4 commits October 5, 2021 12:01

Revert HTTP exporter

5f86d40

Apply to metrics / trace

1da3689

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java i…

c251c73

…nto grpc-nodeps

Most

e442433

Fix

858235b

anuraaga changed the title ~~Allow OTLP HTTP exporter to also export in gRPC format.~~ Allow OTLP gRPC exporters to work without grpc-java using okhttp directly. Oct 5, 2021

Integration tests

6042f1a

anuraaga marked this pull request as ready for review October 5, 2021 07:42

anuraaga commented Oct 5, 2021

View reviewed changes

Clean

3640eb0

jkwatson approved these changes Oct 5, 2021

View reviewed changes

jack-berg reviewed Oct 5, 2021

View reviewed changes

Anuraag Agrawal added 4 commits October 6, 2021 08:20

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java i…

9fe3a04

…nto grpc-nodeps

Fix log message

22723d5

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java i…

25a8c5e

…nto grpc-nodeps

Assume validated

97a91b9

anuraaga force-pushed the grpc-nodeps branch from eadb3b9 to 97a91b9 Compare October 6, 2021 03:45

anuraaga merged commit 63bec07 into open-telemetry:main Oct 6, 2021

trask mentioned this pull request Oct 18, 2021

Remove gRPC from javaagent open-telemetry/opentelemetry-java-instrumentation#4415

Closed

This was referenced Dec 19, 2021

Temurin JDK #4011

Merged

use Eclipse Temurin JDK docker image #4012

Merged

geoand mentioned this pull request Jul 7, 2023

OOM in Quarkus 3.0.0.Beta1 caused by okio via OpenTelemetry quarkusio/quarkus#32238

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow OTLP gRPC exporters to work without grpc-java using okhttp directly. #3684

Allow OTLP gRPC exporters to work without grpc-java using okhttp directly. #3684

anuraaga commented Sep 30, 2021 •

edited

Loading

anuraaga Sep 30, 2021

anuraaga commented Sep 30, 2021

anuraaga Sep 30, 2021

jack-berg Sep 30, 2021

jack-berg commented Sep 30, 2021

jack-berg Sep 30, 2021

anuraaga commented Oct 1, 2021 •

edited

Loading

anuraaga commented Oct 1, 2021

anuraaga commented Oct 4, 2021 •

edited

Loading

jkwatson commented Oct 4, 2021

jack-berg commented Oct 4, 2021

anuraaga commented Oct 5, 2021

anuraaga Oct 5, 2021

anuraaga Oct 5, 2021

jkwatson left a comment

anuraaga commented Oct 5, 2021

jkwatson commented Oct 5, 2021

jack-berg Oct 5, 2021

jkwatson Oct 5, 2021

anuraaga Oct 6, 2021

jack-berg Oct 5, 2021

anuraaga Oct 5, 2021

jack-berg Oct 5, 2021

jack-berg Oct 5, 2021

jack-berg Oct 5, 2021

anuraaga Oct 5, 2021

jack-berg Oct 5, 2021

anuraaga Oct 6, 2021

jack-berg commented Oct 5, 2021

anuraaga commented Oct 6, 2021

		@@ -127,9 +129,28 @@ public OtlpHttpSpanExporter build() {
		}
		}

Allow OTLP gRPC exporters to work without grpc-java using okhttp directly. #3684

Allow OTLP gRPC exporters to work without grpc-java using okhttp directly. #3684

Conversation

anuraaga commented Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

anuraaga commented Sep 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jack-berg commented Sep 30, 2021

Choose a reason for hiding this comment

anuraaga commented Oct 1, 2021 • edited Loading

anuraaga commented Oct 1, 2021

anuraaga commented Oct 4, 2021 • edited Loading

jkwatson commented Oct 4, 2021

jack-berg commented Oct 4, 2021

anuraaga commented Oct 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkwatson left a comment

Choose a reason for hiding this comment

anuraaga commented Oct 5, 2021

jkwatson commented Oct 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jack-berg commented Oct 5, 2021

anuraaga commented Oct 6, 2021

anuraaga commented Sep 30, 2021 •

edited

Loading

anuraaga commented Oct 1, 2021 •

edited

Loading

anuraaga commented Oct 4, 2021 •

edited

Loading