Make Kafka payload encoding configurable #1584

pavolloffay · 2020-08-18T15:44:37Z

Signed-off-by: Pavol Loffay ploffay@redhat.com

Description:

This PR makes configurable encoding of Kafka message payloads. I have added support for Jaeger proto and JSON for both receiver and exporter and also Zipkin JSON and thrift for receiver to cover the functionality we had in Jaeger. This will allow users to migrate to the OTEL collector and also Jaeger project to use this new implementation directly.

In addition to that additional encodings can be programmatically added via a registry.

Link to tracking Issue:

Resolves #1580

Testing: < Describe what testing was performed and which tests were added.>

Documentation: < Describe the documentation added.>

Added to readme

codecov · 2020-08-18T15:52:30Z

Codecov Report

Merging #1584 into master will increase coverage by 0.05%.
The diff coverage is 95.62%.

@@            Coverage Diff             @@
##           master    #1584      +/-   ##
==========================================
+ Coverage   91.97%   92.03%   +0.05%     
==========================================
  Files         254      259       +5     
  Lines       17286    17418     +132     
==========================================
+ Hits        15899    16030     +131     
- Misses        989      990       +1     
  Partials      398      398

Impacted Files	Coverage Δ
exporter/kafkaexporter/otlp_marshaller.go	`77.77% <77.77%> (ø)`
exporter/kafkaexporter/jaeger_marshaller.go	`90.00% <90.00%> (ø)`
receiver/kafkareceiver/zipkin_unmarshaller.go	`92.85% <92.85%> (ø)`
exporter/kafkaexporter/factory.go	`100.00% <100.00%> (ø)`
exporter/kafkaexporter/kafka_exporter.go	`100.00% <100.00%> (ø)`
exporter/kafkaexporter/marshaller.go	`100.00% <100.00%> (ø)`
receiver/kafkareceiver/factory.go	`100.00% <100.00%> (ø)`
receiver/kafkareceiver/jaeger_unmarshaller.go	`100.00% <100.00%> (ø)`
receiver/kafkareceiver/kafka_receiver.go	`100.00% <100.00%> (ø)`
receiver/kafkareceiver/otlp_unmarshaller.go	`100.00% <100.00%> (ø)`
... and 8 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 b13392b...cf9ee8e. Read the comment docs.

objectiser

Just some initial comments about naming.

objectiser · 2020-08-18T15:54:52Z

exporter/kafkaexporter/README.md

+- `encoding` (default = otlp_proto): The encoding of the payload sent to kafka. Available encodings:
+  - `otlp_proto`: the payload is deserialized to `ExportTraceServiceRequest`.
+  - `jaeger_proto_span`: the payload is deserialized to a single Jaeger proto `Span`.
+  - `jaeger_json_span`: the payload is deserialized to a single Jaeger JSON Span using `jsonpb`.


Just for consistency, it might be better to remove the _span from the above jaeger values, or add _span to the zipkin ones, as they are all related to spans.

For Jaeger encoding I wanted to empasize that payload is a single span which can have perf implications I assume. However this is the encoding Jaeger upstream uses.

Ok that is reasonable, hadn't understood the difference between jaeger and zipkin (in terms of single versus multiple spans), but clear now.

exporter/kafkaexporter/config.go

objectiser · 2020-08-18T15:57:43Z

exporter/kafkaexporter/factory.go

-	defaultTopic  = "otlp_spans"
-	defaultBroker = "localhost:9092"
+	typeStr         = "kafka"
+	defaultTopic    = "otlp_spans"


Would it be better to have a generic defaultTopic not perceived to be associated with the encoding? e.g. just spans

Please create a separate issue for this. Ideally, this should not clash with other topic names used in OSS systems Zipkin/Jaeger.

I assume that the non OTLP encodings will be used in legacy deployments that already use different topic names (e.g. jaeger-spans, zipkin).

I guess it is reasonable to have a default topic aligned with the default encoding - so if someone is changing the encoding they are likely to change the topic, so nevermind.

receiver/kafkareceiver/README.md

pavolloffay · 2020-08-19T06:26:31Z

