OTLP HTTP trace exporter #3418

jack-berg · 2021-07-21T21:27:10Z

OTLP HTTP trace exporter implementation.

jack-berg · 2021-07-21T21:27:22Z

I added the OtlpHttpSpanExporter to the :exporters:otlp:trace to defer thinking about where to put it, but it will have to be addressed.

Keeping it in :exporter:otlp:trace avoids extra modules / artifacts. It also removes the need to rename or think about renaming existing modules / packages / artifacts to disambiguate between otlp/grpc and otlp/http. The current naming conventions imply that otlp = otlp/grpc. We can keep that, and have new modules / packages / artifacts use otlp-http to disambiguate, but this asymmetry isn't ideal. Keeping the exporter in :expoter:otlp:trace sidesteps this whole conversation. This doesn't feel wrong to me, since otlp/grpc and otlp/http with JSON format and Protobuf format are all different flavors of otlp.

Another thing to think is whether to shade the new dependencies on okhttp, and whether it matters based on where the exporter lives.

...on/src/main/java/io/opentelemetry/exporter/otlp/internal/HexEncodingStringJsonGenerator.java

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

...lp/trace/src/main/java/io/opentelemetry/exporter/otlp/trace/OtlpHttpSpanExporterBuilder.java

exporters/otlp/common/build.gradle.kts

jkwatson · 2021-07-21T23:34:51Z

I think it would be unfortunate if otlp grpc users suddenly got an okhttp dependency thrust upon them, forcing them to exclude it to keep their artifact smaller (and avoid version clashes). so, if we want to put them in the same module, we should definitely shade-in okhttp.

anuraaga · 2021-07-23T01:57:43Z

Let's use a different module - I expect a very small minority of users to need this module so want to reduce it's discoveribility vs our "standard recommendation" grpc.

anuraaga · 2021-07-23T01:59:25Z

exporters/otlp/trace/build.gradle.kts

@@ -28,6 +28,10 @@ dependencies {
    implementation("io.grpc:grpc-stub")
    implementation("com.google.protobuf:protobuf-java")

+    implementation("com.squareup.okhttp3:okhttp:4.9.0")


Let's target okhttp3 to not pull in kotlin stdlib. This will be especially important if we shade (let's not worry about it in the first PR)

Any thoughts on publishing both shaded and not shaded (the one use case made easy by Gradle shadow plugin : 😹)?

Ok! I'm inclined to keep the surface area smaller and choosing to either shade or not shade. Can always publish additional artifacts later, but it's harder to stop publishing.

On second thought, maybe okhttp3 isn't such a great idea. Its only getting critical fixes through December 31st, 2021. And since we're going with the separate module route, it doesn't seem as critical that the kotlin stdlib is included.

3.12.x isn't getting updates, but the 4.x will be.

Ah I didn't know about the EoL then seems ok (indeed especially with the separate module)

...lp/trace/src/main/java/io/opentelemetry/exporter/otlp/trace/OtlpHttpSpanExporterBuilder.java

anuraaga · 2021-07-23T02:08:15Z

exporters/otlp/trace/src/main/java/io/opentelemetry/exporter/otlp/trace/OtlpHttpUtil.java

