Add a OTLP JSON span exporter to logs. #2295

anuraaga · 2020-12-14T09:08:34Z

This uses a protobuf-json library I wrote a while ago, and we use it in Armeria for production. It's based on Jackson vs gson, which I think has a higher chance of being on an app's classpath which is nice. The use of Jackson also made it quite easy to switch the binary marshalling to hex from base64. But it's designed for speed (for use in production) so uses bytebuddy for runtime code generation as opposed to runtime reflection like the official json-format does. That does unfortunately make it more heavyweight than it could be, but at least it's fast 😅.

Wondering where to put this, probably not in -otlp, new -otlp-logging, or replace the current -logging? Leaning towards -otlp-logging.

anuraaga · 2020-12-14T09:19:00Z

For reference, in official, the base64 encoding is hard coded inside this private method, so no go

https://github.com/protocolbuffers/protobuf/blob/master/java/util/src/main/java/com/google/protobuf/util/JsonFormat.java#L1235

jkwatson · 2020-12-14T19:24:56Z

how "heavyweight" does it end up being? My only concern with replacing the existing logging exporter is how much users would be taking on to get it. -otlp-logging seems good to me, otherwise. We can always decide to replace later, if there's appetite for it.

anuraaga · 2020-12-15T04:44:16Z

@jkwatson I think ~4MB for byte-buddy + jackson. That's after I do some pruning since it's currently getting ~3MB of guava too due to a dependency on protobuf-java-util for a very small amount of logic, as it stands it's about 7MB. Let me go ahead and make a new module for it at least for now.

trask · 2020-12-15T04:59:16Z

