OTLP Zipkin Collector #5

marcingrzejszczak · 2024-06-26T13:40:17Z

Code + tests for OTLP encoding and collecting. Tests using OTel OTLP exporters against OTLP Zipkin Collector

HTTP sender works fine, GRPC is failing

anuraaga

Just a couple of comments on the Armeria portion, didn't look in detail at other parts

anuraaga · 2024-06-27T00:23:31Z

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

+          try (ReadBuffer readBuffer = ReadBuffer.wrapUnsafe(nioBuffer)) {
+            try {
+              InputStream inputStream = readBuffer;
+              if ("gzip".equalsIgnoreCase(req.headers().get("content-encoding"))) {


We should add DecodingService in reconfigure instead of handling content-encoding ourselves

https://github.com/line/armeria/blob/main/core/src/main/java/com/linecorp/armeria/server/encoding/DecodingService.java#L49

It doesn't support base64 in it's default decoders list, but I've never seen that encoding before and don't see it mentioned in otlp docs (it just mentions gzip) so suspect the default is fine.

Tried to remember why we don't use DecodingService in zipkin-server and couldn't (looked through openzipkin/zipkin#2348). It might be related to metrics there but I can't see any reason to avoid it here.

Great hint, I've updated the PR

anuraaga · 2024-06-27T00:25:21Z

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

+              } else if ("base64".equalsIgnoreCase(req.headers().get("content-encoding"))) {
+                inputStream = Base64.getDecoder().wrap(content.toInputStream());
+              }
+              ExportTraceServiceRequest request = ExportTraceServiceRequest.parseFrom(inputStream);


Without needing to decode input streams, you should be able to parse the ByteBuffer directly with CodedInputStream.newInstance(nioBuffer)

How would that look like? I don't want to misuse the API 😬

Assuming I didn't miss anything, I think it's just that

ExportTraceServiceRequest request = ExportTraceServiceRequest.parseFrom(CodedInputStream.newInstance(nioBuffer));

Don't need ReadBuffer, it's a InputStream so currently the parsing is going through protobuf's slow InputStream parsing path

Actually that's even simpler - ExportTraceServiceRequest request = ExportTraceServiceRequest.parseFrom(content.toInputStream());

Ah yeah that's the least lines but I think CodedInputStream.newInstance(content.byteBuf().nioBuffer()) isn't too bad and avoids going through the InputStream version of protobuf parsing. It's not terrible but since we've already aggregated, better to avoid it.

OK, fixed that. Thank you

anuraaga · 2024-06-27T08:46:48Z

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

+import java.util.Base64;
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.zip.GZIPInputStream;


BTW just randomly noticed it seems unused imports aren't hitting CI, not sure if it's intended or not

I can't make the build fail because of that locally either 🤔

codefromthecrypt · 2024-06-28T00:08:09Z

marking this draft until it is mergable without conflict. helps me remember what's ready to review vs not.

codefromthecrypt · 2024-06-28T00:10:45Z

when polishing this later, consider @reta recent PR which had a nice description README updates etc. basically we want to get to a point where merging this won't cause work for others to go back and polish it, add docs, etc openzipkin/zipkin#3765

reta · 2024-06-28T00:20:22Z

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

+            List<Span> spans = SpanTranslator.translate(request);
+            collector.collector.accept(spans, result);
+          } catch (IOException e) {
+            throw new RuntimeException(e);


~~The method throws Exception, do we need to wrap it into RuntimeException?~~ Lost track this is lambda, probably new UncheckedIOException(ex) would be better here

reta · 2024-06-28T00:25:58Z

module/pom.xml

+    <dependency>
+      <groupId>com.fasterxml.jackson.core</groupId>
+      <artifactId>jackson-annotations</artifactId>
+      <version>2.17.0</version>


Suggested change

<version>2.17.0</version>

<version>2.17.1</version>

reta · 2024-06-28T00:26:48Z

pom.xml

+
+    <assertj.version>3.25.3</assertj.version>
+    <awaitility.version>4.2.1</awaitility.version>
+    <junit-jupiter.version>5.10.2</junit-jupiter.version>


Suggested change

<junit-jupiter.version>5.10.2</junit-jupiter.version>

<junit-jupiter.version>5.10.3</junit-jupiter.version>

ps marcin added a different PR for this stuff, so there's a conflict. If you prefer to do a re-bump pass PR, I'll review it as he needs to rebase this anyway

reta · 2024-06-28T00:27:40Z

translation-otel/src/main/java/zipkin2/translation/zipkin/AttributesExtractor.java

+import java.util.Map;
+
+/**
+ * LabelExtractor extracts the set of OTel Span labels equivalent to the annotations in a


Suggested change

* LabelExtractor extracts the set of OTel Span labels equivalent to the annotations in a

* AttributesExtractor extracts the set of OTel Span labels equivalent to the annotations in a

reta · 2024-06-28T00:28:08Z

translation-otel/src/main/java/zipkin2/translation/zipkin/LinkUtils.java

+import java.util.regex.Pattern;
+import java.util.stream.Collectors;
+
+class LinkUtils {


Suggested change

class LinkUtils {

final class LinkUtils {

marcingrzejszczak · 2024-06-28T09:18:23Z

OK, I've done everything that was requested of me, I've applied all the review suggestions. I think that there's no point in delaying the review of this any further.

making · 2024-09-25T14:22:06Z

@marcingrzejszczak can you please close this pr as #13 has already been merged.

marcingrzejszczak added 24 commits June 12, 2024 17:36

WIP

facbedc

WIP

c5bad8b

HTTP sender works fine, GRPC is failing

WIP

37ef965

Passing build

0f7ee5a

Collectors are working fine

45c1d40

HTTP collector tests passing

538da4f

Added OTel HTTP exporter test

a810184

Polish

81e36ed

Removed OTel GRPC module

d55db5f

Removed OTel GRPC module

f0680f4

Removed OTel GRPC module

b766e59

WIP on ported tests from OTel

3639e4d

Fixed tests

7738906

Trying to make the tests pass

c868600

Trying to make the tests pass

02c49f1

SpanTranslator tests

437f9e9

Added moar tests

d9faf6a

Added Zipkin tests

d0d2fe2

Added links

dbbe43c

Removed tests module

791175f

Fixed translation from OTel to Zipkin

dc7b762

Removing GRPC

f30ade5

Merged removal of GRPC branch

cf9c6dc

Left only Zipkin OTLP collector

d64ff9b

marcingrzejszczak added this to the 0.0.1 milestone Jun 26, 2024

marcingrzejszczak requested a review from codefromthecrypt June 26, 2024 13:40

marcingrzejszczak mentioned this pull request Jun 26, 2024

OTLP support via HTTP and GRPC #2

Closed

5 tasks

anuraaga reviewed Jun 27, 2024

View reviewed changes

marcingrzejszczak added 2 commits June 27, 2024 09:58

Version bumps

e5bc2a4

Merge branch 'main' into receiver

afe15ae

marcingrzejszczak added 2 commits June 27, 2024 10:01

Merge branch 'versionBumps' into receiver

03ac1ea

Uses decorators instead of manually decoding payload

bcbfc36

anuraaga reviewed Jun 27, 2024

View reviewed changes

marcingrzejszczak added 3 commits June 27, 2024 11:14

Optimize imports

265e373

Simplified Collector logic

c706abb

Improved byte decoding

579ef19

codefromthecrypt marked this pull request as draft June 28, 2024 00:07

reta reviewed Jun 28, 2024

View reviewed changes

marcingrzejszczak added 3 commits June 28, 2024 09:31

Merge branch 'main' into receiver

45b26b4

Changes following the review

d87046d

Polish

cb1387e

marcingrzejszczak marked this pull request as ready for review June 28, 2024 09:17

codefromthecrypt closed this Sep 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OTLP Zipkin Collector #5

OTLP Zipkin Collector #5

marcingrzejszczak commented Jun 26, 2024

anuraaga left a comment

anuraaga Jun 27, 2024

marcingrzejszczak Jun 27, 2024

anuraaga Jun 27, 2024

marcingrzejszczak Jun 27, 2024

anuraaga Jun 27, 2024

marcingrzejszczak Jun 27, 2024

anuraaga Jun 27, 2024

marcingrzejszczak Jun 27, 2024

anuraaga Jun 27, 2024

marcingrzejszczak Jun 27, 2024

codefromthecrypt commented Jun 28, 2024

codefromthecrypt commented Jun 28, 2024

reta Jun 28, 2024 •

edited

Loading

reta Jun 28, 2024

reta Jun 28, 2024

codefromthecrypt Jun 28, 2024

reta Jun 28, 2024

reta Jun 28, 2024

marcingrzejszczak commented Jun 28, 2024

making commented Sep 25, 2024

	<junit-jupiter.version>5.10.2</junit-jupiter.version>
	<junit-jupiter.version>5.10.3</junit-jupiter.version>

	* LabelExtractor extracts the set of OTel Span labels equivalent to the annotations in a
	* AttributesExtractor extracts the set of OTel Span labels equivalent to the annotations in a

OTLP Zipkin Collector #5

OTLP Zipkin Collector #5

Conversation

marcingrzejszczak commented Jun 26, 2024

anuraaga 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

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 commented Jun 28, 2024

codefromthecrypt commented Jun 28, 2024

reta Jun 28, 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

marcingrzejszczak commented Jun 28, 2024

making commented Sep 25, 2024

reta Jun 28, 2024 •

edited

Loading