The CI failure does not seem to be related to this PR.

pavolloffay · 2020-08-19T13:07:14Z

@bogdandrutu would you like to review this one too?

exporter/kafkaexporter/README.md

bogdandrutu

Not sure why we need a public mechanism to register marshaler

bogdandrutu · 2020-08-19T17:20:49Z

exporter/kafkaexporter/marshaller.go

+var marshallers = map[string]Marshaller{}
+
+// GetMarshaller gets a Marshaller for encoding or nil if no marshaller is registered.
+func GetMarshaller(encoding string) Marshaller {


Does this need to be public?

I want to make this extensible for people extending the collector. Anybody who imports a custom format (e.g. in exporter) can easily support Kafka transport.

Let's not do this prematurely. If there is a request we will consider to do then. Any public API is extra overhead to maintain

This extensibility is something we're looking to make use of in hyptertrace, so it would be great if these could be kept public.

Ok, so if we need to do this it is anyway necessary to compile again the code with the new marshaler. Can we make the Kafka factory accept the custom encodings then?

Essentially avoid the global state if possible

We can do that, I wanted to make it simpler to provide custom encodings. The base idea was that the registry could be called from any place e.g. exporter that imports custom data model.

Done, I have exposed (un)marshalers in the factory.

exporter/kafkaexporter/marshaller.go

pavolloffay · 2020-08-24T14:40:12Z

Can somebody restart the build?

The failure does not seem to be related to this PR

ERROR: 1 dead links found!
[✖] https://cortexmetrics.io/docs/apis/ → Status: 404

marianoao · 2020-08-24T15:12:44Z

I think that zipkin allows several encodings of kafka marshalled spans:

https://github.com/openzipkin/zipkin/blob/master/zipkin/src/main/java/zipkin2/codec/SpanBytesDecoder.java

zipkin v1 json
zipkin v2 json
thrift
proto3

pavolloffay · 2020-08-24T18:49:58Z

@marianoao the proto support is not listed here https://github.com/openzipkin/zipkin/tree/master/zipkin-collector/kafka#encoding-spans-into-kafka-messages. After this PR it's easy to add additional encodings. This PR adds Zipkin JSON v2 and Thrift (list of spans).

Also this PR does not add Zipkin encodings to exporter, again, it's very easy to add them since we should have all model translators in place.

@adriancole could you loop in here? Is the proto encoding supporter in zipkin kafka?

codefromthecrypt · 2020-08-25T01:06:55Z

yeah kafka (and other transports like activemq and rabbit) support proto also

pavolloffay · 2020-08-25T06:35:42Z

yeah kafka (and other transports like activemq and rabbit) support proto also

I will submit support for proto in the follow-up PR.

@bogdandrutu this PR is ready for your review. The CI seems flaky and needs a restart.

bogdandrutu · 2020-08-26T04:36:23Z

service/defaultcomponents/defaults.go

@@ -90,7 +90,7 @@ func Components() (
 		jaegerexporter.NewFactory(),
 		fileexporter.NewFactory(),
 		otlpexporter.NewFactory(),
-		kafkaexporter.NewFactory(),
+		kafkaexporter.NewFactory(kafkaexporter.DefaultMarshallers()),


Can the marshaler be optional?

Done in the next commit, the CI failure does not seem to be related to this PR.

bogdandrutu · 2020-08-26T15:13:35Z

@pavolloffay you need to rebase, the failing test is TestGRPCReceptionWithTLS in jaeger which had the expired certificates and most likely you did not rebase since then so will always fail.

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay · 2020-08-26T16:01:41Z

Rebased

pavolloffay · 2020-08-26T16:32:53Z

The build failed again on the contrib test...

…metry#1584) Bumps [github.com/jaegertracing/jaeger](https://github.com/jaegertracing/jaeger) from 1.33.0 to 1.34.1. - [Release notes](https://github.com/jaegertracing/jaeger/releases) - [Changelog](https://github.com/jaegertracing/jaeger/blob/v1.34.1/CHANGELOG.md) - [Commits](jaegertracing/jaeger@v1.33.0...v1.34.1) --- updated-dependencies: - dependency-name: github.com/jaegertracing/jaeger dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…telemetry#1584)

pavolloffay requested review from bogdandrutu, dmitryax, james-bebbington, nilebox, owais, pjanotti and tigrannajaryan as code owners August 18, 2020 15:44

objectiser reviewed Aug 18, 2020

View reviewed changes

bogdandrutu reviewed Aug 19, 2020

View reviewed changes

exporter/kafkaexporter/README.md Outdated Show resolved Hide resolved

pavolloffay force-pushed the kafka-encoding branch from 5db337f to 3b849e3 Compare August 19, 2020 17:03

bogdandrutu reviewed Aug 19, 2020

View reviewed changes

jrcamp assigned bogdandrutu Aug 19, 2020

pavolloffay force-pushed the kafka-encoding branch from 20ac0f4 to 507dd0a Compare August 24, 2020 15:00

bogdandrutu approved these changes Aug 26, 2020

View reviewed changes

bogdandrutu reviewed Aug 26, 2020

View reviewed changes

pavolloffay added 4 commits August 26, 2020 18:01

Make Kafka payload encoding configurable

9eec63f

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Expose marshalers in factory

8071993

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Remove zipkin from exporter readme

6a917c2

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Make custom encodings optional in the factory

cf9ee8e

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay force-pushed the kafka-encoding branch from d2e82af to cf9ee8e Compare August 26, 2020 16:01

bogdandrutu merged commit 7f13eb6 into open-telemetry:master Aug 26, 2020

pavolloffay mentioned this pull request Aug 27, 2020

Support zipkin proto in Kafka receiver #1646

Merged

Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024

[Metrics SDK] Add Metrics ExemplarFilter and ExemplarReservoir (open-…

7724361

…telemetry#1584)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Kafka payload encoding configurable #1584

Make Kafka payload encoding configurable #1584

pavolloffay commented Aug 18, 2020

codecov bot commented Aug 18, 2020 •

edited

Loading

objectiser left a comment

objectiser Aug 18, 2020

pavolloffay Aug 18, 2020

objectiser Aug 18, 2020

objectiser Aug 18, 2020

pavolloffay Aug 18, 2020

pavolloffay Aug 18, 2020

objectiser Aug 18, 2020

pavolloffay commented Aug 19, 2020

pavolloffay commented Aug 19, 2020

bogdandrutu left a comment

bogdandrutu Aug 19, 2020

pavolloffay Aug 20, 2020

bogdandrutu Aug 20, 2020

davexroth Aug 21, 2020

bogdandrutu Aug 22, 2020 •

edited

Loading

pavolloffay Aug 24, 2020

pavolloffay Aug 24, 2020

pavolloffay commented Aug 24, 2020

marianoao commented Aug 24, 2020

pavolloffay commented Aug 24, 2020 •

edited

Loading

codefromthecrypt commented Aug 25, 2020

pavolloffay commented Aug 25, 2020

bogdandrutu Aug 26, 2020

pavolloffay Aug 26, 2020

bogdandrutu commented Aug 26, 2020

pavolloffay commented Aug 26, 2020

pavolloffay commented Aug 26, 2020

Make Kafka payload encoding configurable #1584

Make Kafka payload encoding configurable #1584

Conversation

pavolloffay commented Aug 18, 2020

codecov bot commented Aug 18, 2020 • edited Loading

Codecov Report

objectiser 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

pavolloffay commented Aug 19, 2020

pavolloffay commented Aug 19, 2020

bogdandrutu 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

bogdandrutu Aug 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Aug 24, 2020

marianoao commented Aug 24, 2020

pavolloffay commented Aug 24, 2020 • edited Loading

codefromthecrypt commented Aug 25, 2020

pavolloffay commented Aug 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdandrutu commented Aug 26, 2020

pavolloffay commented Aug 26, 2020

pavolloffay commented Aug 26, 2020

codecov bot commented Aug 18, 2020 •

edited

Loading

bogdandrutu Aug 22, 2020 •

edited

Loading

pavolloffay commented Aug 24, 2020 •

edited

Loading