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

Fix Kafka and Collector config for macs #74

Merged
merged 2 commits into from
Sep 9, 2024

Conversation

andrewvc
Copy link
Collaborator

@andrewvc andrewvc commented Sep 9, 2024

Neither were working right, in the case of kafka this disables some problematic tracing options and in the case of the collector this fixes the config yaml

Neither were working right, in the case of kafka this disables some problematic tracing options
and in the case of the collector this fixes the config yaml
@@ -618,6 +613,7 @@ services:
- OTEL_RESOURCE_ATTRIBUTES
- OTEL_SERVICE_NAME=kafka
- KAFKA_HEAP_OPTS=-Xmx400m -Xms400m
- OTEL_INFERRED_SPANS_ENABLED=false
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried just changing this in the Dockerfile for the image, but even after deleting the image it was still there.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will be the demo uses the public published images by default: https://github.com/elastic/opentelemetry-demo/pkgs/container/opentelemetry-demo

To test it locally, you would need to rebuild the image and override the image name in the corresponding docker-compose

Copy link
Collaborator

@rogercoll rogercoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrewvc Thanks for adding this!

I see that most of the changes are in the docker-compose.yml file, which we try not to change/diverge from the upstream fork. The upstream repository is heavily forked and to improve the maintainability (we have nightly auto-syncs) the forked versions rely mostly on environment variables, specifically the .env.override file. Another example is the Dockerfiles to generate the images, to override the OpenTelemetry agent with the Elastic distribution one, we added some custom Dockerfile.elastic in the corresponding directories instead of changing the upstream one.

Could you move the OTEL_INFERRED_SPANS_ENABLED=false to the src/kafka/Dockerfile.elastic file? We will build and publish the new images asap.
Also, could you undo the formatting changes in the docker-compose file in this PR? I think it would be better suited for an upstream PR

@@ -618,6 +613,7 @@ services:
- OTEL_RESOURCE_ATTRIBUTES
- OTEL_SERVICE_NAME=kafka
- KAFKA_HEAP_OPTS=-Xmx400m -Xms400m
- OTEL_INFERRED_SPANS_ENABLED=false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will be the demo uses the public published images by default: https://github.com/elastic/opentelemetry-demo/pkgs/container/opentelemetry-demo

To test it locally, you would need to rebuild the image and override the image name in the corresponding docker-compose

@rogercoll
Copy link
Collaborator

I see that OTEL_INFERRED_SPANS_ENABLED=false is already on the Kafka Dockerfile, then I think that if we undo the changes in the docker-compose.yml file we should be good to go.

@andrewvc
Copy link
Collaborator Author

andrewvc commented Sep 9, 2024

@rogercoll I've reverted the docker-compose changes!

@rogercoll rogercoll merged commit 6989226 into elastic:main Sep 9, 2024
21 of 22 checks passed
@andrewvc andrewvc deleted the fix-for-macs branch September 9, 2024 16:54
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.

2 participants