Add Kafka exporter #1439

pavolloffay · 2020-07-27T07:46:51Z

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

Description:

Notable changes

add Kafka exporter for OTLP model (otlptrace.ResourceSpans)
Uses https://github.com/Shopify/sarama Kafka client

Link to tracking Issue:

Created from #1410 (comment)

Resolves open-telemetry/opentelemetry-collector-contrib#268
Related to #1331

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

Tested locally with Confluent 5.0.0 https://docs.confluent.io/5.0.0/installation/docker/docs/installation/single-node-client.html and #1410

Documentation: < Describe the documentation added.>

Added Kafka exporter readme

codecov · 2020-07-27T09:12:03Z

Codecov Report

Merging #1439 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1439      +/-   ##
==========================================
+ Coverage   90.83%   90.88%   +0.05%     
==========================================
  Files         234      237       +3     
  Lines       16472    16541      +69     
==========================================
+ Hits        14962    15033      +71     
+ Misses       1082     1080       -2     
  Partials      428      428

Impacted Files	Coverage Δ
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%> (ø)`
service/defaultcomponents/defaults.go	`84.31% <100.00%> (+0.31%)`	⬆️
translator/internaldata/resource_to_oc.go	`83.72% <0.00%> (ø)`
consumer/consumererror/permanenterror.go	`100.00% <0.00%> (+22.22%)`	⬆️

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 39521ed...160c9cd. Read the comment docs.

bogdandrutu · 2020-07-27T19:29:02Z

exporter/kafkaexporter/config.go

+type Config struct {
+	configmodels.ExporterSettings `mapstructure:",squash"`
+	// The list of kafka brokers (default localhost:9092)
+	Brokers []string `mapstructure:"brokers"`


Is this required to be a list? what is the behavior?

Kafka client accepts a list of brokers not only a single address. In statically configured environments this can be handy.

It is the initial list of broker addresses that is used to discover all nodes https://docs.confluent.io/current/installation/configuration/consumer-configs.html#bootstrap.servers

exporter/kafkaexporter/config_test.go

exporter/kafkaexporter/factory.go

exporter/kafkaexporter/kafka_exporter.go

bogdandrutu · 2020-07-27T20:11:11Z

exporter/kafkaexporter/kafka_exporter.go

+		}
+		c.Version = version
+	}
+	producer, err := sarama.NewAsyncProducer(config.Brokers, c)


Why async producer vs sync? I would suggest to use sync producer and rely on the queued/retry logic from the exporterhelper if possible.

It is good to have a consistent logic of sending queues and retries across exporters.

I agree on the consistent logic across exporter. We did the same approach in the ES exporter. However this might be suboptimal for a couple of reasons:

multiple serializations if retry happens

driver can immediately retry some failed requests

I have switched to sync producer for now. We can switch to async if we prove that it performs better it is just an impl detail.

exporter/kafkaexporter/kafka_exporter.go

bogdandrutu · 2020-07-27T20:15:19Z

Some high-level concerns/questions:

Using of the async processor
Sending individual ResourceSpan instead of the entire pdata.Traces as ExportTraceServiceRequest

	request := &otlptrace.ExportTraceServiceRequest{
		ResourceSpans: pdata.TracesToOtlp(td),
	}

pavolloffay · 2020-07-28T10:35:09Z

qretry test failed.

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

pavolloffay · 2020-07-28T14:35:24Z

@bogdandrutu I have updated the PR it's ready for the second pass.

bogdandrutu · 2020-07-28T15:03:09Z

exporter/kafkaexporter/config.go

+// Retry defines retry configuration for Metadata.
+type Retry struct {
+	// The total number of times to retry a metadata request when the
+	// cluster is in the middle of a leader election or at startup (default 3).
+	Max int `mapstructure:"max"`
+	// How long to wait for leader election to occur before retrying
+	// (default 250ms). Similar to the JVM's `retry.backoff.ms`.
+	BackOff time.Duration `mapstructure:"backoff"`
+}


use RetrySettings from exporter helper.

This configuration is not for retrying failed batches to the broker.

This struct configures retries for getting the metadata from the broker. This happens at the startup or when the broker is in the middle of the leader election. This setting is useful when there is race condition at broker and producer startup e.g .in docker compose deployment.

also exporterhelper.RetrySettings does not define maximum number of retries [int] and it also exposes other settings that does not apply to Kafka metadata retry.

bogdandrutu · 2020-07-28T15:05:17Z

exporter/kafkaexporter/kafka_exporter.go

+	c.Metadata.Full = config.Metadata.Full
+	c.Metadata.Retry.Max = config.Metadata.Retry.Max
+	c.Metadata.Retry.Backoff = config.Metadata.Retry.BackOff


For consistency with other protocols, I would suggest to use this https://github.com/open-telemetry/opentelemetry-collector/blob/master/exporter/exporterhelper/queued_retry.go logic to control queuing and retry and disable native retries from the sarama client.

See my comment above #1439 (comment)

bogdandrutu

I think overall is very close to be done.

bogdandrutu · 2020-07-28T16:04:24Z

exporter/kafkaexporter/README.md

+Kafka exporter exports traces to Kafka.
+
+The following settings are required:
+- `protocol_version` (no default): Kafka protocol version e.g. 2.0.0


Please mention in the readme what is the proto that we write to the queue.

There will be a follow-up PR to make that configurable.

Since we already import Jaeger and Zipkin we should make the encoding configurable. This will allow folks easy migration.

bogdandrutu · 2020-07-29T01:32:04Z

@pavolloffay there are few other comments not addressed. Please fix that and we are good to go.

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

pavolloffay · 2020-07-29T07:07:07Z

@bogdandrutu I have addressed the comments and added more docs especially explaining that metadata retry is not the retry for failed messages.

bogdandrutu · 2020-07-29T14:33:48Z

@pavolloffay will merge and I will do some cleanups.

…y#1439) Bumps [boto3](https://github.com/boto/boto3) from 1.21.33 to 1.21.34. - [Release notes](https://github.com/boto/boto3/releases) - [Changelog](https://github.com/boto/boto3/blob/develop/CHANGELOG.rst) - [Commits](boto/boto3@1.21.33...1.21.34) --- updated-dependencies: - dependency-name: boto3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

pavolloffay requested review from bogdandrutu, dmitryax, james-bebbington, nilebox, owais, pjanotti and tigrannajaryan as code owners July 27, 2020 07:46

project-bot bot added this to In progress in Collector Jul 27, 2020

pavolloffay mentioned this pull request Jul 27, 2020

Add Kafka receiver #1410

Merged

bogdandrutu reviewed Jul 27, 2020

View reviewed changes

pavolloffay force-pushed the kafka-exporter branch 3 times, most recently from 437dc0a to d5c4ece Compare July 28, 2020 08:14

pavolloffay added 5 commits July 28, 2020 12:36

Add Kafka exporter

d1482c8

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

Fix move load config

c529768

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

Fix some review comments

ea81a13

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

Switch to sync producer

4a428d2

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

Expose metadata settings

db1fc33

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

pavolloffay force-pushed the kafka-exporter branch from 733ab4b to db1fc33 Compare July 28, 2020 10:36

pavolloffay added 3 commits July 28, 2020 13:05

Fix typo

e875628

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

trivial rename

5955521

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

docs

a38b6be

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

bogdandrutu reviewed Jul 28, 2020

View reviewed changes

bogdandrutu self-assigned this Jul 28, 2020

pavolloffay added 3 commits July 29, 2020 08:56

Add more commeents

3c7192d

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

more docs

590d68b

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

docs

160c9cd

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

Collector automation moved this from In progress to Reviewer approved Jul 29, 2020

bogdandrutu approved these changes Jul 29, 2020

View reviewed changes

bogdandrutu merged commit 1d31f0f into open-telemetry:master Jul 29, 2020

Collector automation moved this from Reviewer approved to Done Jul 29, 2020

MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021

Remove CircleCI config (open-telemetry#1439)

716ad08

jpkrohling mentioned this pull request Dec 9, 2021

Some components are missing CODEOWNERS open-telemetry/opentelemetry-collector-contrib#3870

Closed

yurishkuro mentioned this pull request Nov 27, 2022

Feature: Added Support accepting OTLP via Kafka jaegertracing/jaeger#4049

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Kafka exporter #1439

Add Kafka exporter #1439

pavolloffay commented Jul 27, 2020

codecov bot commented Jul 27, 2020 •

edited

bogdandrutu Jul 27, 2020

pavolloffay Jul 28, 2020

bogdandrutu Jul 27, 2020

pavolloffay Jul 28, 2020

pavolloffay Jul 28, 2020

bogdandrutu commented Jul 27, 2020 •

edited

pavolloffay commented Jul 28, 2020

pavolloffay commented Jul 28, 2020

bogdandrutu Jul 28, 2020

pavolloffay Jul 29, 2020

pavolloffay Jul 29, 2020

bogdandrutu Jul 28, 2020

pavolloffay Jul 29, 2020

bogdandrutu left a comment

bogdandrutu Jul 28, 2020

pavolloffay Jul 28, 2020

bogdandrutu commented Jul 29, 2020

pavolloffay commented Jul 29, 2020

bogdandrutu commented Jul 29, 2020

Add Kafka exporter #1439

Add Kafka exporter #1439

Conversation

pavolloffay commented Jul 27, 2020

codecov bot commented Jul 27, 2020 • edited

Codecov Report

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 commented Jul 27, 2020 • edited

pavolloffay commented Jul 28, 2020

pavolloffay commented Jul 28, 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

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

bogdandrutu commented Jul 29, 2020

pavolloffay commented Jul 29, 2020

bogdandrutu commented Jul 29, 2020

codecov bot commented Jul 27, 2020 •

edited

bogdandrutu commented Jul 27, 2020 •

edited