Add JSON export option #4435

jonahaapala · 2022-05-04T20:35:34Z

I am working on adding JSON support for the New Relic OTLP Metrics consumer and while writing functional tests (written in Java) to exercise the new protocol I noticed that there wasn't an OOTB way to configure the OkHttpExporter to send payloads as JSON. For now I've copied the classes I need and changed them like I have in this PR, but figured it would be a nice addition to the opentelemetry-java project as it's not too much code and makes testing more convenient.

I recognize that clients will want to configure themselves to send things as Protobuf, but it might be nice to have the JSON option if only for testing. I wanted to just add the option to the OkHttpExporter (which is what we use directly in our tests) so that it wouldn't need to be exposed in the OtlpHttpMetricExporterBuilder, but the existing tests seem to exercise the OkHttpExporter via the OtlpHttpMetricExporterBuilder so I added the extra wiring to keep testing similar.

linux-foundation-easycla · 2022-05-04T20:35:37Z

The committers listed above are authorized under a signed CLA.

✅ login: jonahaapala / name: Jon (20733dc, c76dbaf, 49c528b, 0d2b580)

docs/apidiffs/current_vs_latest/opentelemetry-sdk-metrics.txt

jonahaapala · 2022-05-04T20:36:16Z

...ers/otlp/common/src/main/java/io/opentelemetry/exporter/internal/okhttp/JsonRequestBody.java