+    @Override
+    public RequestBody build(ExportTraceServiceRequest exportTraceServiceRequest) {
+      return RequestBody.create(
+          exportTraceServiceRequest.toByteArray(), MediaType.parse("application/x-protobuf"));


It can and probably should be a future PR but would definitely be good to use okio buffer and codedinputstream to skip the intermediate unpooled buffer

jack-berg · 2021-07-23T21:36:09Z

Let's use a different module - I expect a very small minority of users to need this module so want to reduce it's discoveribility vs our "standard recommendation" grpc.

@anuraaga thanks for the great feedback! Very helpful. For the separate modules, how about:
:exporters:otlp:trace-http and :exporters:otlp:metrics-http (metrics to be added in a future PR)?

codecov · 2021-07-23T23:53:18Z

Codecov Report

Merging #3418 (6ea11f4) into main (614668a) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #3418      +/-   ##
============================================
- Coverage     90.84%   90.83%   -0.02%     
  Complexity     3228     3228              
============================================
  Files           368      368              
  Lines          9941     9941              
  Branches       1003     1003              
============================================
- Hits           9031     9030       -1     
  Misses          598      598              
- Partials        312      313       +1

Impacted Files	Coverage Δ
...telemetry/sdk/trace/export/BatchSpanProcessor.java	`88.18% <0.00%> (-0.79%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60c3c5c...6ea11f4. Read the comment docs.

...g-otlp/src/main/java/io/opentelemetry/exporter/logging/otlp/OtlpJsonLoggingSpanExporter.java

exporters/otlp-http/trace/README.md

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

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

.../trace/src/test/java/io/opentelemetry/exporter/otlp/http/trace/OtlpHttpSpanExporterTest.java

jack-berg · 2021-07-25T18:46:03Z

.../trace/src/test/java/io/opentelemetry/exporter/otlp/http/trace/OtlpHttpSpanExporterTest.java

+
+  private static final MediaType APPLICATION_PROTOBUF =
+      MediaType.create("application", "x-protobuf");
+  private static final HeldCertificate HELD_CERTIFICATE;


Note: I've used the the certificate utility from okhttp-tls instead of the SelfSignedCertificateExtension seen in other tests. I did this because the certificate from SelfSignedCertificateExtension doesn't include a subject alternative name and the okhttp client fails TLS validation without this field.

Ah yeah I noticed this too separately - quite bizarre okhttp restriction

anuraaga

Thanks a lot, almost there

anuraaga · 2021-07-26T02:43:09Z

.../trace/src/test/java/io/opentelemetry/exporter/otlp/http/trace/OtlpHttpSpanExporterTest.java

+
+  private static final MediaType APPLICATION_PROTOBUF =
+      MediaType.create("application", "x-protobuf");
+  private static final HeldCertificate HELD_CERTIFICATE;


Ah yeah I noticed this too separately - quite bizarre okhttp restriction

exporters/otlp-http/trace/build.gradle.kts

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

.../trace/src/test/java/io/opentelemetry/exporter/otlp/http/trace/OtlpHttpSpanExporterTest.java

anuraaga

Thanks, just found one small point left. @jkwatson is on vacation but let's give him a chance to review before merging.

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

…y-java into otlp-http-exporters

jkwatson · 2021-08-03T19:58:13Z

Since this is a new module that isn't marked alpha, shouldn't there be apidiffs for it?

jack-berg · 2021-08-03T20:08:41Z

@jkwatson actually could use a pointer on that. There's no apidiffs because I've commented out the id("otel.publish-conventions") in build.gradle.kts. And I've commented it out because I get the following error trying to run :build if its enabled:

Could not determine the dependencies of task ':exporters:otlp-http:trace:check'.
> Could not create task ':exporters:otlp-http:trace:jApiCmp'.
   > Could not resolve all files for configuration ':exporters:otlp-http:trace:detachedConfiguration1'.
      > Could not find io.opentelemetry:opentelemetry-exporter-otlp-http-trace:1.4.1.
        Searched in the following locations:
          - https://repo.maven.apache.org/maven2/io/opentelemetry/opentelemetry-exporter-otlp-http-trace/1.4.1/opentelemetry-exporter-otlp-http-trace-1.4.1.pom

Seems like a chicken and the egg issue: can't add publish conventions until there's an artifact out there that can be used as a comparison point for the apidiffs.

I know the apidiff stuff is fairly new - is there a playbook on how to proceed adding a new module?

jkwatson · 2021-08-03T20:18:13Z

I know the apidiff stuff is fairly new - is there a playbook on how to proceed adding a new module?

I thought @anuraaga had fixed this issue so that it would generate a new file for a new module.

Curious why you commented out publishing, though... shouldn't this be published?

jack-berg · 2021-08-03T20:35:45Z

It should be published. I commented out publishing because the build fails with this error when publishing is enabled.

Let me see if I can't dig into it a bit more and get to the bottom of why new modules are causing this issue.

jkwatson · 2021-08-03T21:47:01Z

It should be published. I commented out publishing because the build fails with this error when publishing is enabled.

Let me see if I can't dig into it a bit more and get to the bottom of why new modules are causing this issue.

hmm. interesting. If you can't sort this out, I can try to take a look (or perhaps @anuraaga will know the problem right away).

jack-berg · 2021-08-03T23:01:40Z

Looking closer at the otel.japicmp-conventions.gradle.kts config, I don't think this is actually a solved problem:

The jApiCmp task is added if the project does not have otel.release and does not start with bom. Because otel.release is reserved for alpha projects and this new project will not be alpha, it will get the jApiCmp task.
If we have to have this task, then we need to configure it to compare the latest jar to nothing, since no previous version exists. I can't find a way to configure jApiCmp to do a "compare to nothing". The oldClasspath property is what specifies what your comparing the new jar against, and its required.
The jApiCmp code was initially added on May 6th, 2021 in PR Add a task to compare public APIs for every stable module. #3183. If you look at the blame history of settings.gradle.kts, only one new project has been added since May 6th, :sdk:metrics-testing, and its exempt from the jApiCmp task because its marked as otel.release=alpha.

One way we could solve this is by adding another property to new projects that haven't been published yet, such as otel.initial-release. Then configure jApiCmp task to skip projects with this flag, and remove the flag after the first publication.

jkwatson · 2021-08-03T23:17:17Z

One way we could solve this is by adding another property to new projects that haven't been published yet, such as otel.initial-release. Then configure jApiCmp task to skip projects with this flag, and remove the flag after the first publication.

Something like this seems totally reasonable to me. Or, we institute a policy where every new module has to go through at least one alpha release before it is made non-alpha, which would also solve the problem. What do you think about that, @anuraaga ?

anuraaga · 2021-08-04T06:07:22Z

Do we need a new flag? I guess we can disable the task if oldClasspath is invalid, so that'd be pretty easy. If it's not obvious how to do it I can look into that. Let's merge this first though.

jkwatson

Thanks!

sumitm-iiit · 2021-09-07T13:36:44Z

Can someone please specify this change is part of which open telemetry java agent? I have tried with Open Telemetry Java Agent 1.5.3 but it seems this change is not present.

Oberon00 · 2021-09-07T13:43:06Z

I think you should ask that in https://github.com/open-telemetry/opentelemetry-java-instrumentation. As displayed by GitHub, this change is included in version 1.5.0 of "core" opentelemetry-java.

sumitm-iiit · 2021-09-07T13:51:02Z

@Oberon00 : Thank you for your quick response. Sure I will ask this question in htts://github.com/open-telemetry/opentelemetry-java-instrumentation..

Oberon00 · 2021-09-07T14:00:00Z

@sumitm-iiit BTW, under https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk-extensions/autoconfigure/README.md#otlp-exporter-both-span-and-metric-exporters there is otel.experimental.exporter.otlp.traces.protocol, maybe that is what you are looking for?

sumitm-iiit · 2021-09-07T14:19:22Z

I have tried that but it seems the agent itself does not packages OTLP over HTTP exporter inside it.

…

On Tue, Sep 7, 2021, 7:30 PM Christian Neumüller ***@***.***> wrote: @sumitm-iiit <https://github.com/sumitm-iiit> BTW, under https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk-extensions/autoconfigure/README.md#otlp-exporter-both-span-and-metric-exporters there is otel.experimental.exporter.otlp.metrics.protocol, maybe that is what you are looking for? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3418 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AF4BIYAZENX657SYIFDBPHTUAYLG3ANCNFSM5AYYSW4A> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

Draft of OTLP HTTP trace exporter

a172dcc