are we going to break out otlp-metrics and otlp-spans modules due to versioning? maybe this should be logging-otlp, it seems more a variant of logging to me than a variant of otlp (not sure if I'm making sense)

anuraaga · 2020-12-15T05:01:29Z

Ah forgot to mention one more caveat - we'll need an otlp-common to contain the proto translation code. How's that sound?

are we going to break out otlp-metrics and otlp-spans modules due to versioning?

Ah yeah this was the plan

maybe this should be logging-otlp, it seems more a variant of logging to me than a variant of otlp (not sure if I'm making sense)

Makes sense to me

anuraaga · 2020-12-15T05:06:03Z

Hmm - well I guess the README is saying it's ok for me to move the code into this extension?

https://github.com/open-telemetry/opentelemetry-java/tree/master/sdk-extensions/otproto

bogdandrutu · 2020-12-15T05:25:40Z

@anuraaga haven’t read the code, but why did you mention JSON encoding in the other PR? Can you add JSON format? We should prioritize proto

anuraaga · 2020-12-15T05:35:14Z

@bogdandrutu This does use JSON serialization of the proto. Meaning it relies on e.g., SpanAdapter to set up the proto, classes that technically aren't needed if adding #2304

bogdandrutu · 2020-12-15T17:45:21Z

I read the PR and I think the tittle is confusing, this is not an exporter for OTLP logs, but an exporter that writes to the logs. Do we need this?

anuraaga · 2020-12-16T02:00:23Z

@bogdandrutu There was interest in a fully spec'd, stable format for spans when writing to logs in #2293. It seems nice, but could be overkill. @jkwatson @trask any thoughts? I've heard Splunk wanting to use logs for exporting spans in AWS Lambda, maybe @bogdandrutu @iNikem you know if this could help with that?

trask · 2020-12-16T03:11:53Z

We've seen interest in using the logging exporter beyond getting started / toy apps (e.g. open-telemetry/opentelemetry-java-instrumentation#1600).

So it would be great if the output of the logging exporter can have some sort of stability guarantee, similar to the other stability guarantees we have been discussing.

OTLP/JSON[*] seems ideal, only because we get it for "free" and wouldn't have to invent something new.

[*]without the weird traceid/spanid format

… into otlp-logging

anuraaga · 2020-12-21T04:38:53Z

exporters/otlpjson/build.gradle

+ext.moduleName = "io.opentelemetry.exporter.otlpjson"
+
+dependencies {
+    compileOnly project(':sdk:trace')


@jkwatson So I tried a different approach to splitting all exporters for trace vs metrics here since it seemed like it could be a bit too much. If going with this, I'd revert the change to split out the otlp exporters in favor of compileOnly dependencies.

One of the reasons it felt like much is we would need to split otproto up as well - they all end up having a common, trace, metrics split. Lots of artifacts.

But it means that previously, a user could only depend on exporter-otlp to have the SDK as well, but with such a scheme, they'd have to additionally add the SDK as a dependency. I wonder if this is too annoying or reasonable.

So, since the trace SDK is supposed to be stable, I think it would ok to declare this as an api dependency and get that for free, like people would probably expect. Keeping the metrics dependency as provided might be ok, though, since someone has to explicitly opt-in to depending on it.

Do you think so long term? e.g., what if someone replaces the trace SDK with a custom one, would we want the exporter artifacts to still bring in the trace SDK? I guess it could be ok, as custom SDKs still seem rare enough to me. Just want to confirm.

If it's a completely custom SDK, it might have a completely different exporter interface, yeah? This does make me think we should have a separate artifact with the "SDK Extension Points" in it, including exporter, span processor, sampler interfaces, so at least custom SDKs can use the same extension points.

jkwatson · 2020-12-21T16:05:11Z

exporters/otlpjson/README.md

@@ -0,0 +1,9 @@
+# OpenTelemetry - OTLP JSON Exporters


If this is purely a logging exporter, this module should be named logging-otlp I think.

Yeah - there's some tension with the logging signal. But went with that name.

heh...yeah, I had forgotten about that bit.

oh no, I hadn't thought about that either, that's going to be very confusing, should we rename our existing logging exporter to "file" exporter, or "console" exporter, or ...

jkwatson · 2020-12-21T16:10:09Z

...json/src/test/java/io/opentelemetry/exporter/otlpjson/OtlpJsonLoggingMetricExporterTest.java

+              + "    }]\n"
+              + "  }]\n"
+              + "}",
+          logged.get(0).getMessage(),


If I read this right...the log message will be split across multiple lines? Don't we want to make sure that every log message is only a single line, so it can work with normal logging systems?

Thanks for noticing! This is a JSON assert so whitespace is ignored. I added additional checks that the message has no newlines and realized I needed to set that.

jkwatson · 2020-12-21T16:10:36Z

...lpjson/src/test/java/io/opentelemetry/exporter/otlpjson/OtlpJsonLoggingSpanExporterTest.java

+              + "      }\n"
+              + "    }]\n"
+              + "  }]\n"
+              + "}",


same question about multi-line log message

jkwatson · 2020-12-21T16:11:27Z

exporters/otlpjson/build.gradle

+ext.moduleName = "io.opentelemetry.exporter.otlpjson"
+
+dependencies {
+    compileOnly project(':sdk:trace')


Would "provided" be a semantically more correct dependency declaration?

I think provided is "expected to be there", e.g., tomcat, and optional is "add if you want". compileOnly roughly corresponds with optional.

jkwatson · 2020-12-21T16:13:11Z

exporters/otlpjson/build.gradle

+ext.moduleName = "io.opentelemetry.exporter.otlpjson"
+
+dependencies {
+    compileOnly project(':sdk:trace')


So, since the trace SDK is supposed to be stable, I think it would ok to declare this as an api dependency and get that for free, like people would probably expect. Keeping the metrics dependency as provided might be ok, though, since someone has to explicitly opt-in to depending on it.

codecov · 2020-12-22T05:37:59Z

Codecov Report

Merging #2295 (ec65fe4) into master (6cf957b) will increase coverage by 88.53%.
The diff coverage is 77.77%.

@@              Coverage Diff              @@
##             master    #2295       +/-   ##
=============================================
+ Coverage          0   88.53%   +88.53%     
- Complexity        0     2576     +2576     
=============================================
  Files             0      293      +293     
  Lines             0     8678     +8678     
  Branches          0      929      +929     
=============================================
+ Hits              0     7683     +7683     
- Misses            0      660      +660     
- Partials          0      335      +335

Impacted Files	Coverage Δ	Complexity Δ
...er/logging/otlp/OtlpJsonLoggingMetricExporter.java	`65.71% <65.71%> (ø)`	`6.00 <6.00> (?)`
...r/logging/otlp/HexEncodingStringJsonGenerator.java	`88.23% <88.23%> (ø)`	`6.00 <6.00> (?)`
...rter/logging/otlp/OtlpJsonLoggingSpanExporter.java	`90.00% <90.00%> (ø)`	`6.00 <6.00> (?)`
...a/io/opentelemetry/sdk/logging/data/LogRecord.java	`100.00% <0.00%> (ø)`	`2.00% <0.00%> (?%)`
...n/java/io/opentelemetry/sdk/trace/IdGenerator.java	`100.00% <0.00%> (ø)`	`1.00% <0.00%> (?%)`
.../io/opentelemetry/opentracingshim/Propagation.java	`96.42% <0.00%> (ø)`	`5.00% <0.00%> (?%)`
...opentelemetry/opentracingshim/OpenTracingShim.java	`100.00% <0.00%> (ø)`	`3.00% <0.00%> (?%)`
...opentelemetry/sdk/common/export/ConfigBuilder.java	`100.00% <0.00%> (ø)`	`10.00% <0.00%> (?%)`
.../java/io/opentelemetry/sdk/resources/Resource.java	`92.00% <0.00%> (ø)`	`15.00% <0.00%> (?%)`
...entelemetry/sdk/extension/otproto/SpanAdapter.java	`97.91% <0.00%> (ø)`	`25.00% <0.00%> (?%)`
... and 286 more

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 6cf957b...ec65fe4. Read the comment docs.

jkwatson · 2020-12-22T18:29:58Z

exporters/logging-otlp/build.gradle

+}
+
+description = 'OpenTelemetry Protocol JSON Logging Exporters'
+ext.moduleName = "io.opentelemetry.exporter.logging.otlp"


my only possible quibble is that maybe this should be "io.opentelemetry.exporter.logging-otlp" but I'm not quite sure which is going to be more discoverable by end-users. Maybe the description could be more .. descriptive about exactly what's going on here. Maybe description = 'OpenTelemetry logging exporters with OTLP JSON formatting. Currently supports span exports.' ?

jkwatson

Thanks! Can't wait to try it out!

Add a OTLP logging span exporter.

414a00b

anuraaga requested review from trask, Oberon00 and jkwatson December 14, 2020 09:08

Add file to git

89149ce

anuraaga mentioned this pull request Dec 15, 2020

Move OTLP adapters to otproto module for use outside of gRPC exporters. #2306

Merged

anuraaga changed the title ~~Add a OTLP logging span exporter.~~ Add a OTLP JSON span exporter to logs. Dec 16, 2020

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

bb5742e

… into otlp-logging

anuraaga mentioned this pull request Dec 17, 2020

Use normal filesystem-based Gradle paths. #2332

Merged

Anuraag Agrawal added 3 commits December 18, 2020 15:00

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

a34d0ed

… into otlp-logging

Move

b701e00

OTLPJSON

973f541

anuraaga marked this pull request as ready for review December 21, 2020 04:35

anuraaga requested review from arminru, bogdandrutu, carlosalberto, pavolloffay, thisthat and tylerbenson as code owners December 21, 2020 04:35

anuraaga commented Dec 21, 2020

View reviewed changes

jkwatson reviewed Dec 21, 2020

View reviewed changes

Cleanups

ec65fe4

jkwatson reviewed Dec 22, 2020

View reviewed changes

jkwatson approved these changes Dec 22, 2020

View reviewed changes

jkwatson merged commit 6d1bcd9 into open-telemetry:master Dec 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a OTLP JSON span exporter to logs. #2295

Add a OTLP JSON span exporter to logs. #2295

anuraaga commented Dec 14, 2020

anuraaga commented Dec 14, 2020

jkwatson commented Dec 14, 2020

anuraaga commented Dec 15, 2020

trask commented Dec 15, 2020

anuraaga commented Dec 15, 2020

anuraaga commented Dec 15, 2020 •

edited

Loading

bogdandrutu commented Dec 15, 2020

anuraaga commented Dec 15, 2020 •

edited

Loading

bogdandrutu commented Dec 15, 2020

anuraaga commented Dec 16, 2020

trask commented Dec 16, 2020

anuraaga Dec 21, 2020

jkwatson Dec 21, 2020

anuraaga Dec 22, 2020

jkwatson Dec 22, 2020

jkwatson Dec 21, 2020

anuraaga Dec 22, 2020

jkwatson Dec 22, 2020

trask Dec 22, 2020

jkwatson Dec 21, 2020

anuraaga Dec 22, 2020

jkwatson Dec 21, 2020

jkwatson Dec 21, 2020

anuraaga Dec 22, 2020

jkwatson Dec 21, 2020

codecov bot commented Dec 22, 2020

jkwatson Dec 22, 2020 •

edited

Loading

jkwatson left a comment

Add a OTLP JSON span exporter to logs. #2295

Add a OTLP JSON span exporter to logs. #2295

Conversation

anuraaga commented Dec 14, 2020

anuraaga commented Dec 14, 2020

jkwatson commented Dec 14, 2020

anuraaga commented Dec 15, 2020

trask commented Dec 15, 2020

anuraaga commented Dec 15, 2020

anuraaga commented Dec 15, 2020 • edited Loading

bogdandrutu commented Dec 15, 2020

anuraaga commented Dec 15, 2020 • edited Loading

bogdandrutu commented Dec 15, 2020

anuraaga commented Dec 16, 2020

trask commented Dec 16, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 22, 2020

Codecov Report

jkwatson Dec 22, 2020 • edited Loading

Choose a reason for hiding this comment

jkwatson left a comment

Choose a reason for hiding this comment

anuraaga commented Dec 15, 2020 •

edited

Loading

anuraaga commented Dec 15, 2020 •

edited

Loading

jkwatson Dec 22, 2020 •

edited

Loading