Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[exporter/kafkaexporter] feat/partition by trace #29660

Merged
merged 9 commits into from
Dec 12, 2023

Conversation

jwafle
Copy link
Contributor

@jwafle jwafle commented Dec 5, 2023

Description: Adds the partition_traces_by_id option to configuration which defaults to false. When set to true, it sets the message key on trace messages to a hexadecimal string representing the trace ID.

Link to tracking Issue: #12318

Testing: Refined current unit tests to cover new capabilities. Config test now covers the partition_traces_by_id option. marshaler_test.go now tests both partitioned and non-partitioned cases.

Documentation: Updated the README to include the new partition_traced_by_id option in the section describing optional configuration items.

Copy link

linux-foundation-easycla bot commented Dec 5, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

update config tests

improve marshaler test
@jwafle
Copy link
Contributor Author

jwafle commented Dec 10, 2023

Hi @MovieStoreGuy, @evan-bradley, I know it's been a while on this issue, but would really appreciate this feature. Would it be possible to get a review on this?

Thanks!

@jwafle
Copy link
Contributor Author

jwafle commented Dec 11, 2023

Sorry @MovieStoreGuy, I am not super familiar with OpenTelemetry's Collector's CI pipeline, so my changes were not passing the lint check for some variable names, had to add a changelog gen yaml file, and I was missing a replace statement for the batchpersignal dependency. All of those things should be addressed now.

Also, I noticed in the logs of this check that it calls for me to:

Run make crosslink
cd /home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/internal/tools && go build -o /home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/.tools/crosslink -trimpath go.opentelemetry.io/build-tools/crosslink
Executing crosslink
/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/.tools/crosslink --root=/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib --prune
diff --git a/exporter/kafkaexporter/go.mod b/exporter/kafkaexporter/go.mod
index 1c25337..565025b 100644
--- a/exporter/kafkaexporter/go.mod
+++ b/exporter/kafkaexporter/go.mod
@@ -100,3 +100,5 @@ replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/pdatatest
 replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/zipkin => ../../pkg/translator/zipkin
 
 replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/golden => ../../pkg/golden
