Add OTLP/HTTP collector #13

making · 2024-08-29T07:13:34Z

This pull request adds an OTLP/HTTP implementation of the Zipkin Collector.

It mostly follows the following document, but does not implement fallback for remoteEndpoint name and deprecated attribute names have been changed to new ones.
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk_exporters/zipkin.md

Also, the mapping of resource attributes is TBD in this document, and is not implemented in this PR, but I would like to open a separate issue to discuss the mapping of resource attributes.

codefromthecrypt

Lookin good. did you test integrating the module yet?

codefromthecrypt · 2024-08-30T00:15:10Z

collector-http/src/main/java/zipkin2/collector/otel/http/OpenTelemetryHttpCollector.java

    return this;
  }

-  @Override public String toString() {
+  @Override
+  public String toString() {
    return "OpenTelemetryHttpCollector{}";


do we want to name this similar to the sender?

Can you elaborate this?

codefromthecrypt · 2024-08-30T00:16:23Z

collector-http/src/main/java/zipkin2/collector/otel/http/OpenTelemetryHttpCollector.java

+    @Override
+    protected HttpResponse doPost(ServiceRequestContext ctx, HttpRequest req) {
+      CompletableCallback result = new CompletableCallback();
+      req.aggregate(AggregationOptions.usePooledObjects(ByteBufAllocator.DEFAULT, ctx.eventLoop()


will let @anuraaga @kojilin or @minwoox review this part ;)

codefromthecrypt · 2024-08-30T00:18:15Z

collector-http/src/main/java/zipkin2/collector/otel/http/SpanTranslator.java

+   * Taken from OpenTelemetry codebase.
+   * https://github.com/open-telemetry/opentelemetry-java/blob/3e8092d086967fa24a0559044651781403033313/api/all/src/main/java/io/opentelemetry/api/internal/OtelEncodingUtils.java
+   */
+  static class OtelEncodingUtils {


hmm I feel we have some utilities like HexCodec or similar somewhere..

codefromthecrypt · 2024-08-30T00:20:27Z

collector-http/src/main/java/zipkin2/collector/otel/http/SpanTranslator.java

+     * @param traceIdBytes the bytes (16-byte array) representation of the {@code TraceId}.
+     * @return the lowercase hex (base16) representation of the {@code TraceId}.
+     */
+    static String traceIdFromBytes(byte[] traceIdBytes) {


maybe we can convert the bytes to longs, then reduces the amount of copy/paste as we can use this? zipkin2.Span

fixed in ce708bc and 3dbd79a

codefromthecrypt · 2024-08-30T00:21:44Z

collector-http/src/test/java/zipkin2/collector/otel/http/ITOpenTelemetryHttpCollector.java

+    }
+    Awaitility.waitAtMost(Duration.ofSeconds(5))
+        .untilAsserted(() -> assertThat(store.acceptedSpanCount()).isEqualTo(size));
+    List<List<zipkin2.Span>> received = store.getTraces(traceIds).execute();


codefromthecrypt · 2024-08-30T00:22:16Z

collector-http/src/test/java/zipkin2/collector/otel/http/ITOpenTelemetryHttpCollector.java

+      assertThat(span.tags()).containsEntry("int", "100");
+      assertThat(span.tags()).containsEntry("double", "10.5");
+      assertThat(span.tags()).containsEntry("boolean", "true");
+      assertThat(span.tags()).containsEntry("array", "[\"a\",\"b\",\"c\"]");


interesting to serialize the json, but guess that's how otel does it. Before otel we just joined on comma as easier on indexes.

While https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/common/README.md#attribute
says

For protocols that do not natively support non-string values, non-string values SHOULD be represented as JSON-encoded strings. For example, the expression int64(100) will be encoded as 100, float64(1.5) will be encoded as 1.5, and an empty array of any type will be encoded as [].

the test-case doesn't have square brackets https://github.com/open-telemetry/opentelemetry-java/blob/main/exporters/zipkin/src/test/java/io/opentelemetry/exporter/zipkin/OtelToZipkinSpanTransformerTest.java#L382-L385

I thought OtelToZipkinSpanTransformer was not implemented as documented, but do you thing aligning the existing implementation is better?

yes align with existing impl and make a comment of the source line you are referring to and that it is understood to be not the same as the otel spect. The reason is that the experience is worse to json encode, since folks have comma splitters. I suspect that may be why upstream in otel they are not json encoding.

fixed in 420507b

collector-http/src/main/java/zipkin2/collector/otel/http/OpenTelemetryHttpCollector.java

making · 2024-08-30T00:55:49Z

Lookin good. did you test integrating the module yet?

Not yet. I don't think I understand the concept of modules yet 😅

…elemetryHttpCollector.java Co-authored-by: minux <[email protected]>

codefromthecrypt · 2024-09-01T03:31:37Z

Not yet. I don't think I understand the concept of modules yet 😅

no worries that part is simpler for me to help with vs what you've done!

kojilin · 2024-09-01T05:24:45Z

collector-http/src/main/java/zipkin2/collector/otel/http/OpenTelemetryHttpCollector.java

+          }
+          catch (IOException e) {
+            LOG.log(Level.WARNING, "Unable to parse the request:", e);
+            result.onError(new UncheckedIOException(e));


Curious if not rethrow, just use result.onError(e); is better?

agree. fixed in b162311

…ArgumentsException.

codefromthecrypt · 2024-09-02T05:04:21Z

collector-http/src/main/java/zipkin2/collector/otel/http/SpanTranslator.java

+    byte[] traceIdBytes = spanData.getTraceId().toByteArray();
+    long high = bytesToLong(traceIdBytes, 0);
+    if (high == 0) {
+      spanBuilder.traceId(INVALID_TRACE);


invalid spans should end up being dropped instead I think. You can look at collectormetrics and usage for an example.

@codefromthecrypt

What does "collectormetrics" refer to?

If I create a span with an invalid ID in Zipkin it throws an IllegalArgumentException. Does that mean that catch the exception and increment the "drop" counter metric?

like this https://github.com/openzipkin/zipkin/blob/91976d83578a4a4bd22f2d3ddf637c36d8a27c54/zipkin-collector/rabbitmq/src/test/java/zipkin2/collector/rabbitmq/ITRabbitMQCollector.java#L113

Increment spanDropped if the received span is invalid in c869d69

codefromthecrypt · 2024-09-02T06:17:13Z

ps when in doubt direction matters

in collector we are receiving data in a foreign format and putting it into ours. since we define our api and format, it follows that if plain comma separated is a norm in zipkin, we should coerce to that (it is)

on the other side if we are exporting to a foreign format, we should follow that format's conventions as much as possible for the same albeit visa versa reason. (e.g. bytesmessageformat for brave)

hope this helps

codefromthecrypt · 2024-09-13T01:59:56Z

collector-http/src/main/java/zipkin2/collector/otel/http/SpanTranslator.java

+          try {
+            spans.add(generateSpan(span, scope, resourceSpans.getResource()));
+          }
+          catch (RuntimeException e) {


personally i would capture at call site calling translator if you can, as this is more like any other issue about malformed inclusing bad trace ID but not limited to that

If it is caught outside of the translator call, the valid spans in the trace data may also be lost. Is that expected?

I think so, as consider if it were json and one span was in incorrect format, you would lose the message. I think bugs should be fixed basically

say I'm wrong, we get an issue asking for a different behavior also. meanwhile there isn't code at possibly not the right abstraction ;)

The IOException that occurs when converting a protobuf payload to a Java object occurs before calling translate and is handled. This runtime exception occurs when converting an otel span to a zipkin span, and may be an illegal argument or null pointer exception. If a span with an invalid value is inserted among multiple valid spans, is it okay to drop all of them? This is a rare case, so I don't really care.

probably one thing that I didn't highlight is that when in doubt we can refer to the actual collector.java which handles exception and error the same way https://github.com/openzipkin/zipkin/blob/master/zipkin-collector/core/src/main/java/zipkin2/collector/Collector.java#L171-L180

basically what I mean is that error handling and stats handling were all done in collector.java. it is true that we cannot always re-use that type, but the behavior expectations we can borrow from it anyway

Yes, I looked at the implementation of Collector and left incrementing up to Collector as much as possible. I thought I would accept valid spans as much as possible. However, if it's not a good design for Translator to depend on CollectorMetrics, I'll count Dropped outside of Translator.

consider my opinion non binding basically i feel this code is a little cleaner not injecting collectormetrics to it. i will approve either way. your call

basically i feel this code is a little cleaner not injecting collectormetrics to it.

Agreed.

I considered passing a callback to decouple, but that would be too much for some rare cases, so I simply caught the exception outside the translator.
5195c30

codefromthecrypt · 2024-09-16T02:37:10Z

thanks for all the hard work @making and also thanks for the help reviewing @kojilin and @minwoox!

making · 2024-09-16T03:12:22Z

Thanks all!

making added 2 commits August 29, 2024 16:07

Add OTLP/HTTP collector

7b23b75

Remove unused opentelemetry-sdk-common

b5015c0

codefromthecrypt reviewed Aug 30, 2024

View reviewed changes

minwoox approved these changes Aug 30, 2024

View reviewed changes

making and others added 5 commits August 30, 2024 09:59

Update collector-http/src/main/java/zipkin2/collector/otel/http/OpenT…

6f2bec2

…elemetryHttpCollector.java Co-authored-by: minux <[email protected]>

Update collector-http/src/main/java/zipkin2/collector/otel/http/OpenT…

88b3977

…elemetryHttpCollector.java Co-authored-by: minux <[email protected]>

Update collector-http/src/main/java/zipkin2/collector/otel/http/OpenT…

88a5f95

…elemetryHttpCollector.java Co-authored-by: minux <[email protected]>

Fix import

9ac6f61

Fix test

e94d942

kojilin reviewed Sep 1, 2024

View reviewed changes

making added 5 commits September 2, 2024 11:44

Use long to set trace/span id and remove OtelEncodingUtils

ce708bc

Unwrap UncheckedIOException

b162311

Use comma separator instead of json to encode attributes

420507b

Makes no sense to set an id with zeros, as this will throw an Illegal…

3dbd79a

…ArgumentsException.

Remove unused INVALID_TRACE

9f67806

codefromthecrypt reviewed Sep 2, 2024

View reviewed changes

Increment spanDropped if the received span is invalid

c869d69

codefromthecrypt reviewed Sep 13, 2024

View reviewed changes

codefromthecrypt approved these changes Sep 13, 2024

View reviewed changes

Handling invalid span exceptions outside the Translator

5195c30

codefromthecrypt merged commit 96a3550 into openzipkin-contrib:main Sep 16, 2024
2 checks passed

making deleted the otlp-collector branch September 16, 2024 03:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OTLP/HTTP collector #13

Add OTLP/HTTP collector #13

making commented Aug 29, 2024 •

edited

Loading

codefromthecrypt left a comment

codefromthecrypt Aug 30, 2024

making Sep 2, 2024

codefromthecrypt Aug 30, 2024

codefromthecrypt Aug 30, 2024

codefromthecrypt Aug 30, 2024

making Sep 2, 2024 •

edited

Loading

codefromthecrypt Aug 30, 2024

codefromthecrypt Aug 30, 2024

making Aug 30, 2024

codefromthecrypt Sep 1, 2024

making Sep 2, 2024

making commented Aug 30, 2024

codefromthecrypt commented Sep 1, 2024

kojilin Sep 1, 2024 •

edited

Loading

making Sep 2, 2024

codefromthecrypt Sep 2, 2024 •

edited

Loading

making Sep 9, 2024

codefromthecrypt Sep 11, 2024

making Sep 12, 2024

codefromthecrypt commented Sep 2, 2024

codefromthecrypt Sep 13, 2024

making Sep 13, 2024

codefromthecrypt Sep 13, 2024

codefromthecrypt Sep 13, 2024

making Sep 13, 2024

codefromthecrypt Sep 13, 2024 •

edited

Loading

codefromthecrypt Sep 13, 2024

making Sep 13, 2024

codefromthecrypt Sep 13, 2024

making Sep 13, 2024 •

edited

Loading

codefromthecrypt commented Sep 16, 2024

making commented Sep 16, 2024

Add OTLP/HTTP collector #13

Add OTLP/HTTP collector #13

Conversation

making commented Aug 29, 2024 • edited Loading

codefromthecrypt left a 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

making Sep 2, 2024 • edited Loading

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

making commented Aug 30, 2024

codefromthecrypt commented Sep 1, 2024

kojilin Sep 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt Sep 2, 2024 • edited Loading

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

codefromthecrypt commented Sep 2, 2024

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

codefromthecrypt Sep 13, 2024 • edited Loading

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

making Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

codefromthecrypt commented Sep 16, 2024

making commented Sep 16, 2024

making commented Aug 29, 2024 •

edited

Loading

making Sep 2, 2024 •

edited

Loading

kojilin Sep 1, 2024 •

edited

Loading

codefromthecrypt Sep 2, 2024 •

edited

Loading

codefromthecrypt Sep 13, 2024 •

edited

Loading

making Sep 13, 2024 •

edited

Loading