+import okhttp3.RequestBody;
+import okio.BufferedSink;
+
+public class JsonRequestBody extends RequestBody {


Pretty much a copy paste of ProtoResponseBody

I believe this can be package private, and final. (ProtoResponseBody can be package private as well.)

codecov · 2022-05-04T20:46:27Z

Codecov Report

Merging #4435 (3b77cf7) into main (6606020) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #4435      +/-   ##
============================================
- Coverage     90.02%   90.02%   -0.01%     
- Complexity     4970     4979       +9     
============================================
  Files           570      571       +1     
  Lines         15384    15401      +17     
  Branches       1472     1473       +1     
============================================
+ Hits          13850    13864      +14     
- Misses         1064     1067       +3     
  Partials        470      470

Impacted Files	Coverage Δ
...try/exporter/internal/okhttp/ProtoRequestBody.java	`100.00% <ø> (ø)`
...lp/http/metrics/OtlpHttpMetricExporterBuilder.java	`100.00% <100.00%> (ø)`
...etry/exporter/internal/okhttp/JsonRequestBody.java	`100.00% <100.00%> (ø)`
...metry/exporter/internal/okhttp/OkHttpExporter.java	`95.23% <100.00%> (+0.07%)`	⬆️
...xporter/internal/okhttp/OkHttpExporterBuilder.java	`90.00% <100.00%> (+0.63%)`	⬆️
...entelemetry/exporter/jaeger/PostSpansResponse.java	`0.00% <0.00%> (-100.00%)`	⬇️
...exporter/jaeger/MarshalerCollectorServiceGrpc.java	`85.71% <0.00%> (-4.77%)`	⬇️
...ava/io/opentelemetry/micrometer1shim/Bridging.java	`100.00% <0.00%> (ø)`

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 6606020...3b77cf7. Read the comment docs.

jonahaapala · 2022-05-04T20:57:14Z

I have signed the CLA, not sure when the PR will update with that fact though.

jonahaapala · 2022-05-04T23:11:16Z

...ers/otlp/common/src/main/java/io/opentelemetry/exporter/internal/okhttp/JsonRequestBody.java

+
+  /** Creates a new {@link JsonRequestBody}. */
+  public JsonRequestBody(Marshaler marshaler) {
+    preserializedJson = preserializeJson(marshaler);


Initially I wanted to save off the marshaler like ProtoResponseBody does and substitute writeBinaryTo() with writeJsonTo() in the overridden writeTo() method below. However there wasn't an equivalent method for getting the contentLength like ProtoResponseBody does in its constructor, so this seemed like the simplest way to do this with the existing code at my disposal.

I copied the implementation from MarshalerUtil.preserializeJsonFields() but omit the part that trims the beginning and ending brackets.

How about just sticking with -1 for contentLength without preserializing?

Interesting. I initially didn't think we could leave the contentLength unset because we do some validation on our end to see that it is a positive value. But I don't see anything in the spec that indicates the Content-Length header must be set and I don't see anything in our code that relies on it other than for monitoring. Asking my team for any context around this choice, but so far I'm inclined to do as you suggest and return -1.

Though, might I ask, is there a downside to leaving this as is? I think it's just a matter of when this serialization logic gets called. It's a bit wasteful to do it this way as there is an extra copy cost to transfer the bytes into the final buffer, but I don't think by very much.

It's not a big deal either way if only for tests - but OkHttp's intended usage is to "do work" on its I/O thread, so it's a bit more correct to do serialization within writeTo, whereas preserialization in the constructor is serializing on the hot path in principle (not if it's a test, but otherwise it would be).

jonahaapala · 2022-05-04T23:16:18Z

Using these changes locally I was able to verify that it exports as proper JSON that can be parsed by our consumers.

jonahaapala · 2022-05-06T01:47:53Z

/easycla

jonahaapala · 2022-05-06T02:00:05Z

\easycla

jonahaapala · 2022-05-06T02:00:47Z

easycla

jonahaapala · 2022-05-06T17:54:31Z

...src/main/java/io/opentelemetry/exporter/otlp/http/metrics/OtlpHttpMetricExporterBuilder.java

@@ -117,6 +117,11 @@ public OtlpHttpMetricExporterBuilder setAggregationTemporality(
    return this;
  }

+  OtlpHttpMetricExporterBuilder exportAsJson() {


not exposing as a public API. Keeping the one on OkHttpExporterBuilder public though for testing purposes.

anuraaga · 2022-05-09T02:31:27Z

exporters/otlp/common/src/main/java/io/opentelemetry/exporter/internal/ExporterMetrics.java

@@ -70,4 +70,9 @@ public static ExporterMetrics createGrpcOkHttp(String type, MeterProvider meterP
  public static ExporterMetrics createHttpProtobuf(String type, MeterProvider meterProvider) {
    return new ExporterMetrics(meterProvider.get("io.opentelemetry.exporters.otlp-http"), type);
  }
+
+  public static ExporterMetrics createHttpJson(String type, MeterProvider meterProvider) {


Since this is only really for testing anyways, we probably don't need a separate metrics for it

anuraaga · 2022-05-09T02:32:56Z

...ers/otlp/common/src/main/java/io/opentelemetry/exporter/internal/okhttp/JsonRequestBody.java

+
+  /** Creates a new {@link JsonRequestBody}. */
+  public JsonRequestBody(Marshaler marshaler) {
+    preserializedJson = preserializeJson(marshaler);


How about just sticking with -1 for contentLength without preserializing?

…ontentLength instead of preserializing.

jonahaapala · 2022-05-09T22:53:01Z

README.md

+| Logs SDK                    | v<!--VERSION_UNSTABLE-->1.15.0-SNAPSHOT-alpha<!--/VERSION_UNSTABLE--> |
+| Prometheus Metrics Exporter | v<!--VERSION_UNSTABLE-->1.15.0-SNAPSHOT-alpha<!--/VERSION_UNSTABLE--> |
+| OpenTracing Bridge          | v<!--VERSION_UNSTABLE-->1.15.0-SNAPSHOT-alpha<!--/VERSION_UNSTABLE--> |
+| OpenCensus Bridge           | v<!--VERSION_UNSTABLE-->1.15.0-SNAPSHOT-alpha<!--/VERSION_UNSTABLE--> |


yikes, idk what's going on with this and all of the apidiffs updates. This is what the ./gradlew check did...

Sorry about that @jonahaapala, you got caught in a race condition with our release. I pushed a commit to this branch to pull in HEAD to fix it

…nto jhaapala/add-json-export-option

jonahaapala · 2022-05-10T16:17:34Z

@anuraaga thanks for the review! What are the next steps now that you've approved? I don't have the ability to merge this. Are we waiting on more approvers?

jack-berg

Just the minor comment about package privacy.

jonahaapala requested review from anuraaga, jack-berg, jkwatson and jsuereth as code owners May 4, 2022 20:35

jonahaapala requested a review from a user May 4, 2022 20:35

jonahaapala requested a review from Oberon00 as a code owner May 4, 2022 20:35

jonahaapala commented May 4, 2022

View reviewed changes

docs/apidiffs/current_vs_latest/opentelemetry-sdk-metrics.txt Outdated Show resolved Hide resolved

jonahaapala commented May 4, 2022

View reviewed changes

jonahaapala closed this May 4, 2022

jonahaapala reopened this May 4, 2022

jonahaapala force-pushed the jhaapala/add-json-export-option branch from 4d66f31 to 0d2b580 Compare May 6, 2022 16:53

jonahaapala commented May 6, 2022

View reviewed changes

anuraaga reviewed May 9, 2022

View reviewed changes

jonahaapala added 6 commits May 9, 2022 15:07

Add OkHttpExporterBuilder option to serialize proto as json

c0b32c8

Format code to spotless standards and add tests

6612fc1

Include begin and end brackets in json

4fb8f14

Remove exportAsJson from public api

4f68acf

Remove special exporter metrics for json export. Return -1 for json c…

458efcf

…ontentLength instead of preserializing.

Rebase

4fdbc5c

jonahaapala force-pushed the jhaapala/add-json-export-option branch from 0d2b580 to 4fdbc5c Compare May 9, 2022 22:49

jonahaapala commented May 9, 2022

View reviewed changes

anuraaga mentioned this pull request May 10, 2022

Post release 1.14.0 #4453

Merged

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

56e83e8

…nto jhaapala/add-json-export-option

anuraaga approved these changes May 10, 2022

View reviewed changes

jack-berg approved these changes May 10, 2022

View reviewed changes

Make Json/ProtoRequestBody classes final and package private

3b77cf7

jack-berg merged commit 20bd019 into open-telemetry:main May 11, 2022

jonahaapala deleted the jhaapala/add-json-export-option branch May 17, 2022 17:51

jack-berg mentioned this pull request May 17, 2022

Add an OTLP http/json exporter #3651

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add JSON export option #4435

Add JSON export option #4435

jonahaapala commented May 4, 2022

linux-foundation-easycla bot commented May 4, 2022 •

edited

jonahaapala May 4, 2022

jack-berg May 10, 2022

codecov bot commented May 4, 2022 •

edited

jonahaapala commented May 4, 2022

jonahaapala May 4, 2022

anuraaga May 9, 2022

jonahaapala May 9, 2022

jonahaapala May 9, 2022

anuraaga May 10, 2022

jonahaapala commented May 4, 2022

jonahaapala commented May 6, 2022

jonahaapala commented May 6, 2022

jonahaapala commented May 6, 2022

jonahaapala May 6, 2022 •

edited

anuraaga May 9, 2022

anuraaga May 9, 2022

jonahaapala May 9, 2022

anuraaga May 10, 2022

jonahaapala commented May 10, 2022

jack-berg left a comment

Add JSON export option #4435

Add JSON export option #4435

Conversation

jonahaapala commented May 4, 2022

linux-foundation-easycla bot commented May 4, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 4, 2022 • edited

Codecov Report

jonahaapala commented May 4, 2022

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

jonahaapala commented May 4, 2022

jonahaapala commented May 6, 2022

jonahaapala commented May 6, 2022

jonahaapala commented May 6, 2022

jonahaapala May 6, 2022 • edited

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

jonahaapala commented May 10, 2022

jack-berg left a comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented May 4, 2022 •

edited

codecov bot commented May 4, 2022 •

edited

jonahaapala May 6, 2022 •

edited