+
+replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/batchpersignal => ../../pkg/batchpersignal
diff --git a/receiver/kafkareceiver/go.mod b/receiver/kafkareceiver/go.mod
index ae5[9](https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/7154549414/job/19510569089#step:11:10)c36..fa593a4 [10](https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/7154549414/job/19510569089#step:11:11)0644
--- a/receiver/kafkareceiver/go.mod
+++ b/receiver/kafkareceiver/go.mod
@@ -107,3 +107,5 @@ replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/pdatatest
 replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/golden => ../../pkg/golden
 
 replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/azure => ../../pkg/translator/azure
+
+replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/batchpersignal => ../../pkg/batchpersignal
Replace statements are out of date, please run "make crosslink" and commit the changes in this PR.
Error: Process completed with exit code 1.

I did add the replace statement into the kafkaexporter that this step calls for. However, it also calls for the make crosslink to add a replace statement in the kafkareceiver, which I have not made any changes to. Not sure if this is going to cause the next CI run for this PR to fail, but I didn't want to add any changes to a component that I had not interacted with yet.

@MovieStoreGuy
Copy link
Contributor

Sorry @MovieStoreGuy, I am not super familiar with OpenTelemetry's Collector's CI pipeline, so my changes were not passing the lint check for some variable names, had to add a changelog gen yaml file, and I was missing a replace statement for the batchpersignal dependency. All of those things should be addressed now.

Also, I noticed in the logs of this check that it calls for me to:

Run make crosslink
cd /home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/internal/tools && go build -o /home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/.tools/crosslink -trimpath go.opentelemetry.io/build-tools/crosslink
Executing crosslink
/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/.tools/crosslink --root=/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib --prune
diff --git a/exporter/kafkaexporter/go.mod b/exporter/kafkaexporter/go.mod
index 1c25337..565025b 100644
--- a/exporter/kafkaexporter/go.mod
+++ b/exporter/kafkaexporter/go.mod
@@ -100,3 +100,5 @@ replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/pdatatest
 replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/zipkin => ../../pkg/translator/zipkin
 
 replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/golden => ../../pkg/golden
+
+replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/batchpersignal => ../../pkg/batchpersignal
diff --git a/receiver/kafkareceiver/go.mod b/receiver/kafkareceiver/go.mod
index ae5[9](https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/7154549414/job/19510569089#step:11:10)c36..fa593a4 [10](https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/7154549414/job/19510569089#step:11:11)0644
--- a/receiver/kafkareceiver/go.mod
+++ b/receiver/kafkareceiver/go.mod
@@ -107,3 +107,5 @@ replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/pdatatest
 replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/golden => ../../pkg/golden
 
 replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/azure => ../../pkg/translator/azure
+
+replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/batchpersignal => ../../pkg/batchpersignal
Replace statements are out of date, please run "make crosslink" and commit the changes in this PR.
Error: Process completed with exit code 1.

I did add the replace statement into the kafkaexporter that this step calls for. However, it also calls for the make crosslink to add a replace statement in the kafkareceiver, which I have not made any changes to. Not sure if this is going to cause the next CI run for this PR to fail, but I didn't want to add any changes to a component that I had not interacted with yet.

You may need to rebase with the main branch and run make crosslink from the root. Rebasing should fix the problem but unfortunately now it is a game of making the CI happy.

@jwafle
Copy link
Contributor Author

jwafle commented Dec 11, 2023

Sorry @MovieStoreGuy, I am not super familiar with OpenTelemetry's Collector's CI pipeline, so my changes were not passing the lint check for some variable names, had to add a changelog gen yaml file, and I was missing a replace statement for the batchpersignal dependency. All of those things should be addressed now.
Also, I noticed in the logs of this check that it calls for me to:

Run make crosslink
cd /home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/internal/tools && go build -o /home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/.tools/crosslink -trimpath go.opentelemetry.io/build-tools/crosslink
Executing crosslink
/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/.tools/crosslink --root=/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib --prune
diff --git a/exporter/kafkaexporter/go.mod b/exporter/kafkaexporter/go.mod
index 1c25337..565025b 100644
--- a/exporter/kafkaexporter/go.mod
+++ b/exporter/kafkaexporter/go.mod
@@ -100,3 +100,5 @@ replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/pdatatest
 replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/zipkin => ../../pkg/translator/zipkin
 
 replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/golden => ../../pkg/golden
+
+replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/batchpersignal => ../../pkg/batchpersignal
diff --git a/receiver/kafkareceiver/go.mod b/receiver/kafkareceiver/go.mod
index ae5[9](https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/7154549414/job/19510569089#step:11:10)c36..fa593a4 [10](https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/7154549414/job/19510569089#step:11:11)0644
--- a/receiver/kafkareceiver/go.mod
+++ b/receiver/kafkareceiver/go.mod
@@ -107,3 +107,5 @@ replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/pdatatest
 replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/golden => ../../pkg/golden
 
 replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/azure => ../../pkg/translator/azure
+
+replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/batchpersignal => ../../pkg/batchpersignal
Replace statements are out of date, please run "make crosslink" and commit the changes in this PR.
Error: Process completed with exit code 1.

I did add the replace statement into the kafkaexporter that this step calls for. However, it also calls for the make crosslink to add a replace statement in the kafkareceiver, which I have not made any changes to. Not sure if this is going to cause the next CI run for this PR to fail, but I didn't want to add any changes to a component that I had not interacted with yet.

You may need to rebase with the main branch and run make crosslink from the root. Rebasing should fix the problem but unfortunately now it is a game of making the CI happy.

Ah, I think I figured it out. I was getting tripped up because I added the batchpersignal as a replace dependency in the go.mod of the kafkaexporter. However, the kafkareceiver is dependent on the kafkaexporter and was missing the same replace statement, so I needed to add it (other modules dependent on kafkaexporter already had the replace for batchpersignal in their go.mods). That tripped the govulncheck and the unittest.

@jwafle
Copy link
Contributor Author

jwafle commented Dec 11, 2023

😭 , sorry last thing. The linter was failing on the import order.

@MovieStoreGuy
Copy link
Contributor

Do you mind fixing up the conflict as well?

@jwafle
Copy link
Contributor Author

jwafle commented Dec 12, 2023

Do you mind fixing up the conflict as well?

Sure thing! Really appreciate your patience. This is my first PR to OTEL contrib 😅

@MovieStoreGuy MovieStoreGuy merged commit b6b6a6f into open-telemetry:main Dec 12, 2023
79 checks passed
@github-actions github-actions bot added this to the next release milestone Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants