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

Add required jaeger env var #1689

Merged
merged 1 commit into from
Jul 30, 2024
Merged

Conversation

julianocosta89
Copy link
Member

@julianocosta89 julianocosta89 commented Jul 30, 2024

Changes

PR #1683 broke Jaeger as https://github.com/jaegertracing/jaeger/releases/tag/v1.59.0 had a breaking change.

This PR adds the missing env var to Jaeger.

@puckpuck we need this env var before getting #1687 merged and released.

Merge Requirements

For new features contributions please make sure you have completed the following
essential items:

  • [ ] CHANGELOG.md updated to document new feature additions
  • [ ] Appropriate documentation updates in the docs
  • Appropriate Helm chart updates in the helm-charts

Maintainers will not merge until the above have been completed. If you're unsure
which docs need to be changed ping the
@open-telemetry/demo-approvers.

@julianocosta89 julianocosta89 requested a review from a team July 30, 2024 10:41
@github-actions github-actions bot added the helm-update-required Requires an update to the Helm chart when released label Jul 30, 2024
@julianocosta89 julianocosta89 mentioned this pull request Jul 30, 2024
@puckpuck
Copy link
Contributor

Hey @julianocosta89 I'm looking over how to implement this in Helm, and looking at the Helm charts themselves, it seems Jaeger wants this implemented by setting COLLECTOR_OTLP_ENABLED to true, and not the environment variable this PR is doing.

@puckpuck
Copy link
Contributor

Here is the relevant code block for the Helm chart. The variable COLLECTOR_OTLP_GRPC_HOST_PORT is only set if the default port of 4317 is not used.

https://github.com/jaegertracing/helm-charts/blob/main/charts/jaeger/templates/collector-deploy.yaml#L65-L76

@julianocosta89
Copy link
Member Author

@puckpuck have you seen the release notes from Jaeger?
https://github.com/jaegertracing/jaeger/releases/tag/v1.59.0

⛔ Breaking Changes
The OTEL Collector upgrade brought in a change where OTLP receivers started listening on localhost instead of 0.0.0.0 as before. As a result, when running in container environment the endpoints are likely unreachable from other containers (Issue jaegertracing/jaeger#5737). The fix will be available in the next release. Meanwhile, the workaround is to instruct Jaeger to listen on 0.0.0.0, as in this fix:
- COLLECTOR_OTLP_GRPC_HOST_PORT=0.0.0.0:4317
- COLLECTOR_OTLP_HTTP_HOST_PORT=0.0.0.0:4318

@julianocosta89
Copy link
Member Author

If the behaviour is the same, I believe we must use the K8S downward API to get the pod_ip:4317.

@puckpuck puckpuck merged commit 363ad46 into open-telemetry:main Jul 30, 2024
30 checks passed
@julianocosta89 julianocosta89 deleted the fix-jaeger branch July 30, 2024 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
helm-update-required Requires an update to the Helm chart when released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants